addAttrsToDerivation seems broken

allsey87 commented 6 months ago

Describe the bug

I am trying to use addAttrsToDerivation to add some default compilation flags but I think this function is broken. The inline comment suggests the usage should be:

stdenvNoOptimise =
 addAttrsToDerivation
   { env.NIX_CFLAGS_COMPILE = "-O0"; }
   stdenv;

Although a quick search through Github, shows most people using it without the env before NIX_CFLAGS_COMPILE. In either case the following errors are reported:

With env:

error: attribute 'libc_bin' missing

Without env

error: The ‘env’ attribute set cannot contain any attributes passed to derivation. The following attributes are overlapping: NIX_CFLAGS_COMPILE

Steps To Reproduce

Use nix-build to try and compile GNU hello with an additional compilation flag via an overlay:

{
  system ? builtins.currentSystem,
  sources ? import ./nix/sources.nix,
}:

let
  myOverlay = self: super: {
    stdenv = super.addAttrsToDerivation { NIX_CFLAGS_COMPILE = "-O0"; } super.stdenv;
  };
  # 23.11
  pkgs = import sources.nixpkgs {
    overlays = [
      myOverlay
    ];
  };

in pkgs.hello

Expected behavior

GNU hello builds with the compilation flag -O0.

Additional context

I think the changes in PR https://github.com/NixOS/nixpkgs/pull/225787 around how env is handled broke both withCFlags and addAttrsToDerivation. The issue with withCFlags was raised in https://github.com/NixOS/nixpkgs/issues/225740 and fixed in https://github.com/NixOS/nixpkgs/pull/225929, however, addAttrsToDerivation seems to have been overlooked and seems to have the same problem. In fact, both that issue and this issue mention the error message:

attribute 'libc_bin' missing

Notify maintainers

@Artturin

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
this path will be fetched (0.00 MiB download, 0.00 MiB unpacked):
  /nix/store/mh3dkpww5py0fnrihk45pw7hvkj5fxxc-nix-info
copying path '/nix/store/mh3dkpww5py0fnrihk45pw7hvkj5fxxc-nix-info' from 'https://cache.nixos.org'...
 - system: `"x86_64-linux"`
 - host os: `Linux 6.8.7-zen1-1-zen, Debian GNU/Linux, 12 (bookworm), nobuild`
 - multi-user?: `no`
 - sandbox: `no`
 - version: `nix-env (Nix) 2.16.1`
 - nixpkgs: `/home/developer/.nix-defexpr/channels/nixpkgs`

allsey87 commented 6 months ago

Using an older version of nixpkgs (20.09), my MRE works fine without the env prefix. I also checked on the master branch and it seems that the issue is present both there and in 23.11.

allsey87 commented 6 months ago

Interesting enough, this does seem to work (on master and 23.11):

pkgs = import sources.nixpkgs {
  config.replaceStdenv = {pkgs}: pkgs.addAttrsToDerivation {
    env.NIX_CFLAGS_COMPILE = "-O0";
  } pkgs.stdenv;
};

apt-install-coffee commented 4 months ago

I'm also running into the same issue when either applying stdenvAdapters to overlays or crossOverlays:

crossOverlays = [
        (final: prev: { stdenv = prev.stdenvAdapters.useMoldLinker prev.stdenv; })
        (final: prev: { stdenv = (prev.stdenvAdapters.withCFlags [ "-Os" "-flto" "-ffat-lto-objects" ] prev.stdenv); })
 ];

using them with replaceCrossStdenv:

config.replaceCrossStdenv = { buildPackages, baseStdenv }:
        (buildPackages.useMoldLinker
          (buildPackages.withCFlags [ "-Os" "-flto" "-ffat-lto-objects" ]
            baseStdenv)
  );

or changing env directly:

config.replaceCrossStdenv = { buildPackages, baseStdenv }:
        buildPackages.addAttrsToDerivation
          {
            env.NIX_CFLAGS_LINK = "-fuse-ld=mold";
            env.NIX_CFLAGS_COMPILE = "-Os -flto -ffat-lto-objects";
          }
          baseStdenv;

I was able to replicate @allsey87 's success with

config.replaceStdenv = { pkgs }:
        pkgs.addAttrsToDerivation
          {
            env.NIX_CFLAGS_LINK = "-fuse-ld=mold";
            env.NIX_CFLAGS_COMPILE = "-Os -flto -ffat-lto-objects";
          }
          pkgs.stdenv;

so perhaps the issue is in pkgs/stdenv/cross, since it strips config.replaceStdenv, but not overlays? @allsey87 are you also cross compiling?

allsey87 commented 4 months ago

I was cross compiling for Emscripten but I have since given up on Nix and started using Bazel

ConnorBaker commented 2 months ago

Usage with env.NIX_CFLAGS_COMPILE is correct; the error is because it looks like the function doesn't provide any sort of recursive merging of the attribute set.

In your opening post, the error you saw when using it with env was caused by the env attribute set you passed (the singleton containing NIX_CFLAGS_COMPILE) replacing the existing env attribute set (which contained libc_bin).

Unfortunately, I haven't had time to look your findings, @apt-install-coffee :(

SomeoneSerge commented 4 weeks ago

@allsey87 if we expose extendMkDerivationArgs the solution would be something like

extendMkDerivationArgs (args: { env = args.env or { } // { NIX_COMPILE_CFLAGS = args.env.NIX_COMPILE_CFLAGS or "" + "..."; }; }) stdenv

NixOS / nixpkgs