-
Notifications
You must be signed in to change notification settings - Fork 310
Add CUDA Stubs path to LIBRARY_PATH. This fixes #1463 #1464
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
Conversation
Also fixes #6602 of easybuilders/easybuild-easyconfigs
easybuild/easyblocks/c/cuda.py
Outdated
| guesses.update({ | ||
| 'PATH': bin_path, | ||
| 'LD_LIBRARY_PATH': lib_path, | ||
| 'LIBRARY_PATH': ['lib64/stubs'], |
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.
This makes LIBRARY_PATH to just check for lib64/stubs. A possible work around is:
'LIBRARY_PATH': lib_path+['lib64/stubs'],
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.
LIBRARY_PATH wasn't set at all before, so I did not want to change too much. Two notes to regarding your suggestion:
lib_path+['lib64/stubs']: There's alib64too much!lib_path: That would also includeextras/CUPTI/lib64andnvvm/lib64resulting inlib64/stubsbeing added there…
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.
Mmmmh, I see LIBRARY_PATH set to pathJoin(root, "lib64") in the module, when installing it without this fix. Where did you see it as not set at all? Or did you meant that it was not explicitly updated in guesses.update? If the latter, it wasn't explicitly mentioned here, but if you update the dictionary you are overwriting the old definition of that key, which was indeed defined.
Regarding 1.: I am not sure what you mean :-?
Regarding 2.: That's correct. Maybe just change it to 'LIBRARY_PATH': ['lib64', 'lib64/stubs'],?
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 probably don't know enough about EasyBuild. Indeed I thought that guesses.update() would be the only definition of LIBRARY_PATH.
- Oh! I was stupied. I forgot that
lib_pathis a list and+just appends the list. For some reason I thought of the+more as a cross product, addinglib64/stubsto every entry oflib_path. - Ok, fine with me!
|
@akesandgren anything to add before we merge this? I kind of need it in for another PR later on. |
|
You should use os.join.path() instead of explicitly using "a/b" |
|
I'll let @akesandgren's remark slip, since the added lines just follow the style of the preceding ones. @damianam Can you look into cleaning that up in your follow-up PR? |
|
That's a good point, but there are other parts of the easyblock that would need to be changed, regardless of the purpose of the PR. I'll merge the PR now, since functionality wise it is all good and Andreas is on holidays. Then I'll make another PR myself to address that suggestion and do something extra I need. |
|
@boegel, sorry to do thread necropsy, but just as a note, adding stubs to If this happens and you don't use This is probably something you need to check when you are using runpath/rpath with easybuild (for Compute Canada, @bartoldeman will have to adjust our linker wrapper or we need to not put the stubs in our |
|
I adjusted our linker wrapper to not convert -L.../stubs into -rpath .../stubs now. I guess the same could be done to https://github.com/easybuilders/easybuild-framework/blob/develop/easybuild/scripts/rpath_args.py which does similar things but I haven't tested EB's rpath support yet. |
|
@mboisson @bartoldeman Please open an issue in framework to get this fixed in our RPATH support, indeed in |
|
done |
Also fixes #6602 of easybuilders/easybuild-easyconfigs (easybuilders/easybuild-easyconfigs#6602)