stdenv: assert that env is an attrset

[langsjo]

Resolves #425858

This commit makes derivations built with stdenv.mkDerivation have an assert that ensures env is a non-derivation attrset.

Also drops a test that expects the behavior this PR is disallowing to be allowed.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• Nixpkgs 25.11 Release Notes (or backporting 25.05 Nixpkgs Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
• NixOS 25.11 Release Notes (or backporting 25.05 NixOS Release notes)
  • (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, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

---

Add a 👍 reaction to pull requests you find important.

[emilazy]

How did a non-attribute set env behave prior to that PR?

cc @wolfgangwalther @ShamrockLee for structured attrs concerns

[langsjo]

How did a non-attribute set env behave prior to that PR?

It would pass evaluation and build without issue if __structuredAttrs was set to false, otherwise it would fail evaluation on an assert with message "When using structured attributes, env must be an attribute set of environment variables."

[philiptaron]

Wow, this is pretty screwy. Maybe it's a kneejerk reaction, I feel like this PR is going in the wrong direction: how can we enforce env being an attrset more broadly instead of only when __structuredAttrs is set?

Reading the linked issue, I saw that @eclairevoyant gave it a 👍🏻 -- maybe you or they could say more about the cases that led to this PR? Is there a derivation (whether in nixpkgs or not) that uses env as non-attrs?

[emilazy]

Right, but I mean what would it do? Create an environment variable called env? Since we're hoping to turn structured attributes on by default at some point and you can always env.env = ...;, maybe we should just mandate it be an attribute set always. But I don't want to block a fix of a simple regression so this is purely a drive-by comment.

[langsjo]

AFAICT, if env was something that can get coerced to a string, such as an integer, a string, a derivation, a list of things that can get coerced into strings, and probably more, it would set the environment variable env to whatever the result of that string coercion was. If it could not get coerced to a string (such as a function, list containing an attrset, and probably more), it would fail with an error such as this (full trace)

❯ nix-build -E "(import ./. {}).callPackage ./test.nix {}" --show-trace
error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'test-0.0.0'
         whose name attribute is located at /home/langsjo/git/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:468:13

       … while evaluating attribute 'env' of derivation 'test-0.0.0'
         at /home/langsjo/git/nixpkgs/test.nix:28:3:
           27|
           28|   env = lib.map;
             |   ^
           29|

       error: cannot coerce the built-in function 'map' to a string: «primop map»

[langsjo]

maybe you or they could say more about the cases that led to this PR? Is there a derivation (whether in nixpkgs or not) that uses env as non-attrs?

There was a bug in one of llama.cpp's nix file spotted by @amozeo, where they use lib.optionals to optionally set the value of env to an attrset. If the condition was true, it would work fine and pass the attrset, but if it was false, it would set it to an empty list. This very likely was unintentional, but it got me to look at stdenv and found that there was some regression.

I do think that making env throw when not an attrset makes sense, but didn't think that I had enough knowledge to make that decision, so opened this PR instead.

[philiptaron]

I do think that making env throw when not an attrset makes sense, but didn't think that I had enough knowledge to make that decision, so opened this PR instead.

I think I'm gutsy enough to merge that PR now.

There was a bug in one of llama.cpp's nix files

Yeah -- I think either I or @SomeoneSerge wrote that bug 😳 Thanks for the real-life example. I do have commit rights over there and I have let the derivation rot somewhat so please tag me on the PR to fix.

[philiptaron]

I want to merge a more decisive PR that doesn't allow env to be anything other than an attrset.

[langsjo]

@philiptaron there are some Ruby packages which have a pattern of env = bundlerEnv { ... }, which creates a derivation (which is why the nixpkgs-vet check failed). Don't know anything about Ruby, but seems that they just used env as the name of a variable, not to create env vars. Changing the name from env to something else fixed it and the derivations build. Should I add another commit making this change to those packages?

[roberth]

but I mean what would it do? Create an environment variable called env?

Pretty much. env was introduced with an overabundance of caution. In the limit, taking any name out of an open attrset and repurposing it is a breaking change. Unfortunately the mkDerivation argument is both a dict and a record as in https://nix.dev/manual/nix/2.28/development/json-guideline#extensibility. That should have never happened, but otoh environment variables have aspects of both too, so all software is screwed :D (EDIT: unix software, I mean. Hot take, I know)

[emilazy]

I agree with this approach.

[emilazy]

@philiptaron there are some Ruby packages which have a pattern of env = bundlerEnv { ... }, which creates a derivation (which is why the nixpkgs-vet check failed). Don't know anything about Ruby, but seems that they just used env as the name of a variable, not to create env vars. Changing the name from env to something else fixed it and the derivations build. Should I add another commit making this change to those packages?

Yeah, let’s fix those. It would have to be done when we turn on structured attributes by default anyway. Looks like rubyEnv, gemEnv, deps are all used.

This does mean that this is probably a breaking change for out‐of‐tree users, but the fix is trivial enough that I think it’s okay.

[emilazy]

@wolfgangwalther Any idea why https://github.com/NixOS/nixpkgs/actions/runs/16358006170/job/46220573630?pr=426211 is unhappy but the eval CI passed, by the way?

[ShamrockLee]

Thanks for pinning.

How did a non-attribute set env behave prior to that PR?

Aside from how env influences the result derivation arguments, we should also inspect how it behave in <pkg>.overrideAttrs.

Update:

Perhaps we need some more tests inside tests.overriding to guard those features.

[emilazy]

This error happened in pkgs/by-name/ev/evil-winrm/package.nix. This package is a by-name package, so nixpkgs-vet checks it. But this package doesn't have meta.platforms set, so hydra won't build it - thus we don't eval it either.

Hydra does build packages with no meta.platforms. If the CI eval mismatches that it's a bug.

[wolfgangwalther]

Hydra does build packages with no meta.platforms. If the CI eval mismatches that it's a bug.

OK, I will look into that. In any case, this is an existing issue, not caused by this PR at all. I have seen this for quite a while already. I remember reading somewhere that hydra needs meta.platforms and thus concluded this to be the case. But apparently that's not true and these packages (without meta platform) are lost somewhere else along the way.

[wolfgangwalther]

Hydra does build packages with no meta.platforms. If the CI eval mismatches that it's a bug.

OK, I will look into that. In any case, this is an existing issue, not caused by this PR at all. I have seen this for quite a while already. I remember reading somewhere that hydra needs meta.platforms and thus concluded this to be the case. But apparently that's not true and these packages (without meta platform) are lost somewhere else along the way.

Scratch that:

• Packages without meta.platform seem to work the same way as others regarding Eval, according to my tests right now.
• The same kind of assertion error is also not caught by eval, when it's on a package that has meta platform listed (I tested ronn, which was changed in this PR, too).

What happens once an assertion fails: The package is removed from the listed attrpaths already. That's the first step of eval, calling release-attrpaths-superset.nix.

I will have to test more, but I think assert might in general not be caught by Eval.

[wolfgangwalther]

I will have to test more, but I think assert might in general not be caught by Eval.

It's essentially this already known issue: #397184 (comment)

Edit: Also note my conclusion in #397184 (comment).

[roberth]

Looks good overall, although I'm banking on the other reviews a bit for the details.
Nice to get rid of the unnecessary environment variable, thanks!

[philiptaron]

Rebased on master due to CI failure that looked like a GitHub actions issue.