zsh: set environment variables in zshenv instead of zprofile #217205

jian-lin · 2023-02-19T19:24:55Z

This patch fixes two issues:

The file in which environment variables are set is inconsistent.

This file sets them in zprofile since 20111. This is the case when programs.zsh.enable is not set.
Zsh module sets them in zshenv since the very beginning in 20122. This is the case when programs.zsh.enable is set.

Setting environment variables in zprofile overrides what users set in .zshenv. See these3 home-manager4 issues5.

Description of changes

Things done

jian-lin · 2023-02-19T22:26:04Z

An explanation for home-manager issues: nix-community/home-manager#3681 (comment).

nixos-discourse · 2023-02-20T00:40:00Z

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

https://discourse.nixos.org/t/home-manager-doesnt-seem-to-recognize-sessionvariables/8488/25

jian-lin · 2023-02-22T09:21:15Z

In short, when system-level programs.zsh enable is not set, this patch makes environment variables set in users' .zshenv not be overridden by the system-level ones.

nixos-discourse · 2023-02-24T12:02:27Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/1868

samuela

LGTM although I don't have any familiarity with this corner of nixpkgs

jian-lin · 2023-02-28T10:34:27Z

Can we merge this? Or is there anything else I need to do?

NickCao · 2023-02-28T10:36:05Z

I know nothing about zsh and is not confident enough about this. Could any real zsh user provide some opinions?

samuela · 2023-02-28T15:33:35Z

cc maintainers @pSub and @Artturin

pkgs/shells/zsh/default.nix

jian-lin · 2023-03-05T17:27:14Z

A release note is added as requested by @Mic92. IMHO, it's ready to be merged.

chvp

I added this to my nixos-config and can confirm that it fixed the issue I reported originally in nix-community/home-manager@2116fe6#r67182173

jian-lin · 2023-03-11T09:33:22Z

pkgs/shells/zsh/default.nix

-  if test -r /etc/zprofile; then
-    . /etc/zprofile
+  if test -r /etc/zshenv; then
+    . /etc/zshenv
  else
    emulate bash
    alias shopt=false
    . /etc/profile


My motivation of this pr is to solve the issue that env vars set in .zshenv are overridden by those in /etc/set-environment ($out/etc/zprofile sources /etc/profile which sources /etc/set-environment) on NixOS system with programs.zsh.enable = false. This issue comes from users of home-manager.

I can think of three proposals to deal with this issue:

Proposal 0: souce /etc/profile in $out/etc/zprofile, i.e., the old behaviour

Pros

config.environment.loginShellInit is sourced in login zsh

Cons

env vars set in .zshenv are overridden by those in /etc/set-environment

config.environment.shellInit is not sourced in non-login zsh

config.programs.bash.shellInit is also sourced in login zsh

config.programs.bash.loginShellInit is also sourced in login zsh

Proposal 1: souce /etc/profile in $out/etc/zshenv, i.e., what this patch currently dose

Pros

env vars set in .zshenv are not overridden by those in /etc/set-environment

config.environment.shellInit is sourced in all zsh

Cons

config.environment.loginShellInit is also sourced in non-login zsh

config.programs.bash.shellInit is also sourced in all zsh

config.programs.bash.loginShellInit is also sourced in all zsh

Proposal 2: souce /etc/set-environment in $out/etc/zshenv

Pros

env vars set in .zshenv are not overridden by those in /etc/set-environment

config.programs.bash.shellInit is not sourced in any zsh

config.programs.bash.loginShellInit is not sourced in any zsh

Cons

config.environment.shellInit is not sourced in any zsh

config.environment.loginShellInit is not sourced in any zsh

I prefer proposal 2 because it fixes the issue without any wrong behaviour, i.e., sourcing config of bash. If you want those shell configs in config.environment, just set programs.zsh.enable.

WDYT?

I also think proposal 2 sounds more reasonable.

I have changed to proposal 2.

pkgs/shells/zsh/default.nix

This patch fixes two issues: 1. The file in which environment variables are set is inconsistent. - This file sets them in zprofile when programs.zsh.enable is not set. - Zsh module sets them in zshenv when programs.zsh.enable is set. 2. Setting environment variables in zprofile overrides what users set in .zshenv. See these[1] home-manager[2] issues[3]. /etc/profile is also changed to /etc/set-environment. Here is a comparison: Using /etc/profile: - Pros - config.environment.shellInit is sourced in all zsh - Cons - config.environment.loginShellInit is also sourced in non-login zsh - config.programs.bash.shellInit is also sourced in all zsh - config.programs.bash.loginShellInit is also sourced in all zsh Using /etc/set-environment: - Pros - config.programs.bash.shellInit is not sourced in any zsh - config.programs.bash.loginShellInit is not sourced in any zsh - Cons - config.environment.shellInit is not sourced in any zsh - config.environment.loginShellInit is not sourced in any zsh [1]: nix-community/home-manager#2751 (comment) [2]: nix-community/home-manager#2991 [3]: nix-community/home-manager#3681 (comment)

chvp

Works again.

jian-lin · 2023-03-18T09:30:20Z

A friendly ping @Mic92

RaitoBezarius · 2023-03-23T15:57:42Z

This will probably break the existing zsh configurations of some people, right?

jian-lin · 2023-03-23T16:03:21Z

This will probably break the existing zsh configurations of some people, right?

For users who set their shell to zsh and do not enable the system-level zsh module, yes, there are changed behaviours listed in the proposal 2. I have added a release note for that. That should be enough, right?

K900 · 2023-03-23T16:05:44Z

We should really remove the generic shellInit options, especially with more and more people wanting to run stuff like xonsh/nu/whatever other not-even-remotely-posix shell.

expipiplus1 · 2023-03-29T03:52:13Z

FWIW this totally broke zsh for me, every new shell was sourcing /etc/set-environment unconditionally, so resetting PATH (and removing anything inserted by adding nix-shell etc...).

Adding programs.zsh.enable = true in my system config didn't fix things.

(I haven't investigated this deeply at all, but the if [ -z "" ]; looks very suspect to me...)

❯ cat /nix/store/z2cnzz34sqrk8zwqq2g7sq0pfj8kk6j8-zsh-5.9/etc/zshenv_zwc_is_used
if test -e /etc/NIXOS; then
  if test -r /etc/zshenv; then
    . /etc/zshenv
  else
    emulate bash
    alias shopt=false
    if [ -z "" ]; then
      . /etc/set-environment
    fi
    unalias shopt
    emulate zsh
  fi
  if test -r /etc/zshenv.local; then
    . /etc/zshenv.local
  fi
else
  # on non-nixos we just source the global /etc/zshenv as if we did
  # not use the configure flag
  if test -r /etc/zshenv; then
    . /etc/zshenv
  fi
fi

jian-lin · 2023-03-29T05:53:32Z

@expipiplus1 Thanks! Fixed in #223696.

expipiplus1 · 2023-03-30T09:44:24Z

Thanks!

nixos-discourse · 2023-10-08T00:55:52Z

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

https://discourse.nixos.org/t/home-manager-not-setting-sessionvariables-or-shellaliases/33947/2

jian-lin requested a review from Artturin February 19, 2023 19:28

ofborg bot requested a review from pSub February 19, 2023 19:56

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Feb 19, 2023

jian-lin mentioned this pull request Feb 19, 2023

fish: warn of shell init nix-community/home-manager#3610

Closed

7 tasks

ncfavier requested review from eigengrau and nrdxp February 19, 2023 22:58

jian-lin mentioned this pull request Feb 21, 2023

bug: environment variables are unavailable in ZSH since #2708 was merged nix-community/home-manager#2751

Open

1 task

jian-lin requested a review from NickCao February 25, 2023 09:46

NickCao requested a review from samuela February 26, 2023 01:27

samuela approved these changes Feb 28, 2023

View reviewed changes

jian-lin requested review from Mic92 and alyssais February 28, 2023 10:48

Mic92 reviewed Mar 1, 2023

View reviewed changes

pkgs/shells/zsh/default.nix Show resolved Hide resolved

Mic92 reviewed Mar 4, 2023

View reviewed changes

pkgs/shells/zsh/default.nix Show resolved Hide resolved

jian-lin force-pushed the fix-zsh-set-env branch from 9b5ff78 to 878c20d Compare March 4, 2023 11:48

github-actions bot added 6.topic: nixos 8.has: changelog 8.has: documentation labels Mar 4, 2023

jian-lin requested a review from Mic92 March 4, 2023 11:51

jian-lin requested a review from chvp March 7, 2023 08:26

chvp approved these changes Mar 7, 2023

View reviewed changes

jian-lin requested a review from SuperSandro2000 March 8, 2023 15:47

jian-lin commented Mar 11, 2023

View reviewed changes

jian-lin force-pushed the fix-zsh-set-env branch from 878c20d to e23762c Compare March 11, 2023 10:58

chvp suggested changes Mar 13, 2023

View reviewed changes

pkgs/shells/zsh/default.nix Outdated Show resolved Hide resolved

jian-lin force-pushed the fix-zsh-set-env branch from e23762c to 8e2e741 Compare March 13, 2023 09:25

chvp approved these changes Mar 13, 2023

View reviewed changes

K900 merged commit 382c756 into NixOS:master Mar 23, 2023

jian-lin deleted the fix-zsh-set-env branch March 23, 2023 16:11

jian-lin mentioned this pull request Dec 24, 2023

Assert that fish configuration is enabled if any user has fish as their shell #211603

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zsh: set environment variables in zshenv instead of zprofile #217205

zsh: set environment variables in zshenv instead of zprofile #217205

jian-lin commented Feb 19, 2023

jian-lin commented Feb 19, 2023

nixos-discourse commented Feb 20, 2023

jian-lin commented Feb 22, 2023 •

edited

nixos-discourse commented Feb 24, 2023

samuela left a comment

jian-lin commented Feb 28, 2023

NickCao commented Feb 28, 2023

samuela commented Feb 28, 2023

jian-lin commented Mar 5, 2023

chvp left a comment

jian-lin Mar 11, 2023

Mic92 Mar 11, 2023

jian-lin Mar 11, 2023

chvp left a comment

jian-lin commented Mar 18, 2023

RaitoBezarius commented Mar 23, 2023

jian-lin commented Mar 23, 2023 •

edited

K900 commented Mar 23, 2023 •

edited

expipiplus1 commented Mar 29, 2023

jian-lin commented Mar 29, 2023

expipiplus1 commented Mar 30, 2023

nixos-discourse commented Oct 8, 2023

zsh: set environment variables in zshenv instead of zprofile #217205

zsh: set environment variables in zshenv instead of zprofile #217205

Conversation

jian-lin commented Feb 19, 2023

Description of changes

Things done

jian-lin commented Feb 19, 2023

nixos-discourse commented Feb 20, 2023

jian-lin commented Feb 22, 2023 • edited

nixos-discourse commented Feb 24, 2023

samuela left a comment

Choose a reason for hiding this comment

jian-lin commented Feb 28, 2023

NickCao commented Feb 28, 2023

samuela commented Feb 28, 2023

jian-lin commented Mar 5, 2023

chvp left a comment

Choose a reason for hiding this comment

jian-lin Mar 11, 2023

Choose a reason for hiding this comment

Mic92 Mar 11, 2023

Choose a reason for hiding this comment

jian-lin Mar 11, 2023

Choose a reason for hiding this comment

chvp left a comment

Choose a reason for hiding this comment

jian-lin commented Mar 18, 2023

RaitoBezarius commented Mar 23, 2023

jian-lin commented Mar 23, 2023 • edited

K900 commented Mar 23, 2023 • edited

expipiplus1 commented Mar 29, 2023

jian-lin commented Mar 29, 2023

expipiplus1 commented Mar 30, 2023

nixos-discourse commented Oct 8, 2023

jian-lin commented Feb 22, 2023 •

edited

jian-lin commented Mar 23, 2023 •

edited

K900 commented Mar 23, 2023 •

edited