internal/discover: use explicit driver version for matching libraries#1578
internal/discover: use explicit driver version for matching libraries#1578Ronitsabhaya75 wants to merge 1 commit into
Conversation
| logger logger.Interface | ||
| hookCreator HookCreator |
There was a problem hiding this comment.
Question: Why were these members removed?
There was a problem hiding this comment.
Question: Why were these members removed?
@elezar I accidentally removed those members lemme revert those. I'm really sorry for this mistake
| } | ||
| // We use the driver version as a pattern for matching libraries. | ||
| // This pattern is used to identify libraries that are part of the driver. | ||
| cudaVersionPattern := driverVersion |
There was a problem hiding this comment.
Let's just update the strings below to use driverVersion directly.
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Thanks for the contribution addressing a long-outstanding TODO.
I think it looks fine in practice, but needs some minor cleanup.
@elezar I'll work on those reviews and clean it up |
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Thanks. I have some further suggestions.
Please also rebase and squash your changes into a single commit.
| // driverVersion is the version of the driver that is being used. | ||
| driverVersion string | ||
| logger logger.Interface | ||
| hookCreator HookCreator |
There was a problem hiding this comment.
| // driverVersion is the version of the driver that is being used. | |
| driverVersion string | |
| logger logger.Interface | |
| hookCreator HookCreator | |
| logger logger.Interface | |
| hookCreator HookCreator | |
| // driverVersionSuffix is the version of the driver that is being used | |
| // prefixed with a '.' | |
| driverVersionSuffix string |
| Discover: Merge(libraries, xorgLibraries), | ||
| logger: logger, | ||
| hookCreator: hookCreator, | ||
| driverVersion: driverVersion, |
There was a problem hiding this comment.
| Discover: Merge(libraries, xorgLibraries), | |
| logger: logger, | |
| hookCreator: hookCreator, | |
| driverVersion: driverVersion, | |
| Discover: Merge(libraries, xorgLibraries), | |
| logger: logger, | |
| hookCreator: hookCreator, | |
| driverVersionSuffix: "." + driverVersion, |
| func (d graphicsDriverLibraries) isDriverLibrary(filename string, libraryName string) bool { | ||
| // TODO: Instead of `.*.*` we could use the driver version. | ||
| pattern := strings.TrimSuffix(libraryName, ".") + ".*.*" | ||
| pattern := strings.TrimSuffix(libraryName, ".") + "." + d.driverVersion |
There was a problem hiding this comment.
| pattern := strings.TrimSuffix(libraryName, ".") + "." + d.driverVersion | |
| pattern := libraryName + d.driverVersionSuffix |
Note that we never call this function with a libraryName ending in ., so we can remove the additional TrimSuffix to further simplify this.
There was a problem hiding this comment.
@elezar thank you for the review I'll sure work on the commits and once im free from work
|
@Ronitsabhaya75 Please ensure that your commit is signed off. Use |
|
@Ronitsabhaya75 are you able to rebase (and resolve the merge conflicts) and sign off your commit? Thanks. |
Yeah I'm bit busy with my school tests right now and completely forgot about this :( |
Signed-off-by: Ronit Sabhaya <ronitsabhaya75@gmail.com>
|
@rajatchopra @cdesiniotis did the work I'm really sorry I got caught between classes and completely forgot about this :( |
| hookCreator HookCreator | ||
| // driverVersionSuffix is the version of the driver that is being used | ||
| // prefixed with a '.' | ||
| driverVersionSuffix string |
There was a problem hiding this comment.
I don't really see an advantage to using the driverVersionSuffix over the driverVersion as the struct field. The latter reads better IMO and is more "reusable" should we ever want to use the same information for something else in the future.
| func (d graphicsDriverLibraries) isDriverLibrary(filename string, libraryName string) bool { | ||
| // TODO: Instead of `.*.*` we could use the driver version. | ||
| pattern := strings.TrimSuffix(libraryName, ".") + ".*.*" | ||
| pattern := libraryName + d.driverVersionSuffix |
There was a problem hiding this comment.
Adding to my earlier comment, I'd just build the string here by concatenating the libraryName, . and d.driverVersion.
There was a problem hiding this comment.
@elezar It looks like my comment contradicts the review comment you've provided earlier. Let me know if you have any concerns with my reasoning here or If I've missed anything
|
@Ronitsabhaya75 Gentle reminder to address the remaining review comments |
|
Closing as #1948 has been merged. Thank you @Ronitsabhaya75 for your contribution! You have been added as a co-author in the other PR |
5 participants
Previous Behavior
The discovery logic used a generic . wildcard pattern to identify driver libraries. This was less precise and could potentially match unrelated files or incorrect versions.
Current Behavior
The update modifies graphicsDriverLibraries to explicitly use the detected driver version when constructing the match pattern. This ensures that only libraries matching the active driver version are processed, preventing false positives and ensuring consistency.