Don't encourage users to break their system zsh

[mislav]

This reverts 15de0bc, which I find to be harmful advice. El Capitan finally fixed /etc/zshenv to be called /etc/zprofile, and now you're suggesting to users to undo that fix without understanding of what they were doing?

El Capitan's /etc/zprofile calls path_helper, which is a OS X utility to initialize the system PATH. This is meant to run for login shells before any user PATH configuration. Therefore, /etc/zprofile is the perfect place to call it.

If /etc/zshenv called path_helper, like it did in older versions of OS X, then it would reorder PATH on all subsequent invocations of zsh, such as nested shells (e.g. running zsh from bash) or executables with zsh in their shebang. This was a bug, and resulted in failed commands and much confusion to users, who thought that path_helper is the main culprit and some used sudo to disable this utility, further messing up their system.

---

After reverting the offending documentation addition, my approach to solving problem (as implemented in this PR) is as follows:

• Detect broken /etc/zshenv (OS X versions pre-El Capitan) and suggest the user renames them to zprofile.
• Move .zshenv configuration (runs too early) to .zshrc where it runs only once for interactive shells, and after system PATH is now set up.
• Stop suggesting in documentation that users use .zshenv. Instead, most typical user tweaks for their environment need to be in .zshrc, such as PATH edits.
• Support .zshenv.local for backwards compatibility, but issue a warning on stderr when its configuration changes PATH, and suggest to user that it gets moved to .zshrc.local.

Please observe more information for each of my commits from their commit messages.

The benefits of this approach:

• Doesn't use sudo to make harmful changes to the system, except for fixing a OS X bug. El Capitan users don't have to fix anything.
• Avoids excessive and unnecessary usage of .zshenv, thus speeding up non-interactive zsh executables.
• Uses .zshrc as it should be used: to host configuration for interactive shells.
• Helps people transition from the state where everything was messed up (OS X bug, user PATH changes in ~/.zshenv) to the preferred long-term approach.

References #307, #421, closes #423

Note that this probably "breaks" Alfred workflows again as pointed out in #289. However, Alfred purposely doesn't start shells in login mode so that they're isolated from user's terminal environment. Alfred's own documentation states that workflows should not depend on any particular setup in the user's environment. If someone makes private workflows for themselves and needs to use rbenv version switching, they should setup rbenv within the workflow.

Further reading:

• The notice about path_helper on OS X is incorrect sorin-ionescu/prezto#381
• https://github.com/Homebrew/homebrew/blob/4f990eaa65735fe306b7093aac0d71fa6e34a714/Library/Formula/zsh.rb#L51-L54
• http://sourceforge.net/p/zsh/code/ci/master/tree/StartupFiles/zshenv
• https://github.com/sstephenson/rbenv/wiki/Unix-shell-initialization#shell-init-files

/cc @geoffharcourt @ventsislaf @reshleman

[geoffharcourt]

cc: @thoughtbot/shell

[geoffharcourt]

The discussion in #307 influenced the changes made in #422, so that should be reviewed here as well.

[geoffharcourt]

@mislav, these changes and your documentation make sense to me. Given that moving path configuration to .zshenv was done after a long discussion, I want to solicit some feedback from teammates before we make another change.

[mislav]

Moving PATH configuration to .zshenv needed to be done to compensate for OS X buggy /etc/zshenv. If PATH configuration was in .zshrc, then nested zsh processes would mangle that path during system /etc/zshenv which would re-execute for every zsh process.

But if we fix /etc/zshenv by moving it to zprofile, basically replicating El Capitan fix, then the need to put PATH configuration in ~/.zshenv disappears.

BTW here is how the warning messages from this PR look like:

[jferris]

This is, unfortunately, a complex issue that doesn't seem to have an obvious answer.

There is a recommendation to set the PATH in zshenv in the zsh user manual:

It should contain commands to set the command search path, plus other important environment variables.

The user manual has some confusing instructions about zlogin and zprofile, but it seems to recommend that you don't change the environment in either:

.zprofile is meant as an alternative to .zlogin for ksh fans; the two are not intended to be used together, although this could certainly be done if desired. .zlogin is not the place for alias definitions, options, environment variable settings, etc.; as a general rule, it should not change the shell environment at all.

Many systems, such as Arch Linux, recommend configuring the path in zshenv using a unique path variable:

In short, put the following in ~/.zshenv:

~/.zshenv
typeset -U path
path=(~/bin /other/things/in/path $path[@])

Configuring the PATH in zprofile means it won't be loaded for non-login shells. This breaks compatibility with some terminal emulators and can cause issues with tmux or Vim shell commands.

I'm not sure what the correct solution is here given the weird and unfortunate history of path_helper, but issuing warnings when users modify the environment in zshenv seems incorrect to me, as zsh itself contains several recommendations for the user to do so.

[mislav]

zsh's own sample .zshenv says:

.zshenv is sourced on ALL invocations of the shell, unless the -f option is set. It should NOT normally contain commands to set the command search path, or other common environment variables unless you really know what you're doing. E.g. running "PATH=/custom/path gdb program" sources this file (when gdb runs the program via $SHELL), so you want to be sure not to override a custom environment in such cases. Note also that .zshenv should not contain commands that produce output or assume the shell is attached to a tty.

Now, as for your pushback:

Configuring the PATH in zprofile means it won't be loaded for non-login shells. This breaks compatibility with some terminal emulators

Can you be specific about which terminal emulators you speak of? Both Terminal.app and iTerm are configured to start shells in login mode by default, be it bash or zsh. Although this is not the standard in Linux distributions, it seems to be the standard on OS X. In fact, the sole reason how bash (the default shell) gets its system path is that it consults path_helper in /etc/profile— which is only loaded in login mode. El Capitan decided to mirror that for zsh by creating an equivalent /etc/zprofile. Even though they screwed up in the past, right now it seems they're trying to signal us something.

[…] and can cause issues with tmux or Vim shell commands.

Both tmux, vim, and any other program that you start from your shell will inherit the current environment. They don't need to additionally set PATH within their process because they already have correct PATH. When you issue shell commands within vim, they will be executed with the correct PATH environment that vim already inherited.

---

Do you have any specific example of how my approach could backfire? Because I have one example with how setting PATH in zshenv is sub-optimal. Observe:

1. Assume /etc/zshenv from past broken versions of OS X;
2. Assume export PATH=HELLO:"$PATH" in ~/.zshenv;
3. Assume bash is the default login shell and open a new Terminal window;
4. Launch zsh by typing zsh. The PATH in this process is now HELLO:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:HELLO (notice that "HELLO" appears two times);
5. Launch a zsh script or executable, e.g. zsh myscript.sh. The PATH in that process is now HELLO:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:HELLO:HELLO (notice that "HELLO" appears three times).

[mislav]

Another example of how putting user config in .zshenv is sub-optimal: this repository automatically configures rbenv if available, and does so in .zshenv. However, rbenv init does more than just configure PATH. It also sets up a rbenv() function for interactive shells and sources completion scripts for the current shell type. These features are not necessary and should be avoided for non-interactive shell invocations, but .zshenv will always run unconditionally nevertheless.

I realize this is specific to rbenv, but it's just an example of how encouraging people to do their config in .zshenv can lead to cargo-culting and possibly backfire.

[jferris]

zsh's own sample .zshenv says:

.zshenv is sourced on ALL invocations of the shell, unless the -f option is set. It should NOT normally contain commands to set the command search path, or other common environment variables unless you really know what you're doing.

It seems like zsh contains conflicting information, then? It recommends setting PATH in zshenv in the user manual, but then its generated zshenv file tells you not to set the PATH?

Do you have any specific example of how my approach could backfire?

We had issues specifically with tmux (this may only have occurred for users who were also using reattach-to-user-namespace, but I'm not sure) and with running commands from Vim (this may have only occurred with MacVim/gvim, but again, I'm not sure).

We've gone back and forth on this a number of times (as OS X has with path_helper). If we accept this pull request, it seems very likely to me that we're breaking something for some users and it seems equally likely that we'll get screwed by a future OS X update which changes path_helper yet again.

Given that:

• zsh offers conflicting instructions on where to set the PATH
• Different systems disagree on how the PATH will be configured
• Different versions of OS X change how this works arbitrarily

I'm wondering if the benefit we gain by configuring any PATH values in dotfiles is outweighed by the drawbacks from this PATH indecision.

We currently change PATH to do the following:

• Add /usr/local/sbin (Why do we do this? This has no value on my system. I'm guessing it affects specific homebrew installations.)
• Add the .git/safe/../../bin trick. People don't use this consistently; I'm not sure it's worthwhile.
• Add the bin directory from dotfiles itself. Maybe dotfiles, which is shared configuration, should actually not contain executable code?

@geoffharcourt @gylaz @croaky any thoughts on eliminating this issue forever by splitting out the bin directory into another repo and eliminating our tinkering with PATH?

[mislav]

I'm 👍 with any approach that this project takes as long as it doesn't suggest sudo mv /etc/{zprofile,zshenv} in its README anymore.

[geoffharcourt]

I noticed problems with Homebrew when the /usr/local/sbin directory wasn't in the path, so we'll probably want to make sure that's added, but we can do that in .zshrc.

I use the .git/safe/../../bin trick all the time to run binstubs like rake or rspec in the Ruby version of the specific repo I'm working on, but if that's not widely used I can just make that part of my personal config.

[mislav]

/usr/local/sbin directory is where Homebrew will store the nginx executable when that formula is installed.

We had issues specifically with tmux (this may only have occurred for users who were also using reattach-to-user-namespace, but I'm not sure)

When reattach-to-user-namespace is used with tmux, the shell specified should be suffixed with -l to start the shell in login mode, as Terminal or iTerm would on OS X. Example:

set -g default-command "reattach-to-user-namespace bash -l"

For users that want their shell to source /etc/profile (for bash) or /etc/zprofile (for zsh) to get system PATH, and don't want this PATH to be inherited from tmux parent process, they should add -l.

[geoffharcourt]

@mislav, thanks very much for all the research and the contribution. I made a few small comments around the $PATH warning and the rbenv invocation, but I think after those are resolved this can be merged.

[mislav]

@geoffharcourt Changes made.

I've moved broken OS X /etc/zshenv detection to post-up hook instead of being part of .zshrc. I trust that everyone who uses these dotfiles installs them with rcup?

[mike-burns]

rcup(1) seems like the only thing we can assume here.

[mislav]

Added a change so warning messages are colorized only if stderr is attached to a tty (default mode). If redirected to a log file, for instance, ANSI escape codes won't be used.

[geoffharcourt]

I think only running that check in rcup(1) is a good idea. Whenever I have a problem with the shell on either of my machines my first move is to run rcup(1) before diagnosing further.

[mislav]

@geoffharcourt Anything else I could do for you?

@jferris If this gets merged, it might be worth reverting thoughtbot/laptop@50c3e22.

[jferris]

@mislav on new OS X installs, it's located at /etc/zprofile, which is what we expect, right?

The laptop script is designed to be run on a new laptop, so I don't think we have to worry about older versions of OS X there.

[geoffharcourt]

@mislav I'm just waiting a little bit longer for any internal reports from people trying out the branch. The prior changes were made quickly in an attempt to resolve users' problems with El Capitan after upgrading, and I want to be more patient and deliberate making a significant change here. I expect to merge this in this week.

I very much appreciate your work here, just being cautious before we seesaw back from the prior recommendation.

[geoffharcourt]

Rebased and merged as f159f1a, febd718, 6c0a559, ed8619e, 5d397e6, 16219a3, 901faec. Thank you for all the research and effort.

[mislav]

Thanks for the feedback!