stdenv: don't set NIX_LIB*_IN_SELF_RPATH by default by eliasnaur · Pull Request #223861 · NixOS/nixpkgs

eliasnaur · 2023-03-30T05:27:59Z

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

RaitoBezarius · 2023-03-30T11:48:01Z

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.

trofi · 2023-03-30T12:00:18Z

pkgs/stdenv/linux/default.nix

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.

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?

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

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.

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.

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?

Yeah, let's remove those as well.

eliasnaur · 2023-03-30T15:28:37Z

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

RaitoBezarius · 2023-03-30T15:53:52Z

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

you nailed it :)

trofi · 2023-03-30T21:58:06Z

pkgs/stdenv/linux/default.nix

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 · 2023-03-30T22:02:55Z

@ofborg build tests.cc-multilib-gcc

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

ghost · 2023-04-02T05:37:59Z

@ofborg build freshBootstrapTools.mips64el-linux-gnuabin32

ghost · 2023-04-02T05:57:09Z

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:

nixpkgs/pkgs/stdenv/linux/default.nix

Line 119 in 77c642a

${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 · 2023-04-02T06:34:53Z

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 · 2023-04-02T20:42:24Z

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 · 2023-04-02T21:26:21Z

Waiting should be fine.

ghost

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

Built on:

mipsel-linux

eliasnaur · 2023-04-02T23:23:13Z

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 · 2023-04-03T16:53:57Z

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 · 2023-04-03T17:25:17Z

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 · 2023-04-03T17:45:53Z

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 · 2023-04-03T18:06:34Z

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 · 2023-04-03T21:26:07Z

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

ghost · 2023-04-06T06:01:34Z

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 · 2023-04-14T20:55:31Z

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.

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.

eliasnaur · 2023-05-16T22:52:17Z

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

RaitoBezarius · 2023-05-16T23:01:20Z

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

RaitoBezarius · 2023-05-16T23:01:44Z

Branch off is in 5 days though.

eliasnaur · 2023-05-16T23:24:34Z

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

trofi · 2023-05-17T06:08:50Z

Let's pull it in.

github-actions bot added the 6.topic: stdenv Standard environment label Mar 30, 2023

RaitoBezarius marked this pull request as draft March 30, 2023 11:48

RaitoBezarius requested review from a user and trofi March 30, 2023 11:48

trofi reviewed Mar 30, 2023

View reviewed changes

eliasnaur changed the base branch from master to staging March 30, 2023 15:26

eliasnaur force-pushed the no-nix-lib64-in-self-rpath branch from 451b831 to 02df378 Compare March 30, 2023 15:27

trofi approved these changes Mar 30, 2023

View reviewed changes

eliasnaur marked this pull request as ready for review March 30, 2023 22:11

eliasnaur force-pushed the no-nix-lib64-in-self-rpath branch from 02df378 to 3ecb6e1 Compare March 31, 2023 14:38

eliasnaur requested review from Ericson2314 and matthewbauer as code owners March 31, 2023 14:38

trofi approved these changes Mar 31, 2023

View reviewed changes

eliasnaur force-pushed the no-nix-lib64-in-self-rpath branch from 3ecb6e1 to 88fd660 Compare March 31, 2023 16:54

ghost mentioned this pull request Apr 2, 2023

NIX_LIB64_IN_SELF_RPATH leaks through to cross-builds #221350

Closed

ghost approved these changes Apr 2, 2023

View reviewed changes

eliasnaur mentioned this pull request Apr 13, 2023

stdenv: delete the NIX_COREFOUNDATION_RPATH sledgehammer #226048

Closed

12 tasks

eliasnaur mentioned this pull request May 2, 2023

gcc: only disable aligned_alloc for darwin build/host/target platforms #226290

Merged

12 tasks

mweinelt added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 6, 2023

trofi merged commit c3bdae6 into NixOS:staging May 17, 2023

mgurlitz mentioned this pull request Jun 28, 2023

Avidemux startup error, missing library (libADM_coreVideoCodec6.so) #239424

Closed

	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

Uh oh!

Conversation

eliasnaur commented Mar 30, 2023

Things done

Uh oh!

RaitoBezarius commented Mar 30, 2023

Uh oh!

trofi Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliasnaur commented Mar 30, 2023

Uh oh!

RaitoBezarius commented Mar 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trofi commented Mar 30, 2023

Uh oh!

ghost commented Apr 2, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 2, 2023

Uh oh!

ghost commented Apr 2, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 2, 2023

Uh oh!

trofi commented Apr 2, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

eliasnaur commented Apr 2, 2023

Uh oh!

trofi commented Apr 3, 2023

Uh oh!

eliasnaur commented Apr 3, 2023

Uh oh!

trofi commented Apr 3, 2023

Uh oh!

eliasnaur commented Apr 3, 2023

Uh oh!

trofi commented Apr 3, 2023

Uh oh!

ghost commented Apr 6, 2023

Uh oh!

eliasnaur commented Apr 14, 2023

Uh oh!

eliasnaur commented May 16, 2023

Uh oh!

RaitoBezarius commented May 16, 2023

Uh oh!

RaitoBezarius commented May 16, 2023

Uh oh!

eliasnaur commented May 16, 2023

Uh oh!

trofi commented May 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

trofi Mar 30, 2023 •

edited

Loading

ghost commented Apr 2, 2023 •

edited by ghost

Loading

ghost commented Apr 2, 2023 •

edited by ghost

Loading