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

fix replaceStdenv evaluation: avoid infinite recursion #144747

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from

Conversation

dguibert
Copy link
Member

@dguibert dguibert commented Nov 5, 2021

This avoids infinite recursion when using config.replaceStdenv

Motivation for this change
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 wip"
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 5, 2021
@dguibert
Copy link
Member Author

dguibert commented Nov 6, 2021

Unfortunately coreutils requires automake.
This patch needs to be modified.

@dguibert dguibert force-pushed the dg/fix-replacestdenv-coreutils branch from 6574886 to f044a50 Compare November 6, 2021 18:58
@dguibert dguibert changed the title fetchurl -> curl -> coreutils: autoreconfHook=null fetchurl -> curl -> coreutils: override autoreconfHook Nov 6, 2021
@dguibert
Copy link
Member Author

dguibert commented Nov 6, 2021

config.replaceStdenv now works with this patch.

@dguibert dguibert force-pushed the dg/fix-replacestdenv-coreutils branch from f044a50 to 7f362b9 Compare November 6, 2021 19:01
@dguibert dguibert changed the title fetchurl -> curl -> coreutils: override autoreconfHook fix replaceStdenv evaluation: avoid infinite recursion Nov 9, 2021
@roberth roberth requested review from Ericson2314 and removed request for roberth November 12, 2021 11:03
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-can-i-override-the-stdenv-to-be-gcc11stdenv-with-optimisations/16547/3

@dguibert dguibert force-pushed the dg/fix-replacestdenv-coreutils branch from 70bacf6 to 82e35f5 Compare April 29, 2022 07:34
@dguibert
Copy link
Member Author

dguibert commented Apr 29, 2022

