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

Add target libraries/executables to executable's RPATH #29

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

billysuh7
Copy link
Contributor

@billysuh7 billysuh7 commented Oct 30, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

These files will be patched:
targets/x86_64-linux/bin/__nvcc_device_query
targets/x86_64-linux/bin/bin2c
targets/x86_64-linux/bin/cudafe++
targets/x86_64-linux/bin/fatbinary
targets/x86_64-linux/bin/nvcc
targets/x86_64-linux/bin/nvlink
targets/x86_64-linux/bin/ptxas

These will be skipped:
patchelf
targets/x86_64-linux/bin/crt/link.stub
targets/x86_64-linux/bin/crt/prelink.stub
targets/x86_64-linux/bin/nvcc.profile

Added recipe/patchelf_exclude.txt to exclude files to patch as they aren't binaries. The conda build will fail if a new binary is added without updating this file.

xref: conda-forge/cuda-feedstock#10

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@jakirkham
Copy link
Member

Seeing the following warning on CI

WARNING (cuda-nvvm-tools,nvvm/bin/cicc): $RPATH/libstdc++.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

Can we take a closer look at how cicc is being patched and make sure it is doing the right thing?

@billysuh7
Copy link
Contributor Author

Seeing the following warning on CI

WARNING (cuda-nvvm-tools,nvvm/bin/cicc): $RPATH/libstdc++.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

Can we take a closer look at how cicc is being patched and make sure it is doing the right thing?

This is all set w/ latest commit pushed - there is no longer 'not found' warning in the output for all platforms

2024-11-06T03:29:59.3341975Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libpthread.so.0 found in CDT/compiler package conda-forge/noarch::sysroot_linux-64==2.17=h4a8ded7_18
2024-11-06T03:29:59.3343173Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/librt.so.1 found in CDT/compiler package conda-forge/noarch::sysroot_linux-64==2.17=h4a8ded7_18
2024-11-06T03:29:59.3355962Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libdl.so.2 found in CDT/compiler package conda-forge/noarch::sysroot_linux-64==2.17=h4a8ded7_18
2024-11-06T03:29:59.3364557Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO lib/libstdc++.so.6 found in conda-forge/linux-64::libstdcxx==14.2.0=hc0a3c3a_1
2024-11-06T03:29:59.3365350Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libm.so.6 found in CDT/compiler package conda-forge/noarch::sysroot_linux-64==2.17=h4a8ded7_18
2024-11-06T03:29:59.3366171Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libc.so.6 found in CDT/compiler package conda-forge/noarch::sysroot_linux-64==2.17=h4a8ded7_18
2024-11-06T03:29:59.3366995Z    INFO (cuda-nvvm-tools,nvvm/bin/cicc): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/ld-linux-x86-64.so.2 found in CDT/compiler package conda-forge/noarch::sysroot_linux-64==2.17=h4a8ded7_1

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Billy! 🙏

Had a couple minor suggestions below

Generally this seems reasonable

recipe/build.sh Show resolved Hide resolved
recipe/build.sh Outdated
Comment on lines 49 to 52
# Do not patch libnvvm.so for now as it gets corrupted by patchelf. See build.sh for details
#elif [[ "${j}" =~ /lib.*/.*\.so($|\.) && ! -L "${j}" ]]; then
# echo patchelf --force-rpath --set-rpath "\$ORIGIN" "${j}" ...
# patchelf --force-rpath --set-rpath "\$ORIGIN" "${j}"
Copy link
Member

Choose a reason for hiding this comment

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

This comment references build.sh for details however this is in build.sh. Are there more details we should add here or can that portion be dropped?

Copy link
Contributor Author

@billysuh7 billysuh7 Nov 6, 2024

Choose a reason for hiding this comment

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

I meant meta.yaml, not build.sh, sorry. But after taking a closer look at how this came about, I found conda-forge/staged-recipes@94a71a6 which was to set binary_relocation to false in order to fix the corruption issue. Therefore I will uncomment the block and patch libnvvm.so. Thank you for raising the question on this!

The corruption they were referring to is conda-forge/staged-recipes#22802 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Ready for review.

@billysuh7
Copy link
Contributor Author

Seeing the following warning on CI

WARNING (cuda-nvvm-tools,nvvm/bin/cicc): $RPATH/libstdc++.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

Can we take a closer look at how cicc is being patched and make sure it is doing the right thing?

cicc was not being patched - it is all set now

@billysuh7
Copy link
Contributor Author

The outstanding questions have been resolved.

@jakirkham jakirkham merged commit 23f9f00 into conda-forge:main Nov 8, 2024
6 checks passed
@jakirkham
Copy link
Member

Thanks Billy! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants