Skip to content
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.

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

zsh: move sessionVariables from .zshrc to .zshenv #2708

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

jian-lin
Copy link
Member

@jian-lin jian-lin commented Feb 8, 2022

Description

This patch moves both home.sessionVariables and
programs.zsh.sessionVariables from .zshrc to .zshenv. Additionally,
these two kinds of session variables will not be sourced more than
once to allow user-customized ones to take effect.

Before, session variables are in .zshrc, which causes non-interactive
shells to not be able to get those variables. For example, running a
command through SSH is in a non-interactive and non-login shell, which
suffers from this. With this patch, all kinds of shells can get
session variables.

The reason why these session variables are not moved to .zprofile is
that programs started by systemd user instances are not able to get
variables defined in that file. For example, GNOME
Terminal (gnome-terminal-server.service) is one of these programs and
doesn't get variables defined in .zprofile. As a result, the shells it
starts, which are interactive and non-login, do not get those
variables.

Fixes #2445

Related NixOS/nixpkgs#33219
Related NixOS/nixpkgs#45784

This file is not formatted before and is excluded by ./format, so I don't format it.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

This patch moves both home.sessionVariables and
programs.zsh.sessionVariables from .zshrc to .zshenv. Additionally,
these two kinds of session variables will not be sourced more than
once to allow user-customized ones to take effect.

Before, session variables are in .zshrc, which causes non-interactive
shells to not be able to get those variables. For example, running a
command through SSH is in a non-interactive and non-login shell, which
suffers from this. With this patch, all kinds of shells can get
session variables.

The reason why these session variables are not moved to .zprofile is
that programs started by systemd user instances are not able to get
variables defined in that file. For example, GNOME
Terminal (gnome-terminal-server.service) is one of these programs and
doesn't get variables defined in .zprofile. As a result, the shells it
starts, which are interactive and non-login, do not get those
variables.

Fixes nix-community#2445

Related NixOS/nixpkgs#33219
Related NixOS/nixpkgs#45784
@jian-lin jian-lin force-pushed the fix-zsh-session-variables branch from 0808d32 to 8fd901a Compare February 8, 2022 16:18
@jian-lin
Copy link
Member Author

jian-lin commented Feb 8, 2022

fixed test

@jian-lin
Copy link
Member Author

jian-lin commented Feb 8, 2022

cc @berbiche as you recently merged several PRs related to zsh

Copy link
Collaborator

@teto teto left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I will test this in my config.

@teto teto merged commit 2116fe6 into nix-community:master Feb 17, 2022
@jian-lin jian-lin deleted the fix-zsh-session-variables branch February 17, 2022 09:22
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 21, 2022
@clhodapp
Copy link
Contributor

I believe that this is busted because it re-uses the same environment variable name as hm-session-vars.sh.

See from the code that generates hm-session-vars.sh:

if [ -n "$__HM_SESS_VARS_SOURCED" ]; then return; fi
export __HM_SESS_VARS_SOURCED=1

and the code that this puts into .zshenv:

# Environment variables
. "${config.home.profileDirectory}/etc/profile.d/hm-session-vars.sh"
# Only source this once
if [[ -z "$__HM_ZSH_SESS_VARS_SOURCED" ]]; then
export __HM_ZSH_SESS_VARS_SOURCED=1
${envVarsStr}
fi

When hm-session-vars.sh gets sourced, it sets __HM_ZSH_SESS_VARS_SOURCED which causes the user's zsh-specific variables to get skipped!

@clhodapp
Copy link
Contributor

Never mind! I need to check my eyes! 👓

However! It does seem to be broken for some reason, as my prompt is no longer getting set... I'll post again if I find the real root cause

@clhodapp
Copy link
Contributor

Alrighty. It looks like the actual culprit was the prompt init code in /etc/zshrc that comes courtesy of the defaults from nixos:

https://github.com/NixOS/nixpkgs/blob/d5f237872975e6fb6f76eef1368b5634ffcd266f/nixos/modules/programs/zsh/zsh.nix#L90-L96

That, coupled with the fact that our variables were previously getting sourced after that by virtue of being ~/.zshrc but are now getting source before it by virtue of being in ~/.zshenv.

From the zsh man page:

STARTUP/SHUTDOWN FILES
       Commands  are  first read from /etc/zshenv; this cannot be overridden.  Subsequent behaviour is modified by the RCS and GLOBAL_RCS options; the former affects all startup files, while the second only affects global startup files (those shown here with an path starting with a /).
       If one of the options is unset at any point, any subsequent startup file(s) of the corresponding type will not be read.  It is also possible for a file in $ZDOTDIR to re-enable GLOBAL_RCS. Both RCS and GLOBAL_RCS are set by default.

       Commands are then read from $ZDOTDIR/.zshenv.  If the shell is a login shell, commands are read from /etc/zprofile and then $ZDOTDIR/.zprofile.  Then, if the shell is interactive, commands are read from /etc/zshrc and then $ZDOTDIR/.zshrc.  Finally,  if  the  shell  is  a  login
       shell, /etc/zlogin and $ZDOTDIR/.zlogin are read.

       When  a  login shell exits, the files $ZDOTDIR/.zlogout and then /etc/zlogout are read.  This happens with either an explicit exit via the exit or logout commands, or an implicit exit by reading end-of-file from the terminal.  However, if the shell terminates due to exec'ing an‐
       other process, the logout files are not read.  These are also affected by the RCS and GLOBAL_RCS options.  Note also that the RCS option affects the saving of history files, i.e. if RCS is unset when the shell exits, no history file will be saved.

       If ZDOTDIR is unset, HOME is used instead.  Files listed above as being in /etc may be in another directory, depending on the installation.

       As /etc/zshenv is run for all instances of zsh, it is important that it be kept as small as possible.  In particular, it is a good idea to put code that does not need to be run for every single shell behind a test of the form `if [[ -o rcs ]]; then ...' so that it  will  not  be
       executed when zsh is invoked with the `-f' option.

       Any of these files may be pre-compiled with the zcompile builtin command (see zshbuiltins(1)).  If a compiled file exists (named for the original file plus the .zwc extension) and it is newer than the original file, the compiled file will be used instead.

In the very short term, I've updated my system config to remove prompt suse from programs.zsh.promptInit.

@jian-lin
Copy link
Member Author

@clhodapp PROMPT should be set in .zshrc since it only needed by an interactive shell.

2116fe6#r67162926

jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 22, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 24, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 28, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 4, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 7, 2022
@NorthIsUp
Copy link

this breaks my setup as i have an existing .zshenv, while i attempt to port it over to nix, is there an override to disable the creation of the .zshenv file?

@berbiche
Copy link
Member

berbiche commented Mar 7, 2022

@NorthIsUp you could move your .zshenv to another file and source it: programs.zsh.envExtra = ". $ZDOTDIR/my-zshenv";

jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 13, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 14, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 18, 2022
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Dec 18, 2023
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Dec 20, 2023
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Dec 24, 2023
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Dec 28, 2023
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Dec 29, 2023
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 2, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 4, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 8, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 12, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 14, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 15, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 16, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 17, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 18, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 22, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 24, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 28, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Jan 30, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 4, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 5, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 8, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 12, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 19, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 21, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 22, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Feb 27, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 4, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 7, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 11, 2024
jwiegley added a commit to jwiegley/home-manager that referenced this pull request Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Session vars are loaded in different startup scripts in Bash and ZSH
6 participants