Skip to content

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

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.

Sign up for GitHub

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

Closed
Artturin wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c18190f
stdenv: run toString on lists in env to allow lists in env
Artturin Feb 24, 2023
b413784
show that it works
Artturin Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkgs/applications/misc/keepassxc/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,17 @@ stdenv.mkDerivation rec {
sha256 = "sha256-amedKK9nplLVJTldeabN3/c+g/QesrdH+qx+rba2/4s=";
};

env.NIX_CFLAGS_COMPILE = lib.optionalString stdenv.cc.isClang [
__structuredAttrs = true;
env.NIX_CFLAGS_COMPILE = lib.optionals true [
"-Wno-old-style-cast"
"-Wno-error"
"-D__BIG_ENDIAN__=${if stdenv.isBigEndian then "1" else "0"}"
];

postConfigure = ''
typeset -p NIX_CFLAGS_COMPILE
'';

NIX_LDFLAGS = lib.optionalString stdenv.isDarwin "-rpath ${libargon2}/lib";

patches = [
Expand Down
4 changes: 2 additions & 2 deletions pkgs/stdenv/generic/make-derivation.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,8 @@ else let
assert lib.assertMsg (overlappingNames == [ ])
"The ‘env’ attribute set cannot contain any attributes passed to derivation. The following attributes are overlapping: ${lib.concatStringsSep ", " overlappingNames}";
lib.mapAttrs
(n: v: assert lib.assertMsg (lib.isString v || lib.isBool v || lib.isInt v || lib.isDerivation v)
"The ‘env’ attribute set can only contain derivation, string, boolean or integer attributes. The ‘${n}’ attribute is of type ${builtins.typeOf v}."; v)
(n: v: assert lib.assertMsg (lib.isString v || lib.isBool v || lib.isInt v || lib.isDerivation v || lib.isList v)
"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))
Copy link
Member

@roberth roberth Feb 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

@samueldr samueldr Mar 1, 2023

Choose a reason for hiding this comment

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

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.)

Copy link
Member

@roberth roberth Mar 1, 2023

Choose a reason for hiding this comment

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

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.

env;

in
Expand Down