Skip to content

avoid rpath'ing default search paths#44686

Merged
haampie merged 2 commits intodevelopfrom
fix/system-rpaths
Sep 30, 2024
Merged

avoid rpath'ing default search paths#44686
haampie merged 2 commits intodevelopfrom
fix/system-rpaths

Conversation

@haampie
Copy link
Copy Markdown
Member

@haampie haampie commented Jun 12, 2024

Closes #40848

On sysroot systems like gentoo prefix, as well as nix/guix, our "is
system path" logic is broken cause it's static.

Talking about "the system paths" is not helpful, we have to talk
about default search paths of the dynamic linker instead.

If glibc is recent enough, we can query the dynamic loader's default
search paths, which is a much more robust way to avoid registering
rpaths to system dirs, which can shadow Spack dirs.

This PR adds an additional filter on rpaths the compiler wrapper
adds, dropping rpaths that are default search paths. The PR does
not
remove any of the original is_system_path code, which is
questionable code, but can't hurt much.

(In a follow up PR we could look to register rpaths based only on
the default search paths of the dynamic linker, and get rid of
is_system_path. I don't want to make too many changes at the
same time.)

This fixes issues where build systems run just-built executables
linked against their not-yet-installed libraries, typically:

LD_LIBRARY_PATH=. ./exe

which happens in perl, python, and other non-cmake packages.
If a default path is rpath'ed, it takes precedence over
LD_LIBRARY_PATH, and a system library gets loaded instead
of the just-built library in the stage dir, breaking the build. If
default paths are not rpath'ed, then LD_LIBRARY_PATH takes
precedence, as is desired.

This PR additionally fixes an inconsistency in rpaths between
cmake and non-cmake packages. The cmake build system
computed rpaths by itself, but used a different order than
computed for the compiler wrapper. In fact it's not necessary
to compute rpaths at all, since we let cmake do that thanks to
CMAKE_INSTALL_RPATH_USE_LINK_PATH. This covers rpaths
for all dependencies. The only install rpaths we need to set are
<install prefix>/{lib,lib64}, which cmake unfortunately omits,
although it could also know these. Also, cmake does not
delete rpaths added by the toolchain (i.e. Spack's compiler
wrapper), so I don't think it should be controversial to simplify
things.

@spackbot-app spackbot-app bot added build-environment compilers core PR affects Spack core functionality utilities labels Jun 12, 2024
@haampie haampie force-pushed the fix/system-rpaths branch from 0059405 to 9dfc766 Compare June 12, 2024 12:49
@haampie
Copy link
Copy Markdown
Member Author

haampie commented Jun 13, 2024

This still needs fixes for CMakePackage which doesn't follow the code path that filters the rpaths.

@haampie haampie force-pushed the fix/system-rpaths branch from 9dfc766 to 0ceab05 Compare June 21, 2024 09:30
@haampie haampie force-pushed the fix/system-rpaths branch 2 times, most recently from afe0f6d to 12616de Compare June 21, 2024 10:25
@haampie haampie requested a review from alalazo June 21, 2024 11:45
@haampie
Copy link
Copy Markdown
Member Author

haampie commented Jun 24, 2024

I realize that this may break cmake packages that do not hit the compiler wrapper due to CMAKE_CXX_COMPILER=hipcc. CMAKE_INSTALL_RPATH_USE_LINK_PATH=ON is sufficient to register rpaths to non-project libraries, but will unfortunately not set an rpath to the package's own install prefix. The compiler wrapper registers <prefix>/lib[64].

@Chrismarsh
Copy link
Copy Markdown
Contributor

I rebased this PR on develop and I'm seeing the following build error

==> Installing cmake-3.30.2-ylbdewin44le3plnp77jd2paeodaacpk [73/209]
==> No binary for cmake-3.30.2-ylbdewin44le3plnp77jd2paeodaacpk found: installing from source
==> Fetching https://github.com/Kitware/CMake/releases/download/v3.30.2/cmake-3.30.2.tar.gz
==> Applied patch /globalhome/cbm038/HPC/code/spack/var/spack/repos/builtin/packages/cmake/mr-9623.patch
==> cmake: Executing phase: 'bootstrap'
==> Error: AttributeError: module 'spack.build_environment' has no attribute 'get_rpaths'

The 'cmake' package cannot find an attribute while trying to build from sources. This might be due to a change in Spack's package format to support multiple build-systems for a single package. You can fix this by updating the build recipe, and you can also report the issue as a bug. More information at https://spack.readthedocs.io/en/latest/packaging_guide.html#installation-procedure

/globalhome/cbm038/HPC/code/spack/var/spack/repos/builtin/packages/cmake/package.py:336, in bootstrap_args:
        333        )
        334
        335        # Make CMake find its own dependencies.
  >>    336        rpaths = spack.build_environment.get_rpaths(self)
        337        prefixes = spack.build_environment.get_cmake_prefix_path(self)
        338        args.extend(
        339            [

See build log for details:

@Chrismarsh
Copy link
Copy Markdown
Contributor

So far this PR seems to be working well for me on gentoo.

This change c4dd67b got me back up and running after I rebased this on a more recent develop

What can I test / provide that can help push this PR forward? Being able to use spack with ease on gentoo would let me ditch easy_build

@haampie haampie force-pushed the fix/system-rpaths branch 2 times, most recently from 0fa9693 to b548924 Compare September 12, 2024 13:51
@haampie
Copy link
Copy Markdown
Member Author

haampie commented Sep 12, 2024

OK, so the CMake solution of the PR is to use CMAKE_INSTALL_RPATH only for <prefix>/{lib,lib64}, and rely for the rest on the compiler wrapper and CMAKE_INSTALL_RPATH_USE_LINK_PATH to set rpaths to dependencies.

@haampie haampie force-pushed the fix/system-rpaths branch 2 times, most recently from 20f497c to ad4a42e Compare September 18, 2024 07:22
On sysroot systems like gentoo prefix, as well as nix/guix, our "is
system path" logic is broken cause it's static.

If glibc is recent enough, we can query the dynamic loader's default
search paths, which is a much more robust way to avoid registering
rpaths to "system" dirs.
alalazo
alalazo previously approved these changes Sep 30, 2024
@alalazo alalazo self-assigned this Sep 30, 2024
@haampie haampie merged commit 02499c7 into develop Sep 30, 2024
@haampie haampie deleted the fix/system-rpaths branch September 30, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

Use of RPATH for implicit link directories problematic for build systems that use LD_LIBRARY_PATH=<build dir> ./executable

3 participants