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.

Sign up for GitHub

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

darwin-installer: work around zshrc overwrites #9243

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+97 −22
Draft

darwin-installer: work around zshrc overwrites #9243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e21e289
install-darwin: work around zshrc overwrites
abathur Oct 12, 2023
cc5db08
nix-profile*.sh.in: append PATHs idempotently
abathur Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions scripts/install-darwin-multi-user.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,3 +222,32 @@ EOF
failure "This script needs a /nix volume with global permissions! This may require running sudo /usr/sbin/diskutil enableOwnership /nix."
fi
}

# appended to shell init
poly_extra_init_for_zshenv() {
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?

case "$1" in
"-c")
# don't interfere if this gets invoked with -c
command /usr/libexec/path_helper "$@"
;;
*)
(
# subshell to see what path_helper would do to PATH. inject a
# separator for splitting (saving previous PATH can duplicate
# entries, and people tend to see that as a smell)
eval "$(PATH="nixpathsep:$PATH" command /usr/libexec/path_helper "$@")"
local prefixed="${PATH%%:nixpathsep:*}" pushed_back="${PATH##*:nixpathsep:}"

# re-emit what path_helper would; remove function to avoid rerunning
echo "PATH=\"${pushed_back}:${prefixed}\"; export PATH; unset -f /usr/libexec/path_helper;"
)
;;
esac
}

EOF
}
52 changes: 34 additions & 18 deletions scripts/install-multi-user.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ NIX_BUILD_USER_NAME_TEMPLATE="nixbld%d"
readonly NIX_ROOT="/nix"
readonly NIX_EXTRA_CONF=${NIX_EXTRA_CONF:-}

readonly PROFILE_TARGETS=("/etc/bashrc" "/etc/profile.d/nix.sh" "/etc/zshrc" "/etc/bash.bashrc" "/etc/zsh/zshrc")
readonly PROFILE_TARGETS=("/etc/bashrc" "/etc/profile.d/nix.sh" "/etc/zshenv" "/etc/bash.bashrc" "/etc/zsh/zshrc")
readonly PROFILE_BACKUP_SUFFIX=".backup-before-nix"
readonly PROFILE_NIX_FILE="$NIX_ROOT/var/nix/profiles/default/etc/profile.d/nix-daemon.sh"

Expand Down Expand Up @@ -845,13 +845,24 @@ EOF
)
}

extra_shell_init() {
case "$1" in
/etc/zshenv)
poly_extra_init_for_zshenv;;
*)
;;
esac
}

# $1 == file we're generating for
shell_source_lines() {
cat <<EOF

# Nix
if [ -e '$PROFILE_NIX_FILE' ]; then
. '$PROFILE_NIX_FILE'
fi
$(extra_shell_init "$1")
# End Nix

EOF
Expand All @@ -870,26 +881,31 @@ end
EOF
}

configure_one_shell_init() {
local profile_target="$1"
if [ -e "$profile_target" ]; then
_sudo "to back up your current $profile_target to $profile_target$PROFILE_BACKUP_SUFFIX" \
cp "$profile_target" "$profile_target$PROFILE_BACKUP_SUFFIX"
else
# try to create the file if its directory exists
target_dir="$(dirname "$profile_target")"
if [ -d "$target_dir" ]; then
_sudo "to create a stub $profile_target which will be updated" \
touch "$profile_target"
fi
fi

if [ -e "$profile_target" ]; then
shell_source_lines "$profile_target" \
| _sudo "extend your $profile_target with nix-daemon settings" \
tee -a "$profile_target"
fi
}

configure_shell_profile() {
task "Setting up shell profiles: ${PROFILE_TARGETS[*]}"
for profile_target in "${PROFILE_TARGETS[@]}"; do
if [ -e "$profile_target" ]; then
_sudo "to back up your current $profile_target to $profile_target$PROFILE_BACKUP_SUFFIX" \
cp "$profile_target" "$profile_target$PROFILE_BACKUP_SUFFIX"
else
# try to create the file if its directory exists
target_dir="$(dirname "$profile_target")"
if [ -d "$target_dir" ]; then
_sudo "to create a stub $profile_target which will be updated" \
touch "$profile_target"
fi
fi

if [ -e "$profile_target" ]; then
shell_source_lines \
| _sudo "extend your $profile_target with nix-daemon settings" \
tee -a "$profile_target"
fi
configure_one_shell_init "$profile_target"
done

task "Setting up shell profiles for Fish with ${PROFILE_FISH_SUFFIX} inside ${PROFILE_FISH_PREFIXES[*]}"
Expand Down
4 changes: 4 additions & 0 deletions scripts/install-systemd-multi-user.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,7 @@ poly_create_build_user() {
poly_prepare_to_install() {
:
}

poly_extra_init_for_zshenv() {
:
}
11 changes: 9 additions & 2 deletions scripts/nix-profile-daemon.sh.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Only execute this file once per shell.
if [ -n "${__ETC_PROFILE_NIX_SOURCED:-}" ]; then return; fi
__ETC_PROFILE_NIX_SOURCED=1
export __ETC_PROFILE_NIX_SOURCED=1

NIX_LINK=$HOME/.nix-profile
if [ -n "${XDG_STATE_HOME-}" ]; then
Expand Down Expand Up @@ -60,5 +60,12 @@ else
unset -f check_nix_profiles
fi

export PATH="$NIX_LINK/bin:@localstatedir@/nix/profiles/default/bin:$PATH"
# only append once
case ":$PATH:" in
*:"$NIX_LINK/bin:@localstatedir@/nix/profiles/default/bin":*)
;;
*)
export PATH="$NIX_LINK/bin:@localstatedir@/nix/profiles/default/bin:$PATH"
;;
esac
unset NIX_LINK
23 changes: 21 additions & 2 deletions scripts/nix-profile.sh.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# Only execute this file once per shell.
if [ -n "${__ETC_PROFILE_NIX_SOURCED:-}" ]; then return; fi
export __ETC_PROFILE_NIX_SOURCED=1

if [ -n "$HOME" ] && [ -n "$USER" ]; then

# Set up the per-user profile.
Expand Down Expand Up @@ -51,9 +55,24 @@ if [ -n "$HOME" ] && [ -n "$USER" ]; then
# pick up `.nix-profile/share/man` because is it close to `.nix-profile/bin`
# which is in the $PATH. For more info, run `manpath -d`.
if [ -n "${MANPATH-}" ]; then
export MANPATH="$NIX_LINK/share/man:$MANPATH"
# only append once
case ":$MANPATH:" in
*:"$NIX_LINK/share/man":*)
;;
*)
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.


export PATH="$NIX_LINK/bin:$PATH"
# only append once
case ":$PATH:" in
*:"$NIX_LINK/bin":*)
;;
*)
export PATH="$NIX_LINK/bin:$PATH"
;;
esac

unset NIX_LINK NIX_LINK_NEW
fi