Consider renaming `environment.variables` to `environment.shellVariables`

[kira-bruneau]

From what I understand, environment.variables is designed to hook into each shell's configuration:

• /etc/profile for bash
• /etc/zshenv for zsh
• /etc/fish/nixos-env-preinit.fish for fish
• ...

The x11 wrapper also sources these variables indirectly through bash's /etc/profile: https://github.com/NixOS/nixpkgs/blob/46468d801d2eaadbfabf7be04e59acdde61efb22/nixos/modules/services/x11/display-managers/default.nix#L61

Which only works when other shells are enabled because there's no option to disable bash:

https://github.com/NixOS/nixpkgs/blob/46468d801d2eaadbfabf7be04e59acdde61efb22/nixos/modules/programs/bash/bash.nix#L114

Although, I keep running into scenarios where environment.variables doesn't get applied, and I end up resorting to environment.sessionVariables instead:

• 109060

• 265886

It seems like environment.sessionVariables can always be used in place of environment.variables and is more reliable. Is there any reason to keep using environment.variables?

[kira-bruneau]

Another approach I also considered is to fold environment.sessionVariables into environment.variables so environment.variables always gets sourced by PAM and then deprecate environment.sessionVariables, but that seems a lot more likely to break people's existing configurations without notice.

It also seems like it would be harder to cleanup hacks that rely on sourcing bash's /etc/profile.

[bjornfor]

A data point from my nixos-config.git repo, which explicitly relies on environment.variables:

    # Not using environment.sessionVariables due to PAM not liking
    # values like BORG_REPO = "backup@server.example". When running
    # sudo with such a config this shows up in the journal:
    #
    # sudo[13522]: pam_env(sudo:session): Expandable variables must be wrapped in {} <@srv1.local:whitetip.borg> - ignoring
    environment.variables = mkIf (cfg.jobs ? "default") {
      BORG_REPO = "${cfg.jobs."default".repository}";
    };

I'm happy to migrate to environment.sessionVariables if shown how.

[Makrennel]

Not sure if this is kinda off-topic for this discussion, but I feel like the current behaviour of environment.sessionVariables is very strange. It forces you to treat it as if you were declaring environment variables for your shell, but if I'm understanding correctly it applies twice - once in /etc/pam/environment and another time in /etc/profile? At least that's the way it seems to me.

For example, if I wanted to, for some reason, change my HOME variable such as HOME=${HOME}/Local it would result in something like /home/alice/Local/Local rather than /home/alice/Local. If I were using a normal /etc/security/pam_env.conf file this wouldn't at all be a problem - I could do HOME DEFAULT=@{HOME}/Local and it wouldn't then also run when /etc/profile is parsed.

Would it not be better to properly separate environment.sessionVariables and environment.variables as they do not do the same thing? The "pam style variables" also serve a slightly different function to environment variables within the pam_env file - you can set HOME and use it within pam_env.conf as ${HOME}, but you can still use the @{HOME} variable within pam_env.conf to set other variables.

Sorry if this is a little off topic, I'm just not sure what purpose there was in merging environment.sessionVariables and environment.variables in the first place.

[kira-bruneau]

@Makrennel It's slightly off topic, but it's a good point! It looks like it was added for convenience: https://github.com/NixOS/nixpkgs/pull/30418? Now that you mention it though, it seems to cause more trouble than it's worth.

Getting back to the main reason I opened this though, I really think that the shell hook-in behaviour for environment.variables is a bad default. It looks like it's still needed for edge-cases like @bjornfor has run into, but the naming is misleading for something that should be treated more like an exception.

I think it would be way clearer if we just renamed environment.variables -> environment.shellVariables, and aliased environment.sessionVariables -> environment.variables to emphasize that PAM should be the preferred way to set environment variables.

[Makrennel]

    # sudo[13522]: pam_env(sudo:session): Expandable variables must be wrapped in {} <@srv1.local:whitetip.borg> - ignoring

I'm happy to migrate to environment.sessionVariables if shown how.

@bjornfor Now that I look at it, I think this partially goes towards what I'm saying with regard to the NixOS option obfuscating the pam_env syntax: pam_env has some of its own variables starting with @ and it will try to expand them if they are wrapped with curly braces. This shouldn't actually stop pam from declaring the variable as you specified because it isn't followed by curly braces, it'll just pollute your journal with warnings. I'm pretty sure if you want to stop the warnings you can just escape the @ with a backslash.

But I think this goes towards what I'm saying in that I think the syntax obfuscation along with running twice from both pam and /etc/profile is confusing and problematic. I would probably agree with @kira-bruneau that generally it is better to declare variables in environment.sessionVariables so that pam can manage them universally without hooking into the shell scripts, but I think it'd be best to also stop obfuscating the syntax as it is, and not duplicate the declaration into /etc/profile. That said, I'm in favour of moving the current environment.variables to environment.shellVariables – the distinction between sessionVariables and variables isn't very clear as it is right now.