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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

darwin-installer: work around zshrc overwrites #9243

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abathur
Copy link
Member

@abathur abathur commented Oct 26, 2023

Motivation

Context

We know of 2 ~workarounds to the problem that doesn't entail compromising on the general purpose of a multi-user install. I'll save thoughts on pros/cons for a later post and focus on nuts and bolts:

  1. Set up a ~persistence-daemon that will attempt to restore the shell hooks on boot if they go missing. This has recently been implemented in the detsys installer: Make our Nix installation immune to macOS upgrades聽DeterminateSystems/nix-installer#672

  2. Move our shell hook to /etc/zshenv on macOS (which macOS doesn't ship, and updates presumably don't touch). This doesn't work by itself (indeed, a previous PR trying this was reverted) because the /etc/zprofile that ships with macOS will run path_helper later in the zsh init process. Evaling the output from path_helper resets PATH with items from /etc/paths, /etc/paths.d/*, and the default system PATH all to the left of anything we've added before that point in init (which includes anything we add in /etc/zshenv).

    We need one of two potential fixes to make zshenv work:

    1. add the Nix paths to /etc/paths. This would put them in the right position, but it's just a static list of hardcoded paths (and macOS does ship the file, so we'd have to worry about them overwriting it on update as well).
    2. shadow/wrap /usr/libexec/path_helper in order to keep it from pushing our PATH additions to the rear of the PATH.

This PR implements 2.ii by declaring a shell function that shadows /usr/libexec/path_helper and adding it only to /etc/zshenv. (The shell function currently removes itself when it runs. I'm not quite sure if that's the right call or not.)


While not strictly related, this PR also tries to address the PATH-appending issue because of a fundamental difference between zshrc and zshenv--zsh runs the latter for all shells, and the former for interactive ones. IIUC, this means a hook in zshenv can run when zsh is used as a shebang.

I'm not certain this PR makes the right set of choices here (I'm not very familiar with zsh; I'm just tired of watching people run into this particular sharp corner...), but I'm implementing two approaches mentioned in #5950: exporting __ETC_PROFILE_NIX_SOURCED to minimize reruns, and trying to make the actual append step idempotent.

I think this will mitigate one known issue with moving the hook to zshenv which was pointed out by the detsys folks (see the 2nd bullet under https://github.com/DeterminateSystems/nix-installer#using-macos-remote-ssh-builders-nix-binaries-are-not-on-path), but I'm less certain that it's the right fix in all circumstances.

Testing notes

  1. I've run installer tests in my fork: https://github.com/abathur/nix/actions/runs/66490502622.
  2. I've manually tested a fresh install of this on a spare pre-catalina mac I have laying around. I don't use zsh and don't have it set up for day-to-day use, so this is just a basic check.
  3. @iFreilicht has been helping with some manual testing (not clean install; just spot-applying equivalent changes) and should be able to weigh in on how this is going.
  4. I have also set up a gist with instructions for zsh users willing to help test this (but please only do this if you know you're comfortable enough with shell init/environment issues to recognize if this causes any subtle problems and work around trouble). It describes how to do a fresh install on x86_64 macs (the test installer won't support Apple Silicon macs, unfortunately) and how to ~patch an existing install to try it: https://gist.github.com/abathur/877a4fa25ec7047baeb3033a9153b269

Priorities

Add 馃憤 to pull requests you find important.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-survival-mode-macos-upgrades-wont-break-nix-anymore-when-you-install-nix-using-the-determinate-nix-installer/34593/10

*)
export MANPATH="$NIX_LINK/share/man:$MANPATH"
;;
esac
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how well this will work. When I run echo $MANPATH, I see this in kitty:

/Applications/kitty.app/Contents/Resources/man:

And get empty output in Terminal.

This is in a session where both the default and my user profile have been sourced and man can access manpages installed by nix.

Running manpath -d:

-- Searching PATH for man directories
--   Adding /Users/feuh/.nix-profile/share/man to manpath
--   Adding /nix/var/nix/profiles/default/share/man to manpath
--   Adding /usr/share/man to manpath
--   Adding /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/share/man to manpath
--   Adding /Library/Developer/CommandLineTools/usr/share/man to manpath
-- Adding default manpath entries
-- Parsing config file: /etc/man.conf
-- Using manual path: /Users/feuh/.nix-profile/share/man:/nix/var/nix/profiles/default/share/man:/usr/share/man:/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/share/man:/Library/Developer/CommandLineTools/usr/share/man
/Users/feuh/.nix-profile/share/man:/nix/var/nix/profiles/default/share/man:/usr/share/man:/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/share/man:/Library/Developer/CommandLineTools/usr/share/man

But the environment variable is still empty. Very strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what you're observing here is that the block will only operate if MANPATH is already set when the shell hook runs, because it's guarded with:

if [ -n "${MANPATH-}" ]; then

This doesn't sound like a problem, though I'm less certain about whether a knock-on implication of your observation and this change would be a problem:

Moving Nix's shell hook from /etc/zshrc to /etc/zshenv does move it up in the execution order, which in turn comes with some risk of shifting behavior for any users that explicitly set MANPATH before the /etc/zshrc hook runs, but not before the /etc/zshenv hook would run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that seems to only be a problem if the order of paths in PATH and MANPATH differs. In that case, you could hit a situation where by order of PATH, you execute one version of a program, but by order of MANPATH, the manpage of a different version would be shown. On macOS in particular, the highest disruption around this would probably be related to coreutils, where the BSD versions are used instead of the GNU versions, and git, where Apple provides their own fork that interacts well with the macOS keychain. In these cases, though, they're installed system-wide, and their PATH is not mentioned in /etc/zshrc.

