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.

Sign up for GitHub

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

stdenv: don't set NIX_LIB*_IN_SELF_RPATH by default #223861

Merged
merged 1 commit into from
May 17, 2023
Merged

stdenv: don't set NIX_LIB*_IN_SELF_RPATH by default #223861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions pkgs/stdenv/generic/setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,12 +311,6 @@ _accumFlagsArray() {
_addRpathPrefix() {
if [ "${NIX_NO_SELF_RPATH:-0}" != 1 ]; then
export NIX_LDFLAGS="-rpath $1/lib ${NIX_LDFLAGS-}"
if [ -n "${NIX_LIB64_IN_SELF_RPATH:-}" ]; then
export NIX_LDFLAGS="-rpath $1/lib64 ${NIX_LDFLAGS-}"
fi
if [ -n "${NIX_LIB32_IN_SELF_RPATH:-}" ]; then
export NIX_LDFLAGS="-rpath $1/lib32 ${NIX_LDFLAGS-}"
fi
fi
}

Expand Down
2 changes: 0 additions & 2 deletions pkgs/stdenv/linux/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ let
''
export NIX_ENFORCE_PURITY="''${NIX_ENFORCE_PURITY-1}"
export NIX_ENFORCE_NO_NATIVE="''${NIX_ENFORCE_NO_NATIVE-1}"
${lib.optionalString (system == "x86_64-linux") "NIX_LIB64_IN_SELF_RPATH=1"}
${lib.optionalString (system == "mipsel-linux") "NIX_LIB32_IN_SELF_RPATH=1"}
Comment on lines -118 to -119
Copy link
Contributor

@trofi trofi Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it's not that easy and depends on specific ports.

In theory non-multilib targets should just use lib. In practice what is a multilib has a moot definition: for example x86_64-linux-glibc is always multilib while x86_64-linux-musl is (or, should be) never multilib.

You can see remnants of multilib in gcc itself:

$ gcc -print-multi-os-directory
../lib64

Sometimes packages are diligent enough to have lib -> lib64 symlink (or the other way around). Sometimes they are not.

The question here is: What API do we expect for nixpkgs to provide and packages to use for non-multilib? Is it always lib or something else (lib64)? Today we do both with lib64 being a canonical path.

This change effectively says lib64 is not considered anymore as a searchpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I follow, and quite possibly this change is too ambitious. My naïve thinking was that given the explicit dependencies of Nix, there shouldn't ever be multiple library search paths, only one set of paths per target.

However, assuming nixpkgs wants to keep the NIX_LIB*_IN_SELF_RPATH API, this change also fixes what I consider a genuine bug: that NIX_LIB*_IN_SELF_RPATH are set according to the build platform, not the target platform.
See #221350 for an example where the builder platform (x86_64-linux vs aarch64-linux) decides whether NIX_LIB*_IN_SELF_RPATH is set, not the target platform which is the same (and non-multilib).

So, can I move the setting of NIX_LIB*_IN_SELF_RPATH somewhere that depends on the target/host platform? pkgs/stdenv/linux/default.nix is itself prefixed with

assert crossSystem == localSystem;

and thus cannot decide whether to set NIX_LIB*_IN_SELF_RPATH. No?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compilation case is a great point! I'll spend some time today to understand how nixpkgs plumbs rpath details there and get back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fair to say NIX_LIB*_IN_SELF_RPATH should only be set for multiStdenv? If so, the variables could be set conditionally there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fair to say NIX_LIB*_IN_SELF_RPATH should only be set for multiStdenv? If so, the variables could be set conditionally there.

Ideally (if non-multilib environment used lib everywhere consistently) - yes. In practice I don't think we are there yet. We probably want both lib and lib64 to still be pulled in. Normally I would expect NIX_LDFLAGS to apply to host toolchains (and NIX_LDFLAGS_FOR_BUILD / NIX_LDFLAGS_FOR_TARGET be disambiguating variables).

On the other hand looking at the history far back it was added before cross-compilation was a well-supported target: d9213df . It probably outlived it's purpose and we have many other ways to influence the driver.

Let's try your change as is and see if it works on multilib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try your change as is and see if it works on multilib.

Nice, thank you.

It probably outlived it's purpose and we have many other ways to influence the driver.

By "other means to influence the driver", do you mean the corresponding (and only) users of the NIX_LIB*_IN_SELF_RPATH,

nixpkgs/pkgs/stdenv/generic/setup.sh

Lines 314 to 319 in 2ba1f86

if [ -n "${NIX_LIB64_IN_SELF_RPATH:-}" ]; then
export NIX_LDFLAGS="-rpath $1/lib64 ${NIX_LDFLAGS-}"
fi
if [ -n "${NIX_LIB32_IN_SELF_RPATH:-}" ]; then
export NIX_LDFLAGS="-rpath $1/lib32 ${NIX_LDFLAGS-}"
fi

should be removed as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's remove those as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

'';


Expand Down