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

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

Merged
merged 1 commit into from
May 17, 2023

Conversation

eliasnaur
Copy link
Contributor

There doesn't seem to be a point in having separate /lib64 nor /lib32 directories in rpath in NixOS. Further, the setting of the environment variables NIX_LIB64|32_IN_SELF_RPATH depends on the build platform, not the target platform; see details in issues #221350.

Fixes #221350

This change removes a Chesterton's Fence, and so is probably incomplete as is. I'm posting it anyway to start the discussion: only packages that need NIX_LIB64|32_IN_SELF_RPATH should define them, and only for the required target platforms. If possible, support for NIX_LIB64|32_IN_SELF_RPATH should be removed altogether.

Tested with nix run .#hello. I can test with a more representative package set once the approach is validated.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@RaitoBezarius
Copy link
Member

Hey, thank you for the change, such changes triggers mass rebuilds, therefore you need to target staging, I will let you read carefully the CONTRIBUTING guide to understand how to do it without masspinging everyone, in the meantime, I will put this PR to draft.

@RaitoBezarius RaitoBezarius marked this pull request as draft March 30, 2023 11:48
@RaitoBezarius RaitoBezarius requested review from a user and trofi March 30, 2023 11:48
Comment on lines -118 to -119
${lib.optionalString (system == "x86_64-linux") "NIX_LIB64_IN_SELF_RPATH=1"}
${lib.optionalString (system == "mipsel-linux") "NIX_LIB32_IN_SELF_RPATH=1"}
Copy link
Contributor

@trofi trofi Mar 30, 2023

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,

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.

@eliasnaur eliasnaur changed the base branch from master to staging March 30, 2023 15:26
@eliasnaur eliasnaur force-pushed the no-nix-lib64-in-self-rpath branch from 451b831 to 02df378 Compare March 30, 2023 15:27
@eliasnaur
Copy link
Contributor Author

@RaitoBezarius retargeted staging according to the contribution guide, thank you. I hope I did it correctly.

@RaitoBezarius
Copy link
Member

@RaitoBezarius retargeted staging according to the contribution guide, thank you. I hope I did it correctly.

you nailed it :)

Comment on lines -118 to -119
${lib.optionalString (system == "x86_64-linux") "NIX_LIB64_IN_SELF_RPATH=1"}
${lib.optionalString (system == "mipsel-linux") "NIX_LIB32_IN_SELF_RPATH=1"}
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.

@trofi
Copy link
Contributor

trofi commented Mar 30, 2023

@ofborg build tests.cc-multilib-gcc

@eliasnaur eliasnaur marked this pull request as ready for review March 30, 2023 22:11
@eliasnaur eliasnaur force-pushed the no-nix-lib64-in-self-rpath branch from 02df378 to 3ecb6e1 Compare March 31, 2023 14:38
The NIX_LIB64|32_IN_SELF_RPATH environment variables control whether
to add lib64 and lib32 to rpaths. However, they're set depending
on the build paltform, not the target platform and thus their values
are incorrect for for cross-builds.

On the other hand, setting them according to the build platform introduce
pointless differences in build outputs; see NixOS#221350 for details.

This change fixes the issues by boldly removes the NIX_LIB*_IN_SELF_RPATH
facility altogether, in the hope that it is no longer necessary. They
were introduced in 2009, long before nixpkgs had good support for
cross-builds.

Fixes NixOS#221350
@eliasnaur eliasnaur force-pushed the no-nix-lib64-in-self-rpath branch from 3ecb6e1 to 88fd660 Compare March 31, 2023 16:54
@ghost
Copy link

ghost commented Apr 2, 2023

@ofborg build freshBootstrapTools.mips64el-linux-gnuabin32

@ghost
Copy link

ghost commented Apr 2, 2023

Hey, I'm generally in favor of the goal here. I need a bit more time to review this; I recall from the last time I dealt with this stuff that it was hairy.

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

To test the effect of this deletion requires MIPS hardware (i.e. a native build) which Hydra doesn't have:

${lib.optionalString (system == "mipsel-linux") "NIX_LIB32_IN_SELF_RPATH=1"}

I'm working on starting a build on one of my mips machines to confirm this.

@ghost
Copy link

ghost commented Apr 2, 2023

Alright, started on a mips64 host, which can native-compile mips32:

nix build -L -f . \
  --arg localSystem '(import ./lib).systems.examples.mipsel-linux-gnu' \
  --extra-extra-platforms mipsel-linux \
  --option sandbox false \
  hello 

Needed #224325

(I thought I fixed the mips32-seccomp-sandbox-on-mips64-hosts situation, but I guess not... needed --option sandbox false).

@ghost
Copy link

ghost commented Apr 2, 2023

Alright, started on a mips64 host, which can native-compile mips32:

It's on the final (stage4) build of binutils so it should be done in a few hours.

@trofi were you planning on merging this now or after the staging branchoff on 2023-May-15? If you don't mind waiting until the branchoff, I'll approve this when my build finishes. If there's some motivation for merging it before the branchoff we should probably run a full hydra jobset first... I don't see any reason why this would break things, but changing the -rpath flags of every package this close to ZHF makes me a bit nervous.

