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 intoNVIDIA:mainfrom
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
internal/discover/graphics.go
Outdated
| } | ||
| // 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.
internal/discover/graphics.go
Outdated
| // 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 |
internal/discover/graphics.go
Outdated
| 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, |
internal/discover/graphics.go
Outdated
| 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
2 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.