However, if PATH (or MANPATH) were to be set before the nix hook, that could indeed change the behavior.

So the only situation where this change would cause an effect is if a user changed /etc/zshrc to contain something like this:

PATH=/home/myself/repos/myprogram/bin:$PATH

# Nix
if [ -e /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh ]; then
  . /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh
# End Nix

This seems exceedingly unlikely though. Normally you customize the zshrc of your user, and if you want one of your paths to take precedence, you just add it in the front:

PATH=$PATH:/home/myself/repos/myprogram/bin

I would say the positive impact of this change far outweighs the small likelyhood that this might potentially break some obscure configuration change that the next OS update would have wiped out anyway.

Copy link
Contributor

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

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

From my side this looks good, all the testing I've done so far has shown this to work exactly as I expect, including proper handling in ssh, nix develop -c zsh (which is a common workaround for zsh users) and direnv.

I could not test the installer as I'm on an aarch64 mac and did not test in tmux as I'm not familiar with it.

Even for a flake that itself ships a version of nix like this

      pkgs.mkShell {
        packages = pkgs.nixVersions.nix_2_17;
      };

the order of versions of nix in the path is correct:

$ whence -a nix
/Users/feuh/.nix-profile/bin/nix
/nix/var/nix/profiles/default/bin/nix
$ nix --version
nix (Nix) 2.15.1
$ nix develop -c zsh
$ whence -a nix
/nix/store/ibzj90n2lxmd8jbph5izcgz76f46r514-nix-2.17.1/bin/nix
/Users/feuh/.nix-profile/bin/nix
/nix/var/nix/profiles/default/bin/nix
$ nix --version
nix (Nix) 2.17.1

This works in both kitty and Terminal, and I tried replacing nix develop -c zsh with direnv allow, and the impact on the path is identical, which is great!

The other concern that PATH can be overwritten when zsh is used in a shebang was also tested.

$ cat ./empty-path.sh
#!/bin/zsh
echo $PATH
$ PATH= ./empty-path.sh

Overall, for the parts that I could test, this seems like a solid solution.

scripts/install-darwin-multi-user.sh Outdated Show resolved Hide resolved
cat <<'EOF'

# wrap path_helper to keep it from pushing nix rightwards
/usr/libexec/path_helper() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit concerned that this is a pretty surprising way to implement this. If I came across something like this on my own I'd start from the assumption that my system was compromised ...

Is there a way this could be done in a less surprising way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so--and this is a good complaint.

That said, both workarounds have this property to some degree. What would you think if you weren't aware of the repair-daemon's presence, intentionally removed the Nix shell hook, and found it in place later?

Reaching a consensus decision about which is least-surprising will probably be the main task ahead here. I'm on the fence, myself :)

Copy link
Member

Choose a reason for hiding this comment

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

That's a good note. I don't know :').

Copy link
Contributor

Choose a reason for hiding this comment

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

One additional issue I'm seeing with this change is that it only fixes the issue for new installations. Existing installations have to be manually removed and then re-installed or manually fixed. Even if #7603 was merged, one could not simply re-run the installer, as that would leave the change in /etc/zshrc from the previous run.

@grahamc AFAIU, this is not an issue for the repair-daemon; you can just run the installer again and an existing installation will receive the fix, correct?

macOS updates have been overwriting /etc/zshrc for a while.
This evicts the Nix shell hook, causing quite a few support
threads (and inevitabling harming people's impression of
the project).

This workaround *is* cursed :)
When people source the Nix profile scripts more than once,
they can cause appended PATH variables to grow longer each
time. This is somewhat benign, but it does spook people and
cause support load.
@abathur
Copy link
Member Author

abathur commented Nov 3, 2023

@lilyball curious what you think about the two approaches :)

@lilyball
Copy link
Member

lilyball commented Nov 3, 2023

It's been a while since I've looked at this issue. I recall in the past suggesting that nix-daemon should just repair the installation itself, though having a separate launchd at-boot entry for it works pretty much the same way. But what this PR is doing is interesting too. I am not a ZSH user myself so I can't really evaluate it, though I am pretty surprised that ZSH (and apparently Bash too!) lets you define functions with slashes in them so at least I learned something.

Doesn't macOS also break the bash installation? Fixing ZSH is nice, but it kind of sucks if Bash is left as the only broken shell. Having nix-daemon or a separate launchd script repair the installation means it can fix Bash too.

@abathur
Copy link
Member Author

abathur commented Nov 4, 2023

Doesn't macOS also break the bash installation? Fixing ZSH is nice, but it kind of sucks if Bash is left as the only broken shell. Having nix-daemon or a separate launchd script repair the installation means it can fix Bash too.

I think some people have said it can, though I haven't experienced it myself (and virtually all reports here are zsh users). IME when they want to overwrite bashrc they instead write the incoming file to Relocated Items.

iFreilicht added a commit to iFreilicht/.dotfiles that referenced this pull request Nov 7, 2023
This workaround didn't really work that well anyway, and as I'm testing
NixOS/nix#9243 right now, it's not necessary
anymore.
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-installer-workgroup/21495/27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS updates often break nix installation (updates replace path-hooks on multi-user install)
5 participants