@trofi
Copy link
Contributor

trofi commented Apr 2, 2023

Waiting should be fine.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM if we can wait until staging branch-off on 2023-May-15.

Built on:

  • mipsel-linux

@eliasnaur
Copy link
Contributor Author

LGTM if we can wait until staging branch-off on 2023-May-15.

Thank you for the review!

As I understand it, cache.nixos.org won't have packages with this PR applied in at least 40 days.
I can understand you don't want a change like this for 23.05, but is there a way for me to get cached packages with this PR applied before that? Continuing my bit-for-bit reproducibility quest is painful if I have to rebuild everything from scratch locally.

@trofi
Copy link
Contributor

trofi commented Apr 3, 2023

Yeah, it's a bit unfortunate timing before a next major release preparation.

Past history shows that we need to make sure toolchains are not too broken before we pull in major desktop environment updates right before NixOS release (like gnome) into staging and staging-next without the fear that broken toolchains mask issues there.

Note that staging does not normally get cached either. Only staging-next gets rebuilt continuously. Merges from staging to staging-next happen once in 1-2 weeks. Thus it should not be much difference as it's not an "interactive" process (not even within a day).

I personally use config.contentAddressedByDefault = true; when I'm doing benign changes against stdenv and expect no/little binary changes even if derivations change a lot.

Otherwise large-scale changes with many incremental steps might warrant a separate jobset on hydra to allow multiple iterations to get something into shape. This approach is usually used for glibc, default gcc and similar large-scale updates with a high chance to break large percentage of nixpkgs.

@eliasnaur
Copy link
Contributor Author

I personally use config.contentAddressedByDefault = true; when I'm doing benign changes against stdenv and expect no/little binary changes even if derivations change a lot.

I considered enabling content-adressing, however it's my understanding it won't help because this PR does change the binaries (in particular, rpaths will shorten when built from x86_64-linux).

@trofi
Copy link
Contributor

trofi commented Apr 3, 2023

Yeah, in this case it does change stdenv script and forces full rebuild anyway. But does it change final binaries that much? I would normally expect patchelf to trim unused RUNPATHs out. Thus there is a slight change that future changes (not as drastic as this) might have a better chance of hitting cache.

@eliasnaur
Copy link
Contributor Author

Yeah, in this case it does change stdenv script and forces full rebuild anyway. But does it change final binaries that much? I would normally expect patchelf to trim unused RUNPATHs out.

patchelf may or may not trim unused paths, but it does not change the size of the RUNPATH section (fills it with XXXX's if I remember correctly). Certainly I wouldn't have noticed this problem if the binaries didn't end up with different RUNPATH.

I suppose what you're saying is to keep working on reproducibility, ignoring this problem. That may work. However, I did see another indeterministic variation in binaries output by gcc that I suspect are caused by object files being different.

@trofi
Copy link
Contributor

trofi commented Apr 3, 2023

Yup. Or use this change locally and keep getting world rebuilds.

@ghost
Copy link

ghost commented Apr 6, 2023

I personally use config.contentAddressedByDefault = true;

This is so cool; I really need to start doing it.

but it does not change the size of the RUNPATH section (fills it with XXXX's if I remember correctly).

@eliasnaur I see you noticed the problem with that...

@eliasnaur
Copy link
Contributor Author

I personally use config.contentAddressedByDefault = true;

This is so cool; I really need to start doing it.

but it does not change the size of the RUNPATH section (fills it with XXXX's if I remember correctly).

@eliasnaur I see you noticed the problem with that...

Indeed. I hope to avoid some/all use of patchelf/nuke-refs/etc. for bit-for-bit reproducibility by switching to content addressing. Waiting for NixOS/nix#6065.

eliasnaur added a commit to eliasnaur/nixpkgs that referenced this pull request May 5, 2023
Similar to PR NixOS#223861 and the issue NixOS#221350, NIX_COREFOUNDATION_RPATH
is set unconditionally in the darwin stdenv. This is wrong for cross-
compiles and results in `rpaths` depending on the builder platform.

This change makes another bold move and deletes the
NIX_COREFOUNDATION_RPATH facility completely, in the hope it is no
longer needed, or that any breakage is manageable.

As an added bonus we can delete a darwin specific hack in glibc.
@mweinelt mweinelt added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 6, 2023
@eliasnaur
Copy link
Contributor Author

Can this be merged now that the branch-off date has passed?

@RaitoBezarius
Copy link
Member

Now that the staging restrictions doesn't apply anymore, I believe this can proceed with standard reviewing as far as I know.

@RaitoBezarius
Copy link
Member

Branch off is in 5 days though.

@eliasnaur
Copy link
Contributor Author

Yeah, branch-off was an unfortunate label, sorry. I referred to the unrestricting of breaking changes to staging as listed in #223562.

@trofi
Copy link
Contributor

trofi commented May 17, 2023

Let's pull it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NIX_LIB64_IN_SELF_RPATH leaks through to cross-builds
4 participants