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

autotools: add activation value "non_system_prefix" #32212

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

skosukhin
Copy link
Member

Most of the Autotools packages trust the users and blindly take whatever paths they provide as arguments for the --with-some-package configure options. The configure scripts normally use the path to extend LDFLAGS with -L flags and even -Wl,-rpath, flags. The problem is that we don't want those flags for the system directories. Some time ago, I have implemented a custom activation_value that filters the system paths out. I think that other packages can benefit from that too. We already have the activation value prefix and this PR introduces an additional one, non_system_prefix (note that --with-some-package is the same as --with-some-package=yes, therefore we can make it yes_or_prefix instead).

The problem with the suggested change is that the maintainers of Spack packages (myself included) often forget to cover the case when a dependency is an external from a system prefix and will keep using prefix. Therefore, I think that we should not really introduce non_system_prefix but make prefix do what non_system_prefix does.

I would also change

if activation_value is not None and activation_value(option_value):

to

if activation_value is not None and activation_value(option_value) is not None:

because --with-some-package and --with-some-package= are different things and an empty string returned by a user-provided activation_value might be intended.

@spackbot-app spackbot-app bot added build-systems core PR affects Spack core functionality labels Aug 17, 2022
@haampie
Copy link
Member

haampie commented Aug 17, 2022

Dropping system paths from -L and -rpath is a mistake imho.

When libraries in default paths such as {,/usr}/lib{,64} are shadowed by ones in global config from /etc/ld.so.conf and /etc/ld-musl-<arch>.path, you really need rpaths.

Instead we should prioritize Spack prefixes, which is already done in the compiler wrapper iirc.

Actually, after #31948, we really only need rpaths for system dirs.

@skosukhin
Copy link
Member Author

skosukhin commented Aug 17, 2022

@haampie good point but I have a question then. Why do we try to avoid -L and -rpath flags for the system directories so much that we filter out dependencies from the system prefixes not even once but twice (here and here).

I admit that the compiler wrapper does a very good job de-prioritizing system prefixes (wow, it even understands that -Wl,-rpath,/usr/lib64 is the same as -Wl,-rpath -Wl,/usr/lib64). The problem, however, is that we have no control over the order, in which the build system puts the -L/-rpath flags for the compiler. For example, we have special treatment for externals from non-system prefixes (see here). The build system might reshuffle the order and, I assume, the compiler wrapper will honour that and ignore the de-prioritization of the externals.

As a package maintainer, I always had the impression that once I'm in the Spack build environment, I can do

$CC -O2 smoothie.c -lbanana -lorange

and expect the compiler wrapper to add the missing -I/-L/-rpath flags. The only reason for me to provide the flags to the build system was to make sure that the *.pc and *.la (which we don't have already) files, and things like nc-config and mpicc had the full information to be usable outside of Spack. Actually, -L/usr/lib64 in mpicc of the Spack-provided installation of openmpi on our HPC system is a real problem, which made me dig into this direction. Currently, it does not look like this particular PR is going to fix it but it's rather easy to imagine a situation when it could.

Coming back to banana and orange above. With your example, it looks like I have to check whether the libraries I use are from the system prefixes and add the corresponding -L/-rpath flags if they are. Also, if my assumption above is correct, I should also check whether the dependencies are externals from non-system prefixes and if at least one of them is, specify the -I/-L/-rpath flags to all dependencies in the right order (the directory to an external from non-system prefix should come last).

So, the idea behind this PR is that if Spack "ignores" a directory, it is implied that both the linker at the build time and the dynamic linker at the runtime require no additional information (neither -L/-rpath flags nor DT_RPATH/DT_RUNPATH entries, respectively) to link to the correct library, and, therefore, the build system should not get any prefixes too, just yes.

I'm a bit confused now. I would say that we should make this configurable so that the users of such "strangely" configured systems like in your example could tell Spack not to consider certain prefixes as the system ones.

UPD. Another option to handle cases like in your example could be to add an additional property for the externals in packages.yaml that would tell Spack: "treat me like I'm not in the system prefix although I am".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-systems core PR affects Spack core functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants