tests/plotinus, or NixOS environment variables can not be merged

[roberth]

Describe the bug

nix-build -A nixosTests.plotinus

The unique option `environment.variables.XDG_DATA_DIRS' is defined multiple times. Definition values:
- In `/home/user/h/nixpkgs/nixos/modules/programs/plotinus.nix':
    [
      "/nix/store/ak6k93ba1vngvf4yjw7rsa3an9s0w4if-plotinus-0.2.0/share/gsettings-schemas/plotinus-0.2.0"
    ]
- In `/home/user/h/nixpkgs/nixos/modules/config/shells-environment.nix': "/nix/store/s1635zhdyz4ysmwncrkdnp6n7n1in3ml-desktops/share"

environment.variables can not be merged when multiple definitions of type str are defined. It does work when all definitions are list, although merging may not be appropriate.

To Reproduce
Steps to reproduce the behavior:

1. nix-build -A nixosTests.plotinus

Expected behavior

A merge, as is appropriate for XDG_DATA_DIRS, using :

Additional context

Environment variables could be modeled RFC42-style. This allows accurate merging of variables where merging makes sense. It would disallow merging where it is not know to make sense. Merging variables that don't join by : would be possible.
This type could be exposed to all of NixOS (not just environment.variables, but environments in systemd as well, for example). They could even be used outside NixOS; for example in arion, nix-processmgt, etc.

Notify maintainers

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

Maintainer information:

# a list of nixpkgs attributes affected by the problem
attribute:
# a list of nixos modules affected by the problem
module:

[roberth]

I've tried changing a definition to a list, but I must have done something wrong and could not make it work.

Refs #122042

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-environment-variables-can-not-be-merged/13477/1

[jtojnar]

The issue is that environment.sessionVariables are stringified:

nixpkgs/nixos/modules/config/system-environment.nix

Line 36 in aa24e64

apply = mapAttrs (n: v: if isList v then concatStringsSep ":" v else v);

so when it gets passed to environment.variables, it will conflict

nixpkgs/nixos/modules/config/shells-environment.nix

Line 166 in aa24e64

environment.variables = config.environment.sessionVariables;

(The conflicting value comes from

nixpkgs/nixos/modules/services/x11/display-managers/default.nix

Lines 486 to 488 in 6d1934a

	environment.sessionVariables.XDG_DATA_DIRS = [
	"${cfg.displayManager.sessionData.desktops}/share"
	];

but it does not really matter.)

[ncfavier]

What if we did

environment.variables = mapAttrs (_: toList) config.environment.sessionVariables;

instead?

This seems to work at least for the use case of #126832.

[jtojnar]

Yeah, that should work.

Ideally, we would remove the apply and only stringify the variables when actually passing them to shell rc file but that would be a BC break 😿

[ncfavier]

Or do something like what's done for systemd's unitConfig and consorts, i.e. our own type with custom merge semantics:

nixpkgs/nixos/modules/system/boot/systemd-unit-options.nix

Lines 18 to 28 in 99d6ee5

	unitOption = mkOptionType {
	name = "systemd option";
	merge = loc: defs:
	let
	defs' = filterOverrides defs;
	defs'' = getValues defs';
	in
	if isList (head defs'')
	then concatLists defs''
	else mergeEqualOption loc defs';
	};

[jtojnar]

That sounds like a good solution. It does not increase the complexity too much and it can keep BC while resolving this issue.

[ncfavier]

By the way, I'm not sure what the filterOverrides call achieves in that snippet, since that's already done in lib/modules.nix (and it was already the case when that call was added)

[roberth]

The systemd-like approach requires that all definition sites agree that it's going to be mergeable, by defining a list. This is error prone.

Using an RFC42-style submodule we can move this decision to the option declaration where the intent is clear and definitions can not break the merging.
It has the added benefit of supporting space-separated environment variables like NIX_PROFILES and we don't have to implement any new merging functions (again, error prone)
Having documented environment variables is also pretty neat imo.

[ncfavier]

The systemd-like approach requires that all definition sites agree that it's going to be mergeable, by defining a list. This is error prone.

I was thinking of using map toList in the merge function to avoid this problem.

EDIT: I was really just using systemd as an example of a custom merge function, not saying we should use the same merging approach. Sorry if that wasn't clear.

Using an RFC42-style submodule we can move this decision to the option declaration where the intent is clear and definitions can not break the merging.

Isn't that orthogonal? Assuming we make a distinct e.g. environment.variables.GIO_EXTRA_MODULES option, the question remains: what type does it have? (But the idea sounds nice.)

[roberth]

Some variables can not be merged and defining them twice should be an error.

I was thinking of using map toList in the merge function to avoid this problem.

This way everything becomes mergeable, so we can't check this anymore.

Using an RFC42-style submodule we can move this decision to the option declaration where the intent is clear and definitions can not break the merging.

Isn't that orthogonal?

Considering the loss of checking, it's not.

Assuming we make a distinct e.g. environment.variables.GIO_EXTRA_MODULES option, the question remains: what type does it have?

List of :-separated strings is a good starting point. It will be just a matter of declaring it.

[ncfavier]

OK, thanks for the clarifications. Sounds good to me now.