stdenv: run toString on lists in env to allow lists in env

[Artturin]

will make appending to lists in env in overrideAttrs more ergonomic due
to no longer needing to add spaces manually

nix-repl> keepassxc.NIX_CFLAGS_COMPILE
"-Wno-old-style-cast -Wno-error -D__BIG_ENDIAN__=0"

nix-repl> (keepassxc.overrideAttrs(oA: { passthru.oA = oA; })).oA.env.NIX_CFLAGS_COMPILE
[ "-Wno-old-style-cast" "-Wno-error" "-D__BIG_ENDIAN__=0" ]

nix-repl> (keepassxc.overrideAttrs(oA: { passthru.oA = oA; })).oA.NIX_CFLAGS_COMPILE
error:
       … while evaluating the attribute 'oA.NIX_CFLAGS_COMPILE'

         at «string»:1:32:

            1| (keepassxc.overrideAttrs(oA: { passthru.oA = oA; })).oA.NIX_CFLAGS_COMPILE
             |                                ^

       error: attribute 'NIX_CFLAGS_COMPILE' missing

       at «string»:1:1:

            1| (keepassxc.overrideAttrs(oA: { passthru.oA = oA; })).oA.NIX_CFLAGS_COMPILE
             | ^

if merged revert most changes in #217206

Description of changes

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:
  • 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/)
• 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.

[Artturin]

one issue i can think of is that users may use the env list values outside of overrideAttrs so they will be strings, but in that case there will be a error so it shouldn't be a big issue

[Artturin]

after digging in #72074 found #72074 (comment)

env could be preprocessed and toString applied to lists of strings -@FRidh

I have purposefully avoided that, I'd prefer keeping env.* vars consistently scalar types. If necessary people can themselves build lists and toString it before passing it to the env attrs. They then should be aware that there is no special handling of arg splitting etc in env. -@globin

[Artturin]

I do still think this would be nice to have for ergonomics

[samueldr]

(I was asked to add a comment here after a discussion earlier today.)

I don't exactly know if it is desirable or undesirable to handle lists in env.*. Here are my thoughts for end-users authoring using stdenv.mkDerivation.

I think it is cumbersome as a public API to externalize building lists of things as strings.

I do not think it is confusing for lists in env to end-up flattened using the same semantics as Nix derivations, as it is the expected behaviour for derivations.

My opinion is that we probably should follow those semantics here, as env.* is AFAIUI the direct replacement to adding misc. environment variables to the bare derivation attributes. This would be the least diruptive change to do for end-users; migrating from bare attrs to env.* should be limited to adding the existing constructs to the env attribute.

[roberth]

I don't think toString is the right behavior.
IIUC, env is treated as a normal string coercion, and not a toString invocation. This affects the list item interpretation of paths:

nix-repl> "${./lib}"
"/nix/store/68rmyx4vrvghbkkqlhvb9kv63n761nvc-lib"

nix-repl> "${toString [./lib]}"
"/home/user/nixpkgs/lib"

I expect the same inconsistency to occur in env (although tbh I didn't check). This is bad. Paths should always behave the same. Their default interpretation is as a vehicle for inserting sources into derivations, and I think we should stick to that. The eval-time location of the sources has nothing to do with the build.

Furthermore, in many, but not all cases, a separator would be expected to be inserted, usually :. I don't think we should make any assumptions about this either. Explicit concatStringsSep is good for reading and understanding, whereas implicit behavior is where we expose ourselves to silly bugs and overcomplicated compatibility behaviors. There's even a performance cost. mkDerivation is basically our most low level function when it comes to packaging, so we better be careful.

[samueldr]

Furthermore, in many, but not all cases, a separator would be expected to be inserted

I think it would be fine, and maybe desirable, to resort to the basic derivation bare attributes semantics, instead of making new ones here. My opinion is that moving things that should be in the environment and not bash variables should be limited to only moving the attribute from the bare derivation to the env attrset. Any different behaviour would be a cause for surprises.

(everything else prior I have no issues with.)

[roberth]

Moving something to env is a manual action. We may as well use this opportunity to "ask" the author to make their intent explicit. A surprise is ok if it leads to a better outcome overall.

Suggested change

	"The ‘env’ attribute set can only contain derivation, list, string, boolean or integer attributes. The ‘${n}’ attribute is of type ${builtins.typeOf v}."; (if lib.isList v then toString v else v))
	"The ‘env’ attribute set can only contain derivation, string, boolean or integer attributes. The ‘${n}’ attribute is of type ${builtins.typeOf v}.${optionalString (lib.isList v) " ‘env’ requires you to be explicit about how the list should be rendered. Use for example `lib.concatStringsSep \":\" (<your_value>)` and consider whether escaping is needed."}"; v)

I've probably butchered the code with that suggestion, but you catch my drift. I chose : because I think that's the most likely candidate in the general case. Moving something to env seems a bit odd to me, whereas adding a :-separated environment variable seems much more likely.

[wolfgangwalther]

I have purposefully avoided that, I'd prefer keeping env.* vars consistently scalar types. If necessary people can themselves build lists and toString it before passing it to the env attrs. They then should be aware that there is no special handling of arg splitting etc in env. @globin

Furthermore, in many, but not all cases, a separator would be expected to be inserted, usually :. I don't think we should make any assumptions about this either. Explicit concatStringsSep is good for reading and understanding, whereas implicit behavior is where we expose ourselves to silly bugs and overcomplicated compatibility behaviors. @roberth

I find those arguments compelling.

My opinion is that moving things that should be in the environment and not bash variables should be limited to only moving the attribute from the bare derivation to the env attrset. Any different behaviour would be a cause for surprises.

I intend to enable structuredAttrs by default eventually.. and with that I am facing a huge number of attributes that potentially need to be moved to env. I actually think it's a feature to have a warning here - most likely something that was a list before, wasn't supposed to be passed as an environment variable.

Looking at the example made in this PR and here:

if merged revert most changes in #217206

... I can't help myself thinking that all those NIX_ environment variables are just not supposed to be used like that. Imho, the "proper" solution, even if we wanted to use those environment variables, would be to have derivation arguments which are passed to some setup hook, which in turn sets those environment variables. This way, we could encode all kinds of knowledge in those setup hooks, e.g. how to concatenate the lists, which separator to use etc.

I conclude: We shouldn't do this, thus closing. Please re-open, if you disagree.