Skip to content
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

update CUDA easyblock to add CUPTI and nvvm library directories to $LIBRARY_PATH #3516

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

trz42
Copy link
Contributor

@trz42 trz42 commented Nov 24, 2024

When using RPATH linking (e.g., in EESSI) we need to ensure that all directories containing shared libraries must be listed in $LIBRARY_PATH. Otherwise the RPATH linker wrapper would not add these directories to the RPATH header, and thus the libraries cannot be resolved by the dynamic linker later.

Here, we add two directories to LIBRARY_PATH such that corresponding prepend_path commands are added to the module file for CUDA. These directories are known to the easyblock, but currently only added to $LD_LIBRARY_PATH which is filtered in EESSI.

To restrict the change to certain environments, it is only used when LD_LIBRARY_PATH is a filtered environment variable and LIBRARY_PATH is not.

@@ -356,10 +356,18 @@ def make_module_req_guess(self):
lib_path.append(os.path.join('nvvm', 'lib64'))
inc_path.append(os.path.join('nvvm', 'include'))

library_path = ['lib64', os.path.join('stubs', 'lib64')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want the stubs in the final RPATH, these need to come from the OS at runtime

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, this is explicitly filtered in framework (I am pretty sure...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be much simpler than this, there's no need for the checking, you can just directly set

'LIBRARY_PATH': lib_path + [os.path.join('stubs', 'lib64')],

below. The stubs location is filtered out by the rpath wrappers at link time (which is correct).

Copy link
Member

@ocaisa ocaisa Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference between the link libraries and runtime libraries should be the stubs location

Copy link
Contributor Author

@trz42 trz42 Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can always add extras/CUPTI/lib64 and nvvm/lib64 to LIBRARY_PATH, then we can implement it as you suggested.

BTW, while it may not be relevant anymore, CUPTI and nvvm lib dirs should only be added for CUDA 7 or newer. That's currently not the case in the PR, but would be following your suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directories being added are just guesses, if they don't exist they won't be included in the final module file (I think...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented suggestions in 5077104

@boegel boegel changed the title add CUPTI and nvvm lib dirs to LIBRARY_PATH update CUDA easyblock to add CUPTI and nvvm library directories to $LIBRARY_PATH Dec 3, 2024
@boegel boegel added bug fix EESSI Related to EESSI project labels Dec 3, 2024
@boegel boegel added this to the release after 4.9.4 milestone Dec 3, 2024
@boegel boegel requested a review from ocaisa December 3, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix EESSI Related to EESSI project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants