Don't set `LD_LIBRARY_PATH` on package load by haampie · Pull Request #28354 · spack/spack

haampie · 2022-01-11T13:31:13Z

For me this has been mostly just annoying, cause if I do
spack env activate ... and it sets LD_LIBRARY_PATH, system executables that don't
use rpath will now consider spack-built libs first, and are sometimes
incompatible.

And executables built by Spack already use rpath anyways, so they know
where there libs are.

The only case is maybe when libs are dlopen'ed by say Python, but even
then, I think the libs are generally located because of Python's own
rpaths set by Spack.

Closes #3955 #31731

adamjstewart

This PR will break most Python/SCons packages with external dependencies. Python does not properly support RPATHs.

adamjstewart · 2022-01-11T16:40:16Z

etc/spack/defaults/cray/modules.yaml

-      - LD_LIBRARY_PATH
-    lib64:
-      - LD_LIBRARY_PATH
+modules: {}


If these files are no longer being used they can just be removed

That made the tests fail :( didn't check why, but then I thought it wouldn't hurt as a blueprint

haampie · 2022-01-11T16:57:36Z

Can't PythonPackage set LD_LIBRARY_PATH for link type deps that are meant to be dlopen'ed? Just like PYTHONPATH

adamjstewart · 2022-01-11T17:31:47Z

It can, although that will make it even harder to get rid of setup_dependent_*_environment.

adamjstewart · 2022-01-18T03:21:06Z

etc/spack/defaults/darwin/modules.yaml

-    lib:
-      - DYLD_FALLBACK_LIBRARY_PATH
-    lib64:
-      - DYLD_FALLBACK_LIBRARY_PATH


I understand your concern with LD_LIBRARY_PATH, but this issue does not arise with DYLD_FALLBACK_LIBRARY_PATH. On macOS, there are two env vars that control library selection at load-time: DYLD_LIBRARY_PATH and DYLD_FALLBACK_LIBRARY_PATH. The former acts just like LD_LIBRARY_PATH and can break system exes that aren't RPATHed. The latter acts more like RUNPATH and provides libraries to use only when the libraries aren't found in normal search locations. So I think it's safe to keep these here.

Yeah, reverted that, it's just about linux now.

haampie · 2022-03-07T12:30:57Z

This PR will break most Python packages with external dependencies. Python does not properly support RPATHs.

Is this really true @adamjstewart? Don't python packages usually call C functions through a generated wrapper library that does have rpaths already?

For instance when running ./bin/python directly without activating an environment, and running import numpy, I get:

$ cat /proc/451559/maps | grep openblas
7f7218ad1000-7f7218b40000 r--p 00000000 103:04 40283657                  /home/harmen/spack/opt/spack/linux-ubuntu20.04-zen2/gcc-10.3.0/openblas-0.3.19-ixyrqkwedhttcxokzzczho4zefy7b3ji/lib/libopenblas_zen-r0.3.19.so

so it manages to load libraries without LD_LIBRARY_PATH just fine.

And can you explain what you mean with SCons given that LD_LIBRARY_PATH is runtime.

salotz-sitx · 2022-03-11T15:08:35Z

When I load an environment and LD_LIBRARY_PATH is set this sometimes conflicts with the distro installed packages that depend on dynamic linking via that variable. This can cause segfaults to occur depending on the versions. I haven't experienced a segfault myself (Ubuntu 18.04) but a coworker has with Ubuntu 21.04 with nano.

E.g.:

--> ldd /usr/bin/python3
	linux-vdso.so.1 (0x00007ffc9603d000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fce3ef58000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fce3ed39000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fce3eb35000)
	libutil.so.1 => /lib/x86_64-linux-gnu/libutil.so.1 (0x00007fce3e932000)
	libexpat.so.1 => /home/salotz/scratch/mdutil/.spack-env/view/lib/libexpat.so.1 (0x00007fce3e6fe000)
	libz.so.1 => /home/salotz/scratch/mdutil/.spack-env/view/lib/libz.so.1 (0x00007fce3e4e7000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fce3e149000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fce3f349000)
	libbsd.so.0 => /home/salotz/spack/opt/spack/linux-ubuntu18.04-skylake/gcc-7.5.0/libbsd-0.11.3-rkyd4ndtgypgsbhazhd4dwbflwskzf36/lib/libbsd.so.0 (0x00007fce3df34000)
	libmd.so.0 => /home/salotz/spack/opt/spack/linux-ubuntu18.04-skylake/gcc-7.5.0/libmd-1.0.3-fwt4srl6qp7veimuan65dc23zzobjkpr/lib/libmd.so.0 (0x00007fce3dd28000)

adamjstewart · 2022-03-11T15:39:22Z

Is this really true @adamjstewart? Don't python packages usually call C functions through a generated wrapper library that does have rpaths already?

Most Python libraries use ctypes.util.find_library, which searches typical system locations and environment variables only. Packages like numpy which hardcode the library location in setup.cfg are the exception, not the rule. If you want to reproduce this, try py-shapely, that's the first example that comes to mind. The same is true for most geospatial Python libs.

Can't PythonPackage set LD_LIBRARY_PATH for link type deps that are meant to be dlopen'ed? Just like PYTHONPATH

If we do push through with this PR, we'll have to do this if we don't want to break hundreds of packages.

lib/spack/spack/user_environment.py

adamjstewart · 2022-06-02T15:38:21Z

I know I've been the main person opposing this for years, but I'm starting to warm up to this idea. I think the packages that require LD_LIBRARY_PATH are few enough that we can special case them. Now might be a good time to merge this and see what breaks since we just had a release.

haampie · 2022-07-25T21:07:36Z

So, should we merge it?

adamjstewart · 2022-07-25T22:03:01Z

I'm fine with that but would like to get the opinions of a couple more core devs first.

tgamblin · 2022-08-09T15:33:53Z

I am strongly in favor of this change -- the question is what to do for people who relied on this stuff. Basically I think Spack should work and loading an environment should not affect your compiler or ld.so. So I agree with removing:

LD_LIBRARY_PATH
LIBRARY_PATH
CPATH

The last two AFAIK don't work on all compilers anyway, so it's at best inconsistent to set them.

@haampie: could you modify this, though, so that we set these?

SPACK_LIBRARY_PATH
SPACK_INCLUDE_PATH

to the prior values of LD_LIBRARY_PATH/LIBRARY_PATH and CPATH, respectively? And document it?

That way someone who loads an env will have something they can use to set the variables they need, but we won't be borking the user environment by default.

tgamblin · 2022-08-09T15:49:13Z

I added this here since it's a fairly impactful change: #30634

haampie · 2022-08-09T16:14:38Z

Note that CPATH and LIBRARY_PATH were already removed long ago. To hit the code path in prefix_inspections where you get CPATH and LIBRARY_PATH, you need to delete all modules.yaml config files including $spack/etc/defaults/modules.yaml, which seems unlikely. This PR just fixes some oversights wrt CPATH/LIBRARY_PATH.

So this PR is really about LD_LIBRARY_PATH only.

could you modify this, though, so that we set these?

Do you mean to store <prefix>/lib(64)? in SPACK_LD_LIBRARY_PATH so users can do LD_LIBRARY_PATH=$SPACK_LD_LIBRARY_PATH if they want the previous behavior? I would say users can just change the config files, right? E.g.

spack config add modules:prefix_inspections:lib64:[LD_LIBRARY_PATH]
spack config add modules:prefix_inspections:lib:[LD_LIBRARY_PATH]

Also, can we run a pipeline from scratch using this PR? It affects the builds too (note that modules:prefix_inspections is also used to set up the build environment).

tgamblin · 2022-08-09T16:49:21Z

Do you mean to store /lib(64)? in SPACK_LD_LIBRARY_PATH so users can do LD_LIBRARY_PATH=$SPACK_LD_LIBRARY_PATH if they want the previous behavior?

Yes.

spack config add modules:prefix_inspections:lib64:[LD_LIBRARY_PATH]
spack config add modules:prefix_inspections:lib:[LD_LIBRARY_PATH]

They could do this, but IMO it's a little clunky, and having the env vars allows you do do more -- e.g., you may want to pass the list of transitive library search directories to CMake or use it on your link line or something. I'm basically saying that we should have a convention for that and throw it in a harmless env var (as opposed to a harmful one like now).

Users will be familiar with this from module schemes like OpenHPC, where they have a similar policy. I think it's a good default.

adamjstewart · 2022-08-09T17:20:53Z

I personally agree with @haampie, if people want to set additional env vars in module files, Spack already has a way to do that, we don't need to set SPACK_* env vars for them to use instead.

tgamblin · 2022-08-09T17:41:04Z

I personally agree with @haampie, if people want to set additional env vars in module files, Spack already has a way to do that, we don't need to set SPACK_* env vars for them to use instead.

Ok fair enough -- I'll relent. They can add those settings via spack config. Can that please be documented here, though?

adamjstewart · 2022-08-09T17:43:38Z

Worth documenting in the release notes too since it's a backwards incompatible change.

h-murai · 2022-08-25T07:33:51Z

How about the case where you load a library by Spack and then link your objects with the library?

$ spack load fftw
$ gcc foo.c -lfftw3

Does it work after this change?

jbigot · 2022-09-01T19:49:01Z

How about the case where you load a library by Spack and then link your objects with the library?
$ spack load fftw
$ gcc foo.c -lfftw3
Does it work after this change?

This is my exact use-case (although with en environment) we distribute a spack env that people wanting to hack on our just have to load to start working. I recently noticed LD_LIBRARY_PATH is not set by the env anymore. With cmake, deps are still found ok, but as soon as you try to run the compiled code, it doesn't work anymore.

jbigot · 2022-09-01T19:51:01Z

could a flag be provided to explicitly ask for LD_LIBRARY_PATH to be set?

adamjstewart · 2022-09-01T20:18:13Z

The set of environment variables added by Spack is completely customizable. Although LD_LIBRARY_PATH is no longer added by default, you can add it to your Spack installation like so:

$ spack config add modules:prefix_inspections:lib64:[LD_LIBRARY_PATH]
$ spack config add modules:prefix_inspections:lib:[LD_LIBRARY_PATH]

After this, spack load ... will work identically to the way that it used to. See https://spack.readthedocs.io/en/latest/module_file_support.html#write-a-configuration-file for examples of what this will look like, the above command just writes this file for you. Note that you may need to specify that this is added to your environment, or to a global scope so that it affects all users, not just you.

Does that address your concerns?

jbigot · 2022-09-02T06:43:27Z

thank you for the pointer. Indeed, this should do the trick!

Add option `PIC_ADD_RPATH` to controll if picongpu is setting the rpath. By default PIConGPU will now always use RPATH but optionally allow disabling this and falling back to the old behaviour. Using rpath's is motivated by the spack where `LD_LIBRARY_PATH` is not set anymore: spack/spack#28354

Add option `PIC_ADD_RPATH` to control if picongpu is setting the rpath. By default, PIConGPU will now always use RPATH but optionally allow disabling this and falling back to the old behavior. Using rpath's is motivated by the spack where `LD_LIBRARY_PATH` is not set anymore: spack/spack#28354

Add option `CUDA_MEMTEST__ADD_RPATH` to control if cuda_memtest is setting the rpath. The new default is that RPATH will be used but optionally allow disabling this and falling back to the old behaviori. Using rpath's is motivated by the spack where `LD_LIBRARY_PATH` is not set anymore: spack/spack#28354

Add option `PIC_ADD_RPATH` to control if picongpu is setting the rpath. By default, PIConGPU will now always use RPATH but optionally allow disabling this and falling back to the old behavior. Using rpath's is motivated by the spack where `LD_LIBRARY_PATH` is not set anymore: spack/spack#28354

fspiga · 2022-11-03T14:12:49Z

Today I discovered while I was doing a fresh build, I discovered generated TCL modules do not export anymore LD_LIBRARY_PATH.

Spack has been an extremely useful tool to install packages, self-build modules, expose to users these modules that can be used by users using module load in the old fashion way. Many end users do not really need to know spack exists or learning spack commands.

Please documents very visibly the workaaround

$ spack config add modules:prefix_inspections:lib64:[LD_LIBRARY_PATH]
$ spack config add modules:prefix_inspections:lib:[LD_LIBRARY_PATH]

because a lot of your users and sysadmins will likely need it.

haampie · 2022-11-03T14:21:11Z

Feel free to make a PR

* As discussed at <spack/spack#28354>, spack no longer sets `LD_LIBRARY_PATH`, which makes it harder to run applications compiled by a spack-installed Clacc. This patch adds the spack config workaround suggested there. * Spack's Clacc recipe hasn't been updated since libcxx and libunwind are considered runtimes not projects in LLVM. This patch disables their builds where that causes a failure. * This patch removes the hwloc.h instructions for spack installs on leconte as they no longer appear to be necessary. * This patch updates various ExCL software packages.

spackbot-app bot added defaults documentation Improvements or additions to documentation shell-support labels Jan 11, 2022

haampie force-pushed the fix/dont-set-LD_LIBRARY_PATH-on-load branch from 9bb89ca to 7ebdff6 Compare January 11, 2022 13:46

adamjstewart reviewed Jan 11, 2022

View reviewed changes

haampie mentioned this pull request Jan 12, 2022

libssl version not overriden by packages.yaml ; incompatibility with all system libssl clients then results #28362

Closed

4 tasks

haampie force-pushed the fix/dont-set-LD_LIBRARY_PATH-on-load branch from 7ebdff6 to 8453847 Compare January 12, 2022 09:29

adamjstewart reviewed Jan 18, 2022

View reviewed changes

haampie force-pushed the fix/dont-set-LD_LIBRARY_PATH-on-load branch from 8453847 to ed38dad Compare March 7, 2022 13:46

ChristianTackeGSI reviewed Mar 11, 2022

View reviewed changes

lib/spack/spack/user_environment.py Outdated Show resolved Hide resolved

haampie mentioned this pull request Jul 25, 2022

symbol lookup error: /lib64/libk5crypto.so.3: undefined symbol: EVP_KDF_ctrl, version OPENSSL_1_1_1b #31731

Closed

3 tasks

haampie linked an issue Jul 25, 2022 that may be closed by this pull request

symbol lookup error: /lib64/libk5crypto.so.3: undefined symbol: EVP_KDF_ctrl, version OPENSSL_1_1_1b #31731

Closed

3 tasks

spackbot-app bot added the core PR affects Spack core functionality label Aug 9, 2022

haampie force-pushed the fix/dont-set-LD_LIBRARY_PATH-on-load branch from 39054ff to 12c0a30 Compare August 9, 2022 14:48

haampie requested a review from tgamblin August 9, 2022 14:54

ivan-pi mentioned this pull request Aug 11, 2022

Finding external dependencies in non-standard places fortran-lang/fpm#439

Open

tgamblin changed the title ~~Don't set LD_LIBRARY_PATH on pkg load~~ Don't set LD_LIBRARY_PATH on package load Aug 11, 2022

tgamblin changed the title ~~Don't set LD_LIBRARY_PATH on package load~~ Don't set LD_LIBRARY_PATH on package load Aug 11, 2022

tgamblin approved these changes Aug 11, 2022

View reviewed changes

tgamblin merged commit ceda5fb into spack:develop Aug 11, 2022

haampie deleted the fix/dont-set-LD_LIBRARY_PATH-on-load branch August 15, 2022 07:28

jrwrigh mentioned this pull request Sep 1, 2022

spack environment fails to set LD_LIBRARY_PATH, but only when LD_LIBRARY_PATH isn't set #31051

Closed

3 tasks

jbigot mentioned this pull request Sep 1, 2022

Environment fails pdidev/test_env#6

Closed

greenc-FNAL mentioned this pull request Sep 9, 2022

Changes required for Cetmodules dependencies. FNALssi/spack#31

Merged

MichaelLaufer mentioned this pull request Oct 3, 2022

py-pyfr: add v1.15.0, explicitly set env variables for dependencies #32964

Merged

psychocoderHPC mentioned this pull request Oct 7, 2022

PIConGPU use RPATH by default ComputationalRadiationPhysics/picongpu#4312

Merged

psychocoderHPC mentioned this pull request Oct 7, 2022

use RPATH by default ComputationalRadiationPhysics/cuda_memtest#40

Merged

psychocoderHPC mentioned this pull request Nov 17, 2022

Installing of [email protected] via spack (BOOST/LIBCUDART) ComputationalRadiationPhysics/picongpu#4351

Closed

tgamblin mentioned this pull request Dec 29, 2022

python: remove CPATH from setup_run_environment #34727

Merged

1 task

jrlagrone mentioned this pull request Jun 5, 2023

Spack linking SouthernMethodistUniversity/hpc_docs#51

Closed

TobyBaril mentioned this pull request Jan 18, 2024

Error: package 'GenomeInfoDb' could not be loaded TobyBaril/EarlGrey#79

Closed

donkomura mentioned this pull request Jan 29, 2024

rpath の設定 donkomura/csi-driver-chfs#17

Merged

alephpiece mentioned this pull request Apr 10, 2024

Update packages and the base image ANT-MOC/antmoc-ci#1

Merged

Conversation

haampie commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

haampie Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

haampie commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Jan 11, 2022

Uh oh!

adamjstewart Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

haampie Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

haampie commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salotz-sitx commented Mar 11, 2022

Uh oh!

adamjstewart commented Mar 11, 2022

Uh oh!

Uh oh!

adamjstewart commented Jun 2, 2022

Uh oh!

haampie commented Jul 25, 2022

Uh oh!

adamjstewart commented Jul 25, 2022

Uh oh!

tgamblin commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Aug 9, 2022

Uh oh!

haampie commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Aug 9, 2022

Uh oh!

adamjstewart commented Aug 9, 2022

Uh oh!

tgamblin commented Aug 9, 2022

Uh oh!

adamjstewart commented Aug 9, 2022

Uh oh!

h-murai commented Aug 25, 2022

Uh oh!

jbigot commented Sep 1, 2022

Uh oh!

jbigot commented Sep 1, 2022

Uh oh!

adamjstewart commented Sep 1, 2022

Uh oh!

jbigot commented Sep 2, 2022

Uh oh!

fspiga commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haampie commented Nov 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

haampie commented Jan 11, 2022 •

edited

Loading

haampie commented Jan 11, 2022 •

edited

Loading

haampie commented Mar 7, 2022 •

edited

Loading

tgamblin commented Aug 9, 2022 •

edited

Loading

haampie commented Aug 9, 2022 •

edited

Loading

fspiga commented Nov 3, 2022 •

edited

Loading