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

Support use with linkers/setups needing rpath-link #8

Merged
merged 2 commits into from
May 7, 2024

Conversation

drbenmorgan
Copy link
Contributor

Discovered in testing of integration of G4VG in apt-sim/AdePT#289, targets linking to installed G4VG may fail to link due to linker not having needed information on location of deps-of-deps of G4VG. This happens in build environments where

  1. The installed dependencies (here, Geant4, VecGeom) do not have their rpath set correctly.
  2. They are otherwise in a location the linker is not aware of, e.g. via ldconfig or LD_LIBRARY_PATH etc.

The link error warns about this and suggests adding the path through, e.g. -rpath-link. CMake will always add this flag/path correctly, but only if it knows about the dependencies (even if private).

The G4VG library only depends on the static Celeritas::geocel target, which does not forward its dependencies to Geant4/Vecgeom and so it does not set -rpath-link for these in downstream users

Workaround this issue by explicitly making used Geant4 targets PRIVATE deps of the g4vg target. Refind Geant4/VecGeom dependencies when finding G4VG so these are resolved and available if the linker requires it.

Add smoke test for linking that simply tries to link only the G4VG target. Confirmed to reproduce problem observed in AdePT, with the fixes above resolving it.

Despite the above linked CMake post, this feels somewhat hacky/messy and am not 100% sure it's the right approach as

  1. It doesn't appear to be needed pre GCC 12
  2. It feels like the issue is more having a valid build environment set up - i.e. that everything you are going to link to directly or transitively is visible to the linker directly through linker paths or the r/runpath of the thing needing linking. It's not been spotted before due to the primary use of Spack, which sets r/runpaths for each package and target(s) within those packages correctly.

However, it does seem like G4VG needs to at least declare directly its Geant4/VecGeom deps so these populate the IMPORTED_LINK_DEPENDENT_LIBRARIES property noted in the CMake Discourse discussion.

Discovered in testing of integration of G4VG in apt-sim/AdePT#289,
targets linking to installed G4VG may fail to link due to linker
not having needed information on location of deps-of-deps of G4VG.
This happens in build environments where

1. The installed dependencies (here, Geant4, VecGeom) do not have
   their rpath set correctly.
2. They are otherwise in a location the linker is not aware of,
   e.g. via ldconfig of LD_LIBRARY_PATH etc.

The link error warns about this and suggests adding the path through,
e.g. `-rpath-link`. CMake will always add this flag/path correctly,
but only if it knows about the dependencies (even if private).

The G4VG library only depends on the static Celeritas::geocel target,
which does not forward its dependencies to Geant4/Vecgeom and so
it does not set `-rpath-link` for these.

Workaround this issue by explicitly making used Geant4 targets
PRIVATE deps of the g4vg target. Refind Geant4/VecGeom dependencies
when finding G4VG so these are resolved and available if the linker
requires it.

Add smoke test for linking that simply tries to link only the G4VG
target. Confirmed to reproduce problem observed in AdePT, with
the fixes above resolving it.
@drbenmorgan drbenmorgan requested a review from sethrj May 7, 2024 16:54
@sethrj
Copy link
Member

sethrj commented May 7, 2024

@drbenmorgan Have you used libtree before? It would be instructive to add tree output of the app before and after, maybe even with a "correct" build environment and one without.

I know from experience it's not good practice to manually propagate upstream library dependencies for shared libraries: it ends up prolonging startup time because it has O(N^2) load instructions for dependencies of depth N. (Geant4, Trilinos, and other scientific software "packages" of libraries are guilty of this.)

Since this app is mainly for the benefit of the AdePT group I think it's OK to patch this in.

src/CMakeLists.txt Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
@drbenmorgan
Copy link
Contributor Author

drbenmorgan commented May 7, 2024

Thanks for the link to libtree @sethrj! I certainly agree on minimising shared lib propagation, so if you think a "set up the correct build environment" is the better route, I'd be happy to not have the changes here merged in (and likewise if you have recommendations for better dependency hiding in Geant4, be happy to discuss that) 😆 !

Edit: i.e. fix the problem by setting up the build environments correctly rather than hacking CMake/buildscripts...

@sethrj
Copy link
Member

sethrj commented May 7, 2024

I think it's the age-old question of whether to enforce correctness or be lax at the cost of making it someone else's problem... the purpose of this library is to make the code more accessible as opposed to being pedantic 😉

@sethrj sethrj merged commit f372cdd into celeritas-project:main May 7, 2024
3 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.

2 participants