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

Add Fish suport to the Nix installer #7014

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Sep 8, 2022

Before this patch, installing Nix using the Fish shell did not work because Fish wasn't configured to add Nix to the PATH. Some options in #1512 offered workarounds, but they typically involve extra plugins or packages.

This patch adds native, out-of-the-box support for the Fish shell.

Note that Fish supports a conf.d directory, which is intended for exactly use cases like this: software projects distributing shell snippets. This patch takes advantage of it. The installer doesn't append any Nix loader behavior to any Fish config file. Because of that, the uninstall process is smooth and a reinstall obliterates the existing nix.fish files that we place instead of bothering the user with a backup / manual removal.

Both single-user and multi-user cases are covered. It has been tested on Ubuntu, and a Mac with MacPorts, homebrew, and the Fish installer pkg.

Written in collaboration with @Hoverbear at DetSys.

Closes #1512

@grahamc grahamc changed the title Add Fish suport to installer Add Fish suport to the installer Sep 8, 2022
@grahamc grahamc changed the title Add Fish suport to the installer Add Fish suport to the Nix installer Sep 8, 2022
@edolstra
Copy link
Member

edolstra commented Sep 9, 2022

See also #4689.

@fricklerhandwerk fricklerhandwerk added installer UX The way in which users interact with Nix. Higher level than UI. labels Sep 9, 2022
scripts/install-nix-from-closure.sh Show resolved Hide resolved
scripts/install-nix-from-closure.sh Outdated Show resolved Hide resolved
scripts/install-nix-from-closure.sh Show resolved Hide resolved
scripts/nix-profile.fish.in Outdated Show resolved Hide resolved
end

set --export --prepend --path PATH "@localstatedir@/nix/profiles/default/bin"
set --export --prepend --path PATH "$HOME/.nix-profile/bin"
Copy link
Member

Choose a reason for hiding this comment

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

Query at the end of nix-profile.fish.in applies to these two as well.

@edolstra
Copy link
Member

The main reason why we haven't merged fish support in the past is that it lacks tests, and without that, it's likely to bitrot. So as part of the installer testing, we need to make sure we test fish as well.

@edolstra
Copy link
Member

BTW, we may want to consider the use of /etc/environment on systems that use pam_env.so, since that allows setting environment variables for any shell.

@Hoverbear
Copy link
Contributor

@edolstra I'll open up a ticket about etc/environment and we can address it in a future PR.

Let me add a fish test.

@Hoverbear
Copy link
Contributor

(Please pardon me, it seems to be a Github Actions driven test with no local equivalent, so I need to "test on ci")

@abathur
Copy link
Member

abathur commented Sep 12, 2022

@Hoverbear You may have figured this out since it indicates that the installer test is expected, but just in case there's documentation on enabling the install tests in your fork in #6652.

(I do wonder if it'll make performance sense to just collapse each shell-specific invocation into the same install-test run for speed, but that can wait for confirmation that the matrix approach is working right...)

@Hoverbear
Copy link
Contributor

Hoverbear commented Sep 12, 2022

It seems to be ok: https://github.com/Hoverbear/nix/actions/runs/3039672534

image

It does seem like it would make performance sense. Let me fix that.

@Hoverbear
Copy link
Contributor

image
Fantastic!

@Hoverbear
Copy link
Contributor

I think all comments are addressed. If there is anything I'm missing please let me know and I can do it right away!

Hoverbear and others added 2 commits September 13, 2022 12:56
Before this patch, installing Nix using the Fish shell did not
work because Fish wasn't configured to add Nix to the PATH. Some
options in NixOS#1512 offered workarounds, but they typically involve
extra plugins or packages.

This patch adds native, out-of-the-box support for the Fish shell.

Note that Fish supports a `conf.d` directory, which is intended
for exactly use cases like this: software projects distributing
shell snippets. This patch takes advantage of it. The installer
doesn't append any Nix loader behavior to any Fish config file.
Because of that, the uninstall process is smooth and a reinstall
obliterates the existing nix.fish files that we place instead of
bothering the user with a backup / manual removal.

Both single-user and multi-user cases are covered. It has been
tested on Ubuntu, and a Mac with MacPorts, homebrew, and the
Fish installer pkg.

Closes NixOS#1512

Co-authored-by: Graham Christensen <[email protected]>
@grahamc grahamc force-pushed the graham/ds-327-fish-support-for-the-nix-installer branch from c240c3d to 7194c87 Compare September 13, 2022 16:57
@edolstra edolstra added the backport 2.11-maintenance Automatically creates a PR against the branch label Sep 14, 2022
@edolstra edolstra merged commit 88a45d6 into NixOS:master Sep 14, 2022
@github-actions
Copy link

Git push to origin failed for 2.11-maintenance with exitcode 1

@grahamc grahamc deleted the graham/ds-327-fish-support-for-the-nix-installer branch September 14, 2022 12:47
@grahamc grahamc mentioned this pull request Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.11-maintenance Automatically creates a PR against the branch installer UX The way in which users interact with Nix. Higher level than UI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installing doesnt work under fish shell
7 participants