`env` has a weird format, should be an attrset

[lilyball]

Is your feature request related to a problem? Please describe.

The env options has a bit of an awkward format. I'm doing my configuration in Nix so I have to write it like

{
  env = [
    {
      name = "LIBCLANG_PATH";
      value = "${pkgs.llvmPackages_9.libclang.lib}/lib";
    }
    {
      name = "DEVSHELL_NO_MOTD";
      value = 1;
    }
    {
      name = "NIX_CONFIG";
      eval = ''"''${NIX_CONFIG:+$NIX_CONFIG$'\n'}builders = "${lib.escapeShellArg builderConfig}'';
    }
  ];
}

The TOML isn't much better, having to specify name/value repeatedly.

Describe the solution you'd like

I'd like to be able to write

{
  env = {
    LIBCLANG_PATH = "${pkgs.llvmPackages_9.libclang.lib}/lib";
    DEVSHELL_NO_MOTD = 1;
    NIX_CONFIG.eval = ''"''${NIX_CONFIG:+$NIX_CONFIG$'\n'}builders = "${lib.escapeShellArg builderConfig}'';
  };
}

I believe the TOML would also benefit from this shorthand, which I expect would look something like the following (although I'm a bit rusty at TOML):

[env]
DEVSHELL_NO_MOTD = 1
NIX_CONFIG.eval = '''"${NIX_CONFIG:+$NIX_CONFIG$'\n'}builders = "hard-coded builder config"'''

You should be able to define the type using coercedTo so the old version is coerced to this new version, thus allowing for merging modules that use both. The values themselves could also be done with coercedTo in order to convert the bare value form into { value = …; } (or just update the code to handle either).

Describe alternatives you've considered

A library function could be written that does the conversion, which would only be usable in Nix code, and is less discoverable and more awkward.

Allowing bare values (e.g. DEVSHELL_NO_MOTD = 1) could also be skipped in favor of writing this like

{
  env = {
    DEVSHELL_NO_MOTD.value = 1;
  };
}

although it's nice to make the most common expected form easier to use.

[lilyball]

The attrset approach also means we'll get module-level enforcement that each env var is only specified once. With this, it would be nice to support merging prefix definitions, because prefixing can be done multiple times. This is probably not very useful with the TOML approach but would be useful with complex Nix configurations.

I also wonder if the submodule definition, where it has 4 options of which exactly one must be set, could be instead replaced by a oneOf of 4 separate submodules, each of which only defines one of the exclusive options. I haven't ever tried that but it feels like it might work, and would remove the manual assertions, and might produce better error message when merging as well I'm not sure.

[zimbatm]

I agree that it's cumbersome and would be happy to find another way to arrange this.

The main reason it's using a list is that eval and prefix depend on the order of evaluation. With an attrset, the ordering is lexigographical, which might not be the right order.

Consider:

env = {
  MY_VAR = "1";
  HELLO.eval = "${MY_VAR}";
};

Because Nix orders the keys alphabetically, HELLO would appear first in an iteration.

One thing I considered for a while was to split the set/unset from the eval/prefix ones. Set/unset would be an attrset that gets applied first, and then the eval/prefix is still in a list. I think that would work too but wasn't overwhelmingly better.

[lilyball]

I didn't consider the idea that an eval might reference another env var set this way. Also I wasn't sure if an explicit goal here was to support something like

[[env]]
name = "FOO"
prefix = "bin"
[[env]]
name = "FOO"
prefix = "bin2"

I suppose the implicit realpath-ing of the value means you can't just say prefix = "bin:bin2" (although perhaps it should split on colons so you can say that?), so there is some value in this, although it's also not immediately obvious if the order here particularly matters.

I suggested using coercedTo so the old version would be coerced to the new one, I suggested doing it in this order so that way if I explore the resolved values (e.g. load up nix repl and look at devShells.${system}.default.config) I'd get the attrset version. However you could use coercedTo to convert the new version into the old and therefore allow using the current list style to enforce ordering.

That said, I do like the idea that it would apply set/unset first, then prefix, and then eval. I suppose it could apply this ordering only when processing the attrset version, otherwise you might be surprised at the behavior of

{
  env = [
    { name = "OLD_BAR"; eval = ''"$BAR"''; }
    { name = "BAR"; value = "some value"; }
  ];
}

though explaining the behavior would be simpler if it just applied always. Folks can use scripts if they really need to do fancy things like that (or use { name = "BAR"; eval = lib.escapeShellArg "some value"; }).

But of course applying it always is a backwards compatibility hazard, so we could also just say ordering isn't guaranteed when using the attrset version, use the list version if order matters or write a script to set the variables. But if you're okay with changing ordering then I would prefer to do set/unset, then prefix, then eval. This way set/unset won't ever overwrite prefix/eval, and then prefix next because it only depends on itself, and then eval because it can depend on anything.

[zimbatm]

If you're up for sending a PR, I suggest the following:

Change the env key to become an attrset of key=value, so the user can express things simply:

[env]
FOO = "bar"

Introduce envs which is the current list approach.

[[envs]]
name = "FOO"
value = "bar"

Have an adapter so that if a list is passed to env, it sets the envs and emits a depreciation warning.

Something like that?

[lilyball]

That sounds like a good idea. And perhaps it can be defined/documented such that env is explicitly processed before envs.

What I've done locally is defined a similar env' type that works similarly, although it takes either a value or a submodule with value/prefix/eval so I can write things like

{
  env' = {
    FOO = "bar";
    BAR.eval = "$BAZ";
    QUX = null; # this unsets it, as does { value = null; }
  };
}

The unset-on-null-value part was a bit complicated, I did that by removing the default on value and adding an internal readonly option with a config definition in the submodule that checks options.value.isDefined.

Unfortunately I can't actually submit a PR without running afoul of my employer's OSS contribution policies.

[zimbatm]

Unfortunately I can't actually submit a PR without running afoul of my employer's OSS contribution policies.

I'm sorry to ask but are you using this project at work?

[lilyball]

@zimbatm My employer has strict policies about employees contributing to OSS even for personal usage.

[zimbatm]

I don't know what to think. Reciprocity is important for the OSS ecosystem's health. We spend a lot of time building things for free, helping people out. It's human to do that.

If your company has very strict IP policies, another way to contribute back is to set up a support contract with us. This then helps us fund our lives and in consequence, allows us to do more projects like this ( sales@numtide.com ).

Thanks for contributing your ideas and code samples, I think for now this issue will stay like that until some new event will revive it.

[squirmy]

Not exactly trying to revive this issue, but would like to share a code snippet i'm using to set the env vars:

env = lib.attrsets.attrsToList {
  FOO = "one";
  BAR = "two";
};

From the NixOS 23.11 Release Notes

lib.attrsets: New function: attrsToList.

It won't handle those edge cases, but it's good enough most of the time.

[deemp]

Another idea

[
  { x = null; }
  { a = "b"; c = "d"; }
  { e.eval = "$b"; }
]