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 #18

Merged
merged 8 commits into from
Dec 5, 2024

Conversation

billysuh7
Copy link
Contributor

@billysuh7 billysuh7 commented Nov 2, 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.

xref: conda-forge/cuda-feedstock#10

We're patching both DSO and executables in this feedstock.

patchelf --force-rpath --set-rpath $ORIGIN/../lib:$ORIGIN/../targets/x86_64-linux/lib $PREFIX/compute-sanitizer/compute-sanitizer
patchelf --force-rpath --set-rpath $ORIGIN/../lib:$ORIGIN/../targets/x86_64-linux/lib $PREFIX/compute-sanitizer/TreeLauncherSubreaper
patchelf --force-rpath --set-rpath $ORIGIN/../lib:$ORIGIN/../targets/x86_64-linux/lib $PREFIX/compute-sanitizer/TreeLauncherTargetLdPreloadHelper
patchelf --force-rpath --set-rpath $ORIGIN $PREFIX/compute-sanitizer/libInterceptorInjectionTarget.so
patchelf --force-rpath --set-rpath $ORIGIN $PREFIX/compute-sanitizer/libsanitizer-collection.so
patchelf --force-rpath --set-rpath $ORIGIN $PREFIX/compute-sanitizer/libsanitizer-public.so
patchelf --force-rpath --set-rpath $ORIGIN $PREFIX/compute-sanitizer/libTreeLauncherPlaceholder.so
patchelf --force-rpath --set-rpath $ORIGIN $PREFIX/compute-sanitizer/libTreeLauncherTargetInjection.so
patchelf --force-rpath --set-rpath $ORIGIN $PREFIX/compute-sanitizer/libTreeLauncherTargetUpdatePreloadInjection.so

@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.

@billysuh7 billysuh7 marked this pull request as ready for review November 2, 2024 03:14
@billysuh7 billysuh7 requested a review from a team as a code owner November 2, 2024 03:14
@billysuh7 billysuh7 marked this pull request as draft November 3, 2024 04:30
@jakirkham
Copy link
Member

Seeing a few warnings on CI. In general they look like this

WARNING (cuda-sanitizer-api,compute-sanitizer/libInterceptorInjectionTarget.so): $RPATH/libgcc_s.so.1 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

Can we take a closer look at RPATH patching here to make sure we are addressing this case?

@billysuh7
Copy link
Contributor Author

WARNING (cuda-sanitizer-api,compute-sanitizer/libInterceptorInjectionTarget.so): $RPATH/libgcc_s.so.1 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

Can we take a closer look at RPATH patching here to make sure we are addressing this case?

Apparently this is a side effect of using RPATH in our libs. I see similar warning in cuda-nvtx and others. As long as we have a symlink in $PREFIX/lib it seems to behave OK.

Example:

From the actual location, libsanitizer-collection.so points to the standard location for libgcc_s.so.1: /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f11ece75000)

~/miniforge3/envs/test6/compute-sanitizer$ ldd libsanitizer-collection.so
	linux-vdso.so.1 (0x00007ffc8cfa5000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f11ecff4000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f11ecfea000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f11ecfe4000)
	libutil.so.1 => /lib/x86_64-linux-gnu/libutil.so.1 (0x00007f11ecfdf000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f11ece90000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f11ece75000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f11ecc81000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f11ed8db000)

But from $PREFIX/lib where the symlinks are, libsanitizer-collection.so points to the expected version of libgcc_s.so.1: /home/bsuh/miniforge3/envs/test6/lib/./libgcc_s.so.1

~/miniforge3/envs/test6/lib$ ldd libsanitizer-collection.so
	linux-vdso.so.1 (0x00007fff5859e000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f9bc7353000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f9bc7349000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f9bc7343000)
	libutil.so.1 => /lib/x86_64-linux-gnu/libutil.so.1 (0x00007f9bc733e000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f9bc71ef000)
	libgcc_s.so.1 => /home/bsuh/miniforge3/envs/test6/lib/./libgcc_s.so.1 (0x00007f9bc71d0000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f9bc6fdc000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f9bc7c3a000)

I see parallels with cuda-nvtx which has the RPATH fix.

