fish: native environment

[kevingriffin]

Motivation for this change

fish shell startup with programs.fish.enable set is about 2x slower than without based on my tests
and reports from others. Tracing reveals calls to fenv being the vast majority of the time.

A plugin called babelfish, which I've added as a package in #108946, can be used to pre-compute native fish code from bash at system build time, eliminating the need to call fenv during each shell launch.

This pull request is based off of #108946, but can be rebased onto master if #108946 is accepted.

I've removed calls to fenv for the init configuration specified on the fish module's configuration, as I couldn't find a reason for these to need translation, since the configuration is local to the fish module itself.

A similar pull request exists for nix-darwin, nix-darwin/nix-darwin#270, where I've vendored the package until it lands in nixpkgs.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[toonn]

Since you're probably most familiar with the tools rn. Is there a reason fenv can't be used to precompute the environment?

[pacien]

CC @bouk (#107834 (comment))

[kevingriffin]

@toonn I actually started with that approach here, but decided against it because interpolation was a problem: it's done eagerly, so if your config has environment.variables.FOO = (tty), you'll get something like (no tty)as the value, because the subshell was evaluated at precompute time, not runtime as was intended.

[cole-h]

While this is intriguing, I'd argue against switching to babelfish as the default. According to the project's own README, there is still some amount of work left to do. If anybody depends on any of those not-yet-implemented features (e.g. one of the "bunch of arithmetic expressions"), their system might no longer work as expected.

[kevingriffin]

@cole-h Thank you for the comment. I'd like to hear from @bouk about what those are specifically and generally how suitable this is.

Should it not be a suitable default, is there any path forward that allows those for whom this would work to use it? Perhaps some kind of option on the fish module? The speedup in my shell is great, and I'd love to find some way to get it to others.

[kevingriffin]

@cole-h I've committed a version with new configuration, defaulted to false, on the fish module that enables this behavior. I'd be grateful if you could take a look.

[bouk]

I'd say babelfish will work fine for 100% of the use case we have in Nixpkgs. At some point I manually went though everything to make sure it does.

What it doesn't support yet are quite niche and definitely not used in nixpkgs shell inits (things like arithmetic expressions). I will gladly take on the maintenance burden if we do find something that it doesn't work with. I know for me the speed up has been very noticeable.

[cole-h]

@bouk I'm not only talking about inside Nixpkgs, but also in users' configs. No matter how niche, if a user relies on some unimplemented arithmetic expression in the shell inits in their local configs, using babelfish will break their setup. I'm more in favor of an option where users opt into potential breakage (which, thankfully, @kevingriffin has done here). I'm glad you're interested in maintaining this, however, and look forward to it being feature-complete!

[kevingriffin]

@cole-h I've added a commit which I believe addresses all of your feedback. If it looks good, I can squash it and port the changes back to nix-darwin.

[kevingriffin]

@cole-h I just rebased onto master to get the new babelfish package. I also squashed all of my commits into a single commit, just in case this ends up being mergeable once you've done some more testing. Thank you for the help!

[kevingriffin]

@bouk I'm seeing this in my testing:

❯ nix-shell -p hello
warning: Nix search path entry '/nix/var/nix/profiles/per-user/root/channels/nixos ' does not exist, ignoring
error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I), at (string):1:13

~/src/nixpkgs on fish-native-environment
❯ echo $NIX_PATH
/home/kevin/.nix-defexpr/channels nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos  nixos-config=/etc/nixos/configuration.nix  /nix/var/nix/profiles/per-user/root/channels

Somewhere an extra space is entering into the equation, but I can't work out where it's coming in. The path in setEnvironment.fish seems correct to me. Any ideas? I'll keep testing, but I wanted to put this out there so that it's not merged before this is fixed.

(Incidentally, I don't see this on macOS with virtually the same module)

[bouk]

@kevingriffin could you post a gist of the contents of setEnvironment.fish? And maybe the untranslated bash equivalent?

[kevingriffin]

Sure. I'll note that env reports the path as being set correctly, which is very strange. Can you build a system against this and see if you see the same thing?

fish: https://gist.github.com/kevingriffin/ec289a13ed6176f96e4b9f9e1d250637
bash: https://gist.github.com/kevingriffin/49116566a3c91bc99a8e01d27b2a9676

edit: actually, no, env does show the extra spaces.

Looks like it's this:

/etc/fish
❯ set -gx BAZ_PATH "a:b"

/etc/fish
❯ echo $BAZ_PATH
a b

/etc/fish
❯ set -gx TEST_PATH "$HOME"'/.nix-defexpr/channels'(test -n "$BAZ_PATH" && echo :$BAZ_PATH || echo)

/etc/fish
❯ echo $TEST_PATH
/home/kevin/.nix-defexpr/channels a  b

The problem being that echo on a colon-delimited list ending in PATH in fish will print with spaces, which are then getting colons added to the end, which ends up making a list with extra spaces where the delimiters first were.

[kevingriffin]

@bouk I've opened bouk/babelfish#9

[bouk]

@kevingriffin I've released version 1.0.1, please give that a shot. Thanks for taking a close look 👍

[kevingriffin]

That was quick! It works in my testing. I'll package 1.0.1 and submit a PR for it.

[kevingriffin]

I rebased against master to build against babelfish 1.0.1.

[cole-h]

Diff LGTM, and functionality seems to work fine. Thanks for your collaboration on this, @kevingriffin and @bouk!

Unless anybody finds anything wrong, I plan on merging this tomorrow morning (probably around 10 AM Pacific).

[bouk]

Thanks for the work on this @kevingriffin 👍🏻

Let's see how it works for people, and maybe it could be the default further down the line.

[cole-h]

About half an hour early, but 🚀

[kevingriffin]

@cole-h Thank you! Excited to have this land. And thanks @bouk for the great software that made it possible.