This PR has been updated and rebased on top of master where perl now depends on zlib causing one circular dependency (introduced by #167084).
The last commit fixes this.

@Mindavi
Copy link
Contributor

Mindavi commented Jun 24, 2022

I cannot reproduce, can you post a (minimal) example?

@dguibert
Copy link
Member Author

I cannot reproduce, can you post a (minimal) example?

Here is an example called replaceStdenv.nix:

import ./. {
  config = {
    replaceStdenv = { pkgs }: let
      # Bootstrap a new stdenv that includes our nss_sss in glibc
      glibc = pkgs.glibc.overrideDerivation (old: {
        postInstall = old.postInstall + ''
          ln -s ${pkgs.nss_sss}/lib/*.so.* $out/lib
        '';
      });
      binutils = pkgs.binutils.override {
        libc = glibc;
      };
      gcc = pkgs.gcc.override {
        bintools = binutils;
        libc = glibc;
      };
    in
     builtins.trace "Enabling Custom Stdenv" pkgs.stdenv.override {
      cc = gcc;
      overrides = self: super: {
        inherit glibc binutils gcc;
      };
      allowedRequisites = pkgs.stdenv.allowedRequisites ++
        [ glibc.out glibc.dev glibc.bin binutils pkgs.nss_sss ];
    };
  };
  overlays = [
    (final: prev: {
      nss_sss = prev.callPackage (
        { stdenv
        , fetchurl
        , pkgconfig
        , glibc, pam, openldap, kerberos, dnsutils, cyrus_sasl, nss
        , popt, talloc, tdb, tevent, ldb, ding-libs, pcre, c-ares
        , glib, dbus
      }:

      let
        version = "1.16.5";
      in

      stdenv.mkDerivation rec {
        name = "sssd-nss-client-${version}";

        src = fetchurl {
          url = "https://fedorahosted.org/released/sssd/sssd-${version}.tar.gz";
          sha256 = "0ngr7cgimyjc6flqkm7psxagp1m4jlzpqkn28pliifbmdg6i5ckb";
        };

        # libnss_sss.so does not in fact use any of these -- they're just needed for configure
        nativeBuildInputs = [ pkgconfig
          pam openldap kerberos dnsutils cyrus_sasl nss
          popt talloc tdb tevent ldb ding-libs pcre c-ares
          glib dbus
        ];

        configureFlags = [
            # connect and use to system sssd:
            "--localstatedir=/var"
            "--sysconfdir=/etc"
            "--with-os=redhat"

            "--with-nscd=${glibc.bin}/sbin/nscd"
            "--with-ldb-lib-dir=$(out)/modules/ldb"
            "--disable-cifs-idmap-plugin"
            "--without-autofs"
            "--without-kcm"
            "--without-libnl"
            "--without-libwbclient"
            "--without-manpages"
            "--without-nfsv4-idmapd-plugin"
            "--without-python2-bindings"
            "--without-python3-bindings"
            "--without-samba"
            "--without-secrets"
            "--without-selinux"
            "--without-semanage"
            "--without-ssh"
            "--without-sudo"
          ];

          enableParallelBuilding = true;

          buildFlags = [ "libnss_sss.la" ];
          installTargets = [ "install-nsslibLTLIBRARIES" ];

        }      ) {};
    })
  ];
}

And try with
nix-build ./replaceStdenv.nix -A zlib
on top of either master or the PR

@Mindavi
Copy link
Contributor

Mindavi commented Jun 27, 2022

I'm not really sure about this, hopefully someone else can weigh in.

I tried to make a more minimal example like this:

import ./. {
  config = {
    replaceStdenv = { pkgs }: pkgs.clang11Stdenv;
  };
}

and building with nix-build ./replaceStdenv.nix -A zlib, but that just works fine (no infinite recursion). Same with gcc6Stdenv, clang6Stdenv.

I'm assuming that you can resolve the infinite recursion inside your own built stdenv rather than have to rely on the changes you made. But I might be totally off here.

@dguibert
Copy link
Member Author

I tried to make a more minimal example like this:
[...]
and building with nix-build ./replaceStdenv.nix -A zlib, but that just works fine (no infinite recursion). Same with gcc6Stdenv, clang6Stdenv.

All of those tests (even with ccacheStdenv) are too minimal and don't trigger the infinite recursion as they don't modify the glibc package.

I'm assuming that you can resolve the infinite recursion inside your own built stdenv rather than have to rely on the changes you made. But I might be totally off here.

If you could find a way to do it with my reproducer, I would be happy to study your solution 🤗

@Mindavi
Copy link
Contributor

Mindavi commented Jul 2, 2022

I haven't really had the time to make a better/smaller reproducer, but I seem to remember some eval issues when applying this, so I think we shouldn't merge this as-is.

@dguibert
Copy link
Member Author

dguibert commented Jul 2, 2022

I haven't really had the time to make a better/smaller reproducer, but I seem to remember some eval issues when applying this, so I think we shouldn't merge this as-is.

Which eval issues are you refering since all the checks have passed?

BTW what about this smaller reproducer:

{ useFix ? false }:

let
  nixpkgs = if useFix then
    (builtins.fetchGit {
      url = "https://github.com/dguibert/nixpkgs";
      ref = "refs/heads/dg/fix-replacestdenv-coreutils";
    })
    else
    (builtins.fetchGit {
      url = "https://github.com/NixOS/nixpkgs";
      ref = "refs/heads/staging";
    });
in
import nixpkgs {
  config = {
    replaceStdenv = { pkgs }: let
      # Bootstrap a new stdenv that includes a modified glibc
      glibc = pkgs.glibc.overrideDerivation (old: {
        postInstall = old.postInstall + ''
          touch $out/MODIFIED_GLIBC
        '';
      });
      binutils = pkgs.binutils.override {
        libc = glibc;
      };
      gcc = pkgs.gcc.override {
        bintools = binutils;
        libc = glibc;
      };
    in
     builtins.trace "Enabling Custom Stdenv" pkgs.stdenv.override {
      cc = gcc;
      overrides = self: super: {
        inherit glibc binutils gcc;
      };
      allowedRequisites = pkgs.stdenv.allowedRequisites ++
        [ glibc.out glibc.dev glibc.bin binutils ];
    };
  };
}

Trying to build zlib on staging branch with the new stdenv including a modified glibc:

$ nix-build ./replaceStdenv.nix -A zlib
trace: Enabling Custom Stdenv
error: infinite recursion encountered

       at //builtin/derivation.nix:19:19:
(use '--show-trace' to show detailed location information)

and with this PR:

$ nix-build ./replaceStdenv.nix -A zlib --arg useFix true
trace: Enabling Custom Stdenv
these 5 derivations will be built:
  /nix/store/fkdsf4v540z6x2yih9s3m0hdj3z9q7w6-glibc-2.34-115.drv
  /nix/store/66lf3rh9yfa749ly8d3r6y13cmxs1722-binutils-wrapper-2.38.drv
  /nix/store/ys3nch468bsmqc2k7hx17ji27ix9bxc1-gcc-wrapper-11.2.0.drv
  /nix/store/a0wlkrmzzckajx7zjm6wi215x0i64p9b-stdenv-linux.drv
  /nix/store/9lc9dy9lni7fgh0l58yazxkxyadcnp7f-zlib-1.2.12.drv
building '/nix/store/fkdsf4v540z6x2yih9s3m0hdj3z9q7w6-glibc-2.34-115.drv'...
unpacking sources
unpacking source archive /nix/store/wjbv1k6yigmb280wrvc1gkv8cnrsacij-glibc-2.34.tar.xz
^Cerror: interrupted by the user

This avoids infinite recursion when using config.replaceStdenv
Any dependency update on autoconf, automake, libtool or gettext breaks
the bootstrapping of fetchurl with replaceStdenv. This patchs removes
autoconfHook and avoid automake when building coreutils.
@dguibert dguibert force-pushed the dg/fix-replacestdenv-coreutils branch from 6a09a0d to e2adbbf Compare November 18, 2022 14:43
@dguibert
Copy link
Member Author

dguibert commented Nov 18, 2022

This has been updated for new staging changes

dguibert added a commit to dguibert/nur-packages that referenced this pull request Dec 14, 2022
@SuperSandro2000
Copy link
Member

@Artturin @Mindavi

@Mindavi
Copy link
Contributor

Mindavi commented Dec 24, 2022

TBH I think this is good, but I am unable to build anything right now. I guess the author did build some stuff, could you elaborate your testing @dguibert ?

@ofborg build curl perl zlib cmakeMinimal python

@dguibert
Copy link
Member Author

TBH I think this is good, but I am unable to build anything right now. I guess the author did build some stuff, could you elaborate your testing @dguibert ?

Hi, as a small testing overview you could read to #144747 (comment)
Otherwise I use this on any machines where I have to override glibc (for adding NSS e.g.)

@CertainLach
Copy link
Member

I also use this patch to build shared library for ubuntu with older glibc, and can confirm it works.
Here is the overlay: https://github.com/CertainLach/VivePro2-Linux-Driver/blob/master/nix/oldGlibc.nix

If nixpkgs is switched to original one here: https://github.com/CertainLach/VivePro2-Linux-Driver/blob/master/flake.nix#L4
Then nix build .#driver-proxy-release will fail with the inifinite recursion error

@Mindavi
Copy link
Contributor

Mindavi commented Jan 7, 2023

Thanks for confirming it works for you. I'm inclined to just try it and see how far we get at this point.

@trofi @tpwrules would you happen to have any comments on this?

@trofi
Copy link
Contributor

trofi commented Jan 7, 2023

Tl;DR: I think to get away without patching nixpkgs you just need to use unppatched fetchurl. Then you can avoid most of bootstrap problems:

--- replaceStdenv__.nix 2023-01-07 15:58:52.797881651 +0000
+++ replaceStdenv_.nix  2023-01-07 15:58:50.613852468 +0000
@@ -1,90 +1,91 @@
 import ./. {
   config = {
     replaceStdenv = { pkgs }: let
       # Bootstrap a new stdenv that includes our nss_sss in glibc
       glibc = pkgs.glibc.overrideDerivation (old: {
         postInstall = old.postInstall + ''
           ln -s ${pkgs.nss_sss}/lib/*.so.* $out/lib
         '';
       });
       binutils = pkgs.binutils.override {
         libc = glibc;
       };
       gcc = pkgs.gcc.override {
         bintools = binutils;
         libc = glibc;
       };
     in
      builtins.trace "Enabling Custom Stdenv" pkgs.stdenv.override {
       cc = gcc;
       overrides = self: super: {
         inherit glibc binutils gcc;
+        inherit (pkgs) fetchurl;
       };
       allowedRequisites = pkgs.stdenv.allowedRequisites ++
         [ glibc.out glibc.dev glibc.bin binutils pkgs.nss_sss ];
     };
   };

That allows me to build zlib against patched glibc.

More words on why overriding bootstrap packages is hard, especially glibc.

In bootstrap it takes us a few stdenvs to get new glibc:

  • stage2: build libidn2 and libunistring , and then try hard to mangle out all glibc references. Probably not needed to be dealt with.
  • stage3: build glibc
  • stage4: build gcc
  • final: build gcc

Changing glibc will indeed require us (as you did) to at least re-wrap gcc and binutils wrappers. But ideally we would want to rebuild gcc-unwrapped / binutils-unwrapped if we were to load plugins into gcc and binutils against patched glibc. That's a next level challenge for you :)

Looking at your change in:

              autoreconfHook = null;
              texinfo = null;
            }).overrideAttrs (_: {
              preBuild = "touch Makefile.in"; # avoid automake
            });

This one is especially worrying: preBuild = "touch Makefile.in";. That effectively skips the coreutils change at a distance (away from coreutils/default.nix). If such a hack is needed needed I would suggest to implement it as a derivation parameter and pass it here (like a texinfo = null;). But even better would be to avoid that.

You also have glibc<->nss_sss recursion which feels similar to libidn2<->glibc one with a caveat that you do have an nss_sss reference to glibc: "--with-nscd=${prev.glibc.bin}/sbin/nscd".

AFAIU currently "--with-nscd=${prev.glibc.bin}/sbin/nscd". refers to unpatched glibc. Is it ok? Would it do the right thing? I think you need an updated nscd. libidn2.bin does something similar.

Hope that helps a bit.

Comment on lines +852 to +856
autoreconfHook = null;
texinfo = null;
}).overrideAttrs (_: {
preBuild = "touch Makefile.in"; # avoid automake
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change should not be needed if your overrides don't trigger a rebuild of fetchurl. If you do need to rebuild fetchurlI would suggest handling in the rest of overrides (added a separate longer comment as an alternative not to rebuild fetchpatch). WDYT?

Copy link
Contributor

@johnrichardrinehart johnrichardrinehart Nov 30, 2024

Choose a reason for hiding this comment

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

As someone who just used this patch: I'd defer to the simpler user-facing API (i.e. not asking the user to predicate behavior on whether or not fetchurl needed to be rebuilt). If I understand correctly, you're (@trofi) recommending some support to prevent users from needing to rebuild some things in some particular cases. However, in the case that you're doing so much work that you're rebuilding fetchurl you're probably okay paying the price to rebuild fetchpatch et al.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't worry about the rebuilds. The problem happened here because the overlay introduced a dependency cycle which was not there initially. The change is a workaround for a specific case of newly introduced loop.

It's a workaround I don't feel good about: conditionally disabling autoreconf on coreutils is not great. It would be easier to accept the change if it was a change that strips (or adds) some dependencies unconditionally.

If others think this change is reasonable to break this particular loop type then so be it. I still think it's the overlay user's responsibility to untangle new dependency cycles they introduce.

And to state the obvious: i'm not an authority on stdenv (or anything in nixpkgs).

Copy link
Contributor

@johnrichardrinehart johnrichardrinehart Dec 1, 2024

Choose a reason for hiding this comment

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

Oh, I misunderstood. It seems like you're saying that this change to fetchurl is necessary because of the "solution". It's not that an unchanged fetchurl was the source of the original infinite recursion. Is that right? If so, then I agree that this doesn't seem like the best way to go.

Also, while I claimed that this "fixed my issue", I guess I can only say that it resolved the infinite recursion for me. After building a lot of stuff I actually still saw failure to build cpio (even though it solved the infinite recursion failure) - nix log is attached below. I'm hoping/thinking this was related to a libcxx version (I'm using libcxxStdenv) and am retrying on 24.11 (more on that, below).

6akz7axh5y7393qm0a8w4n5nwkb0wjvp-cpio-2.13.drv.log

I have a few follow-up questions, since you seem to have a pretty deep working understanding of bootstrap and how stdenv's are defined.

  1. I initially tried to use this work as-is (point nixpkgs to this fork) but ran into an issue building cpio (see above). So, then I tried to use this patch on 24.11 and ran into an infinite recursion. How did @dguibert track down those packages which were related to infinite recursions? Or, how would someone, generally?
$ nix build .\#defaultLibcxxStdenv --show-trace --override-input nixpkgs $HOME/code/repos/others/nixos/nixpkgs              
warning: Git tree '/home/john/code/repos/mine/f-u' is dirty
warning: Git tree '/home/john/code/repos/others/nixos/nixpkgs' is dirty
warning: not writing modified lock file of flake 'git+file:///home/john/code/repos/mine/f-u':
• Updated input 'nixpkgs':
    'github:dguibert/nixpkgs/e2adbbf71a6d6e49ecf2f66e48acdbab2dccdaa3' (2022-11-18)
  → 'git+file:///home/john/code/repos/others/nixos/nixpkgs' (2024-11-28)
trace: Enabling Custom Stdenv
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'f-u++'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'f-u++'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'nix-2.24.10'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'nix-2.24.10'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'brotli-1.1.0'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'nativeBuildInputs' of derivation 'brotli-1.1.0'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:380:7:

          379|       depsBuildBuild              = elemAt (elemAt dependencies 0) 0;
          380|       nativeBuildInputs           = elemAt (elemAt dependencies 0) 1;
             |       ^
          381|       depsBuildTarget             = elemAt (elemAt dependencies 0) 2;while evaluating derivation 'cmake-3.30.5'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'cmake-3.30.5'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'libarchive-3.7.7'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'libarchive-3.7.7'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'e2fsprogs-1.47.1'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'e2fsprogs-1.47.1'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'fuse-3.16.2'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'nativeBuildInputs' of derivation 'fuse-3.16.2'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:380:7:

          379|       depsBuildBuild              = elemAt (elemAt dependencies 0) 0;
          380|       nativeBuildInputs           = elemAt (elemAt dependencies 0) 1;
             |       ^
          381|       depsBuildTarget             = elemAt (elemAt dependencies 0) 2;while evaluating derivation 'meson-1.6.0'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'meson-1.6.0'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'openmp-18.1.8'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'buildInputs' of derivation 'openmp-18.1.8'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:383:7:

          382|       depsHostHost                = elemAt (elemAt dependencies 1) 0;
          383|       buildInputs                 = elemAt (elemAt dependencies 1) 1;
             |       ^
          384|       depsTargetTarget            = elemAt (elemAt dependencies 2) 0;while evaluating derivation 'llvm-18.1.8'
         whose name attribute is located at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'nativeBuildInputs' of derivation 'llvm-18.1.8'

         at /nix/store/93ixbis1xdzcq6jjbnkc2kv72qsgg5h8-source/pkgs/stdenv/generic/make-derivation.nix:380:7:

          379|       depsBuildBuild              = elemAt (elemAt dependencies 0) 0;
          380|       nativeBuildInputs           = elemAt (elemAt dependencies 0) 1;
             |       ^
          381|       depsBuildTarget             = elemAt (elemAt dependencies 0) 2;

       error: infinite recursion encountered

       at «none»:0: (source not available)
  1. In @dguibert's minimal reproduction he did more than just pull in this patch, he also redefines glibc, binutils, and gcc and then does some magic with them:
    replaceStdenv = { pkgs }: let
      # Bootstrap a new stdenv that includes a modified glibc
      glibc = pkgs.glibc.overrideDerivation (old: {
        postInstall = old.postInstall + ''
          touch $out/MODIFIED_GLIBC
        '';
      });
      binutils = pkgs.binutils.override {
        libc = glibc;
      };
      gcc = pkgs.gcc.override {
        bintools = binutils;
        libc = glibc;
      };
    in
     builtins.trace "Enabling Custom Stdenv" pkgs.stdenv.override {
      cc = gcc;
      overrides = self: super: {
        inherit glibc binutils gcc;
      };
      allowedRequisites = pkgs.stdenv.allowedRequisites ++
        [ glibc.out glibc.dev glibc.bin binutils ];
    };

I found this was necessary on top of 24.11 (at least, it alleviated the infinite recursion at eval-time - it remains to be seen if cpio is in any better state). Do you think something like this is generally necessary, or did I just "luck out" in that it fixed my eval issue?

  1. How would you recommend solving this problem, instead?

@dguibert
Copy link
Member Author

dguibert commented Jan 9, 2023

Tl;DR: I think to get away without patching nixpkgs you just need to use unppatched fetchurl.

Of course, but this used to work. Adding autoreconfHook to openssl and adding zlib to perl break the config.replaceStdenv feature, see Commit 3a78980 where autoreconfHook is fixed.

I've just wanted to restore this feature I used since many years. Then we could discuss how to enhance the overriding of glibc from early stages (see #50329 and #129595)

You also have glibc<->nss_sss recursion which feels similar to libidn2<->glibc one with a caveat that you do have an nss_sss reference to glibc: "--with-nscd=${prev.glibc.bin}/sbin/nscd".

Good point, thanks.

@trofi
Copy link
Contributor

trofi commented Jan 9, 2023

Tl;DR: I think to get away without patching nixpkgs you just need to use unppatched fetchurl.

Of course, but this used to work.

Just to clarify: what exactly you expect to work? I try to understand if it's one of:

  • your exact snippet with both glibc+nss_sss
  • or just any form of glibc override
  • or something else/extra you have

My worry is that change things like that about fetchurl it's not a maintainable way to do the override.

nss_sss pulls in a lot of depends that can change in future nixpkgs iterations. glibc is not as volatile, but still it did get python as a depend recently.

Adding autoreconfHook to openssl and adding zlib to perl break the config.replaceStdenv feature, see Commit 3a78980 where autoreconfHook is fixed.

Oh, that's quite an invasive change into fetchurl for something that is not enabled by default (I assume default bootstrap was fine on all targets). I'm not saying it's incorrect without looking at it much, but it's at least surprising we have to override packages like that when bootstrap Just Works. I would say that fetchurl should not accommodate replaceStdenv limitations and should only work for package build order defined by bootstrap packages (with small adjustments if it's natural to support them).

I would say that change should be reverted to simplify fetchurl. I understand it's a breakage.

To draw an analogy you hard to rewrap binutils and gcc for the same reason. They don't get rebuilt or reassembled for you by replaceStdenv or by all-packages.nix. That pierces a lot through nixpkgs internals. They are bound to break.

Or you could add an arbitrarily heavy package to perl and get even more complicated dependency loops to break.

I've just wanted to restore this feature I used since many years.

I'm not an expert but scarce documentation and implemntation of replaceStdenv implies it just replaces final set of packages in final stdenv after bootstrap and provides gcc wrapper as an example. That one is probably fine to override as gcc is overriden late in the stdenv rebuild queue. Note that it does not rebuild glibc for example and instead relies on existing one.

Then we could discuss how to enhance the overriding of glibc from early stages (see #50329 and #129595)

Can you help me understand why glibc override is a different matter here? New glibc's dependency does trigger circular dependencies in fetchurl here. Or I misunderstood all the issue here?

What are your thoughts? Should we still pursue complicating fetchurl? (or, maybe replaceStdenv itself to be more resilient against fetchurl change?)

@ConnorBaker
Copy link
Contributor

@dguibert where do we stand progress-wise? Any interest or bandwidth to continue pursuing this?

@dguibert
Copy link
Member Author

dguibert commented Aug 30, 2023

@dguibert where do we stand progress-wise? Any interest or bandwidth to continue pursuing this?

My main concern was that it used to work (for quite some years) and a patch broke that. I also understand that my override might not have use the right fetchurl.
Thanks to @trofi, I've applied his suggestions.
Unfortunately I didn't spend enough time nor have enough bandwidth to provide a cleaner solution.

@Artturin Artturin requested a review from trofi November 23, 2023 12:49
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 22, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@Titaniumtown
Copy link
Contributor

I'm able to reproduce the issue this PR aims to solve:

      pkgs = import nixpkgs {
        config.replaceStdenv = { pkgs }: pkgs.clangStdenv;
      };

This PR seems dead though :(

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 17, 2024
@trofi
Copy link
Contributor

trofi commented Sep 17, 2024

I think adding clang into bootstrap sequence is a lot more than fixing a circular dependency on glibc. My guess would be that this change is not enough for this use case.

@dguibert
Copy link
Member Author

Hi,

I'm able to reproduce the issue this PR aims to solve:

      pkgs = import nixpkgs {
        config.replaceStdenv = { pkgs }: pkgs.clangStdenv;
      };

I'm not able to reproduce with a file nixpkgs-144747-bis.nix (adapted from the earlier reproducer) with content:

{ useFix ? false }:

let
  nixpkgs = if useFix then
    (builtins.fetchGit {
      url = "https://github.com/dguibert/nixpkgs";
      ref = "refs/heads/dg/fix-replacestdenv-coreutils";
    })
    else
    (builtins.fetchGit {
      url = "https://github.com/NixOS/nixpkgs";
      ref = "refs/heads/staging";
    });
    pkgs = import nixpkgs {
      config.replaceStdenv = { pkgs }: pkgs.clangStdenv;
    };
in pkgs // {
  zlib_ = pkgs.zlib.override { stdenv = pkgs.clangStdenv; };
  }
nix build -L -f nixpkgs-144747-bis.nix zlib
[...]
build flags: -j2 SHELL=/nix/store/razasrvdg7ckplfmvdxv4ia3wbayr94s-bootstrap-tools/bin/bash PREFIX= SHARED_MODE=1
gcc -O3 -D_LARGEFILE64_SOURCE=1 -DHAVE_HIDDEN -I. -c -o example.o test/example.c
gcc -O3 -D_LARGEFILE64_SOURCE=1 -DHAVE_HIDDEN  -c -o adler32.o adler32.c
[...]

It builds with gcc where it should use clangStdenv and compile with clang.
Whereas zlib_ builds witg clang as expected...

@johnrichardrinehart
Copy link
Contributor

@dguibert I just used this PR branch to unblock me on a project. Big thanks for drafting this work! It'd be awesome to come to some consensus, here, in order to merge.

johnrichardrinehart added a commit to johnrichardrinehart/f-u that referenced this pull request Dec 1, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants