-
Notifications
You must be signed in to change notification settings - Fork 210
Fix support for NVVM from conda on Windows + other fixes #563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
0c71c35
also search for conda nvvm on windows
leofang a14d95e
fix comment for conda nvvm on linux
leofang 21a7fb4
fix path loop & dll name
leofang a0baf71
Ensure mod_path is always defined when used. Make DLL search order co…
rwgk 03e6e4a
Fix bug in previous commit: need to break out of loop over suffix if …
rwgk 98d7a91
Move LOAD_LIBRARY_SEARCH_* constants outside loop.
rwgk 1a05efc
Fix oversight (forgot to replace one assignment with return)
rwgk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be manually loading this dll instead of modifying
PATH
? Regardless, we don't need to fix this in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvrtc is very special, because of the builtins runtime dependency.
In one of my ChatGPT chats about it a few days ago, it suggested strongly to do both,
os.environ["PATH"]
update, andos.add_dll_directory(mod_path)
. Therefore I'm carrying that into the path_finder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add: I somehow have it in my mind that @leofang made a remark that we shouldn't load the
builtins
ourselves. Leo, does that make sense?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-loading a DLL works (it's what I did in nvmath). My reason against pre-loading is not because it does not work, but because I am not willing to maintain other libraries' implementation details (dlopen, which has no
DT_NEEDED
entry or package dependency metadata for us to inspect). This can easily go out-of-date as these libraries evolve.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Leo, I linked your comment here:
1344621
We can weigh the pros-and-cons of "soft dependency pre-loading" vs
os.environ["PATH"]
&os.add_dll_directory()
management when the path_finder code is complete.