WARNING (cuda-nvtx,targets/x86_64-linux/lib/libnvToolsExt.so.1.0.0): $RPATH/libstdc++.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist.
WARNING (cuda-nvtx,targets/x86_64-linux/lib/libnvToolsExt.so.1.0.0): $RPATH/libgcc_s.so.1 not found in packages, sysroot(s) nor the missing_dso_whitelist.

From the actual location, libstdc++.so.6 and libgcc_s.so.1 point to the standard location.

~/miniforge3/envs/conda-test/targets/x86_64-linux/lib$ ldd libnvToolsExt.so.1.0.0
	linux-vdso.so.1 (0x00007fff159ef000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fb09297e000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fb09295b000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fb092951000)
	libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fb09276f000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fb092620000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fb092605000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fb092411000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fb092ba4000)

From $PREFIX/lib, they point to the symlink destination under /home/bsuh/miniforge3/envs/conda-test/lib/

~/miniforge3/envs/conda-test/lib$ ldd libnvToolsExt.so.1.0.0
	linux-vdso.so.1 (0x00007ffd09fd5000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f8382256000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f8382233000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f8382229000)
	libstdc++.so.6 => /home/bsuh/miniforge3/envs/conda-test/lib/./libstdc++.so.6 (0x00007f8382047000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f8381ef8000)
	libgcc_s.so.1 => /home/bsuh/miniforge3/envs/conda-test/lib/./libgcc_s.so.1 (0x00007f8381ed9000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f8381ce5000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f838247c000)

We go by the expectation that libraries are correctly loaded only from $PREFIX/lib, so I believe this is OK. When I look at the WARNINGs closely, during conda build, DSO libs are being tested with their actual location e.g. **cuda-nvtx,targets/x86_64-linux/lib/**libnvToolsExt.so.1.0.0, **compute-sanitizer/**libInterceptorInjectionTarget.so

@jakirkham jakirkham self-requested a review November 8, 2024 23:58
@jakirkham
Copy link
Member

In the last build on main, am seeing the following on CI:

INFO (cuda-sanitizer-api,compute-sanitizer/libInterceptorInjectionTarget.so): Needed DSO lib/libgcc_s.so.1 found in conda-forge/linux-64::libgcc==14.1.0=h77fa898_1

So it seemed to pick up the dependency before. Maybe it is worth seeing if there is some change we made here that is affecting the result

@billysuh7
Copy link
Contributor Author

In the last build on main, am seeing the following on CI:

INFO (cuda-sanitizer-api,compute-sanitizer/libInterceptorInjectionTarget.so): Needed DSO lib/libgcc_s.so.1 found in conda-forge/linux-64::libgcc==14.1.0=h77fa898_1

So it seemed to pick up the dependency before. Maybe it is worth seeing if there is some change we made here that is affecting the result

Thanks for the catch! The main branch has RPATH value of $ORIGIN/../lib whereas my change has RPATH value of $ORIGIN. I realize reverting to $ORIGIN/../lib works better for this feedstock. The DSOs are under $PREFIX/compute-sanitizer/, same level as $PREFIX/lib/, so $ORIGIN/../lib would always point back to $PREFIX/lib and never go outside the environment, whether we start with $PREFIX/lib (in real life) or $PREFIX/compute-sanitizer (during conda build).

This is unlike other feedstocks where DSOs are under target/<platform>/lib, and so we had to avoid RPATH value of $ORIGIN/../../../lib (as in conda-forge/cuda-nvtx-feedstock#2) which causes RPATH value to go outside the environment in real life.

In hindsight, $ORIGIN/../lib is how we patch the binary executables under $PREFIX/bin/, the same level as $PREFIX/lib, so it makes sense that we do the same here - and this eliminated the 'not found' warning. Please let me know if this sounds OK.

…able as this could be part of CUDA with many other libs such that it may find useful to search in $ORIGIN/../${targetsDir}/lib
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! 🙏

Agree the approach above sounds reasonable given the different structure in this feedstock

Had a couple minor suggestions below

billysuh7 and others added 2 commits December 4, 2024 18:10
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Dec 4, 2024

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.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). Your recipe may not receive automatic updates and/or may not be compatible with conda-forge's infrastructure. Please check the logs for more information and ensure your recipe can be parsed.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12171281242. Examine the logs at this URL for more detail.

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! 🙏

@jakirkham jakirkham merged commit 0510143 into conda-forge:main Dec 5, 2024
6 checks passed
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