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

fish: fix environment clobbering and improve initialization #24314

Merged
merged 4 commits into from
May 10, 2017

Conversation

therealpxc
Copy link
Contributor

@therealpxc therealpxc commented Mar 25, 2017

Motivation for this change

fixing this bug

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • [] Fits CONTRIBUTING.md.

@mention-bot
Copy link

@therealpxc, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jgillich, @rnhmjoj and @Profpatsch to be potential reviewers.

@therealpxc therealpxc changed the title fish: fix environment clobbering fish: fix environment clobbering and improve initialization Mar 25, 2017
@Profpatsch
Copy link
Member

This looks awesome.
Does that mean I can have fish as my nix-shell shell now? If so, how do I enable it with that patch?

@therealpxc
Copy link
Contributor Author

It sure does, my friend!

~/Playground  cat ./fishie-shell.nix                                                 3698ms  Sat 25 Mar 2017 08:54:34 AM MST
with import <nixpkgs> {};
stdenv.mkDerivation {
  name = "whatever";
  buildInputs = [
    pkgs.psmisc # for killall
  ];
}
~/Playground  which killall                                                                   Sat 25 Mar 2017 08:54:37 AM MST
which: no killall in (/home/pxc/bin:/run/wrappers/bin:/run/wrappers/bin:/home/pxc/.nix-profile/bin:/home/pxc/.nix-profile/sbin:
/home/pxc/.nix-profile/lib/kde4/libexec:/nix/var/nix/profiles/default/bin:/nix/var/nix/profiles/default/sbin:/nix/var/nix/profi
les/default/lib/kde4/libexec:/run/current-system/sw/bin:/run/current-system/sw/sbin:/run/current-system/sw/lib/kde4/libexec:/ni
x/store/sq1q54snjvwk5ka7iwdv04nn42h84r55-dbus-1.10.16/bin:/nix/store/zks6v9wdc7jp8wg0yd74b48sdsavgcgw-qttools-5.7.1/bin:/nix/st
ore/g4kkh32q8rj390fyz1fwqqlpy2vbz3qj-socat-1.7.3.2/bin:/nix/store/avmxym1w34sc17nrilsmgrk469l3ml0z-gnugrep-3.0/bin:/nix/store/0
xwrn1p8fp8h3cynszpgbmhmydbzhns5-gnused-4.4/bin:/nix/store/zlqp9kb8399hvihidlnzqsad3q1ivbxz-kconfig-5.31.0/bin:/nix/store/zlqp9k
b8399hvihidlnzqsad3q1ivbxz-kconfig-5.31.0/lib/libexec:/nix/store/zlqp9kb8399hvihidlnzqsad3q1ivbxz-kconfig-5.31.0/lib/libexec/kf
5:/nix/store/40b3czpgrs222fadr11y22phxxj6nm1d-kinit-5.31.0/bin:/nix/store/40b3czpgrs222fadr11y22phxxj6nm1d-kinit-5.31.0/lib/lib
exec:/nix/store/40b3czpgrs222fadr11y22phxxj6nm1d-kinit-5.31.0/lib/libexec/kf5:/nix/store/mfran48hv83bmi9qiiklm0952zq45bf3-kserv
ice-5.31.0/bin:/nix/store/cmypgamykrfys6aa1mz8zdzfqz3vcjv5-plasma-workspace-5.8.6/bin:/nix/store/cmypgamykrfys6aa1mz8zdzfqz3vcj
v5-plasma-workspace-5.8.6/lib/libexec:/nix/store/1qc099pc7iydmj7ly2b1l73k3rs7vpn9-xmessage-1.0.4/bin:/nix/store/bkjji24z8prs7kp
4ckddfzbz8h8j02m0-xprop-1.2.2/bin:/nix/store/n52jjwm629mbfk5k6lcp7p065139wqwv-xsetroot-1.1.0/bin:/nix/store/pxwylyb489aliqql7mz
58xshh2cs8m52-yakuake-3.0.2/bin)

! %  ~/Playground  nix-shell ./fishie-shell.nix                                              Sat 25 Mar 2017 08:54:43 AM MST

[nix-shell:~/Playground]$ killall
Usage: killall [OPTION]... [--] NAME...
      killall -l, --list
      killall -V, --version

  -e,--exact          require exact match for very long names
  -I,--ignore-case    case insensitive process name match
  -g,--process-group  kill process group instead of process
  -y,--younger-than   kill processes younger than TIME
  -o,--older-than     kill processes older than TIME
  -i,--interactive    ask for confirmation before killing
  -l,--list           list all known signal names
  -q,--quiet          don't print complaints
  -r,--regexp         interpret NAME as an extended regular expression
  -s,--signal SIGNAL  send this signal instead of SIGTERM
  -u,--user USER      kill only process(es) running as USER
  -v,--verbose        report if the signal was successfully sent
  -V,--version        display version information
  -w,--wait           wait for processes to die

[nix-shell:~/Playground]$ fish
Linux davetim 4.9.17 x86_64
08:54am  up   9:20,  1 user,  load average: 0.46, 0.35, 0.31
~/Playground  killall                                                                         Sat 25 Mar 2017 08:54:54 AM MST
Usage: killall [OPTION]... [--] NAME...
      killall -l, --list
      killall -V, --version

  -e,--exact          require exact match for very long names
  -I,--ignore-case    case insensitive process name match
  -g,--process-group  kill process group instead of process
  -y,--younger-than   kill processes younger than TIME
  -o,--older-than     kill processes older than TIME
  -i,--interactive    ask for confirmation before killing
  -l,--list           list all known signal names
  -q,--quiet          don't print complaints
  -r,--regexp         interpret NAME as an extended regular expression
  -s,--signal SIGNAL  send this signal instead of SIGTERM
  -u,--user USER      kill only process(es) running as USER
  -v,--verbose        report if the signal was successfully sent
  -V,--version        display version information
  -w,--wait           wait for processes to die

But my nix-shell-fu is weak. What's the most natural way to have the shell drop to fish instead of bash as soon as you enter it? Is it some neat little shebang trick?

@Mic92
Copy link
Member

Mic92 commented Mar 26, 2017

nix-shell --command 'fish' should work. You can also use the nix integration of direnv, which provides fish shell support.

@therealpxc
Copy link
Contributor Author

therealpxc commented Apr 4, 2017

Hey, folks! I made some additional changes. The most important is the first, imo, but here they are

  1. Fish on NixOS now finishes sourcing the NixOS environment before going on to source fish snippets (e.g., ~/.config/fish/conf.d/*). Previously, on NixOS 17.03 and newer, if you sourced Oh-My-Fish from such a snippet, your shell (and possibly login manager) was likely to break. I got some help on figuring out how to do this from some ‘swimmers’, particularly @faho. This required modifying the fish package as well as the module.

  2. I added more support for vendor-provided fish scripts. Previously we had some support for incorporating vendor completions, but none for vendor configuration snippets or vendor functions. I added support for the latter two as options and converted the former to an option as well.

  3. I added fish hooks to a couple of tools that I use, namely fzf and direnv, to test and demonstrate item A small set of package updates #2.

I would like to make the support for fish hooks a little more general/streamlined instead of just having packages that use them by copying their fish scripts to magic paths, but I'm not yet sure how.

I would also like to make something like a custom fish package that links in all the snippets, functions, and completions it wants so that we can take advantage of these features on non-NixOS. I have some ideas, but if you have any suggestions they would be much appreciated.

I've still got more cleaning up to do and I'm not yet sure that everything works perfectly right, but I would appreciate your feedback and thoughts.

@jgillich
Copy link
Member

jgillich commented Apr 4, 2017

I looked into #18946, I think the solution is rather simple: setEnvironment and shellInit should be wrapped in status --is-login. In the case of a non-login shell, the environment would be inherited from the parent. The bash module does the same, it puts them in /etc/profile - read only for login shells.

Although I have to admit, I have no idea why $__ETC_PROFILE_SOURCED exists, since /etc/profile would only be sourced in login shells and not subshells? Can login shells be nested? @edolstra bcbe2dc

If we're going to parse the initialization variable for bash and zsh in fish, that makes me wonder why we're not doing the same in all shell modules (or just use a common variable).

@therealpxc
Copy link
Contributor Author

I think using a common variable for checking whether the NixOS environment has been sourced would be a good idea. Right now the behavior for other shells is to clobber environments set up by one another, which seems like an oversight.

@therealpxc
Copy link
Contributor Author

I made some silly mistakes earlier. The most recent version fixes them and I can confirm that and vendor_functions.d, vendor_completions.d, vendor_conf.d all get linked in correctly now.

@therealpxc
Copy link
Contributor Author

Btw, @jgillich , what were your plans for the promptInit option? It doesn't currently do anything and I'm not sure fish needs one in addition to interactiveShellInit.

@nagisa
Copy link
Contributor

nagisa commented Apr 24, 2017

ping pong bump bleep

# set -l profiles (echo \$NIX_PROFILES | ${coreutils}/bin/tr ' ' '\n')[-1..1]
# set fish_complete_path \$profiles"/share/fish/vendor_completions.d" \$fish_complete_path
# end
# EOF
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this if it is not needed anymore.

EOF
'' + ''
tee -a $out/share/fish/__fish_build_paths.fish < ${./__fish_build_paths.suffix.fish}
Copy link
Member

Choose a reason for hiding this comment

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

Why is tee used in both cases and not cat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because to do the same thing with cat one would need to explicitly use temporary files (you can't use cat to read from a file and modify the same file in-place), I think. I went with it because it was the method in place when I started working on this code. I'm aiming to replace it with a better way of handling fish configurations via Nix, but I think that work is properly out of scope for this PR. (See pending comment in the main thread for this PR)

@therealpxc
Copy link
Contributor Author

Hey, @nagisa !

Sorry this thing hasn't seen much activity for a little while. I've been playing with some related changes on my personal Nixpkgs fork, and I currently lack clean commits for all the changes I wanted to make to this. Those changes will take a bit longer for me to work out in full and probably ought to be in another pull request anyway.

But I will try in the next few hours to clean up this PR by removing things related to my more experimental work and all commented-out code to get this into a suitable state for merging ASAP. I'll push the number of commits back down, too.

@therealpxc
Copy link
Contributor Author

therealpxc commented Apr 24, 2017

@Mic92 , @jgillich

Handling fish configuration in NixOS using the tee command and string interpolation in a NixOS-specific way seems to me to be not great. I'm working out doing it in a better way right now, but I'm still experimenting, which is why I haven't cherry-picked those changes over from my experimental branch just yet. But here's the state of the union:

Here's the state of the union:

I don't love this way of putting the config files together either, but changing it for this update seems out of scope. I'm working on other changes right now on my messy, personal Nixpkgs branch. I'd like to see those changes merged eventually some day soon too, but the basic idea is using Nix to represent a complete fish configuration (completions folder with files, snippets folder with files, functions folder with files, main config file, etc.) through buildEnv. Those fish-config paths in the Nix store can then be used in the following ways (1-3 are the important ones imo)

  1. To specify a fish-config at build time to compile a particular configuration in with fish
    • note: currently fish on Nixpkgs is compiled to read all the same files as a fish-config represents from $out/etc where $out is the final store path of the fish package being built. All this integration would do is generate those files through the mechanisms used to generate a fish-config, then link them in. So right now we're building fishes that read from configs we can't specify through Nix. This would give us more flexibility for defining custom fish packages and not necessarily add anything to the binary caches.
  2. To manage layered fish configurations by installing fish-configs to Nix profiles
    • note: to implement this, all we have to do is patch $__fish_datadir/config.fish ($out/share/config.fish in the fish package) to inject the appropriate sections of fish-config paths into wherever they are usually read. So snippets, for example, will be read from the paths in the list
      • ~/.config/fish/conf.d
      • /nix/var/nix/profiles/per-user/(whoami)/profile/etc/fish/conf.d or ~/.nix-profile/etc/fish/conf.d
      • /nix/var/nix/profiles/default/etc/fish/conf.d
      • /nix/var/nix/profiles/system/etc/fish/conf.d
    • Practically, this just means inserting a few entries in an array during fish initialization. Fish already includes platform-specific initialization hooks, and upstream fish developers have expressed (vaguely; I think we'd still owe them a working example and a clear, concise account of the motivations) a willingness to make Fish similarly Nix-aware upstream, so if the Nix community thinks this is a sound way to handle things, I'd like to see it integrated into fish and then we won't have to modify our fish packages to get fish to read from Nix profiles.
  3. Have NixOS accommodate all fish configuration options by passing through its options to generate an appropriate fish-config, then simply adding it to environment.systemPackages. This gives Nix and NixOS users identical syntax and behavior for declaratively configuring fish through Nix.
  4. Eventually fish-config can be enhanced to handle oh-my-fish and other fish framework packages and their system dependencies.
  5. With further enhancements, one can use a modified fish-config to build a special fish package which doesn't read from /etc, /home, or anywhere not built-in to produce robust/portable fish scripts which install their own dependencies through Nix, use a fixed version of fish, and never access outside paths. (With an extended version of the same tricks necessary to prevent shell initialization breakage when user snippets depend on programs installed to Nix profile paths, this can be done so that all portable fish scripts which rely on the same upstream fish version can rely on a common package for fish, no rebuilding just to change the config)

I'm currently working on a solution that does all of that, and in doing it I'm generating the configuration files in a more Nix-native way (using builtins for writing files and so on, then pulling them all together with buildEnv) than using bash/tee hacks in a postPatchPhase). Right now I am testing to make sure that I generate those fish-config environments correctly, and I think that part is done except for oh-my-fish support (which I'll add later) but I haven't switched the fish package over to using them yet.

Does this seem like a reasonable approach? For users uninterested in use cases 2-4, these changes will have no effect except adding a Nix store path for the default fish configuration used in the default fish package. But that's just symlinks, it shouldn't even really take up more space.

Anyway, that effort is why I'm not super concerned about style issues related to the way config files are generated in the NixOS module for fish at this time.

@therealpxc therealpxc force-pushed the fish branch 2 times, most recently from a494f53 to b49cc43 Compare May 3, 2017 23:59
@therealpxc
Copy link
Contributor Author

I've cleaned thing up a little. Please let me know what you think.

I followed the suggestion of @jgillich with using status --is-login to determine whether to update the environment, but not for the actual NixOS shell configuration. This is just to keep parity with the way configuration for other shells is handled on NixOS. This means that if you run a login shell from within an interactive shell, it will not source the interactive shell configuration over again, but just the login shell configuration, and so on.

end

if status --is-interactive
# if our parent shell didn't source the login config, do it
status --is-interactive; and not set -q __fish_nixos_login_config_sourced
Copy link

Choose a reason for hiding this comment

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

These variable names are switched around - the "login" one should belong to the status --is-login block, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ay ay, this is mixed up in several ways. Thanks for pointing that out. I'll get that fixed right away.

@therealpxc
Copy link
Contributor Author

Thanks for your review @faho ! I'm embarrassed for not catching that earlier, but I'm glad you found it. :-)

FWIW: I've been running this version of the NixOS module for fish and the fish package on my two NixOS machines (my laptop on my multimedia machine) and it successfully initializes my omf config. Using fish as in nix-shell --fish works correctly (no clobbering), and the direnv and fzf fish hooks are loaded automatically (i can remove them from my config and they still work). (The fzf keybinds don't get assigned yet because those are implemented as a little oh-my-fish package and I haven't added oh-my-fish support yet, but the fzf_key_bindings function is made available according to its fish function path.)

@seanparsons
Copy link
Contributor

@Mic92 Given that this PR is the source of the above warnings and the mosh failure above (both hitting me), shouldn't it be reverted out of 17.03?

@therealpxc
Copy link
Contributor Author

therealpxc commented May 15, 2017 via email

@seanparsons
Copy link
Contributor

The mosh issue is just the one listed just above #25789. It's not urgent for me, just might be inconvenient because I tend to use it on the train. @therealpxc The work is appreciated though, trust me, I'm not whinging about it! I just thought I should raise the possibility of a revert just because it may block others.

@seanparsons
Copy link
Contributor

@therealpxc So I've just started seeing this with git annex:

Please make sure you have the correct access rights
and the repository exists.
fish: Unknown command 'git-receive-pack /home/sean/'
fish: git-receive-pack '/home/sean/'
      ^
fatal: Could not read from remote repository.

I'm surmising it's the same thing as what's going on with mosh, which makes me think it's a general issue with non interactive shells.

@therealpxc
Copy link
Contributor Author

therealpxc commented May 15, 2017 via email

@seanparsons
Copy link
Contributor

@therealpxc I'll have a looky see and give it a bash myself.

@therealpxc
Copy link
Contributor Author

therealpxc commented May 15, 2017 via email

@rnhmjoj
Copy link
Contributor

rnhmjoj commented May 15, 2017

diff --git a/pkgs/shells/fish/default.nix b/pkgs/shells/fish/default.nix
index 32b7694ba3..35a548eaac 100644
--- a/pkgs/shells/fish/default.nix
+++ b/pkgs/shells/fish/default.nix
@@ -46,8 +46,7 @@ let
 
   fishPreInitHooks = ''
     # source nixos environment if we're a login shell
-    builtin status --is-login
-    and test -f /etc/fish/nixos-env-preinit.fish
+    test -f /etc/fish/nixos-env-preinit.fish
     and source /etc/fish/nixos-env-preinit.fish
 
     test -n "$NIX_PROFILES"

This apparently fix the problem with mosh.

@therealpxc
Copy link
Contributor Author

therealpxc commented May 15, 2017 via email

@vcunat
Copy link
Member

vcunat commented May 16, 2017

Also note that stable branches are dead ends – normally the code only moves the other way (master -> stable) via cherry-picks.

@Mic92
Copy link
Member

Mic92 commented May 16, 2017

@vcunat these commits are in both branches by now.

@seanparsons
Copy link
Contributor

@Mic92 At the risk of asking a stupid question, that's only in your fork right?

@therealpxc
Copy link
Contributor Author

@seanparsons No. Here it is on master, and here it is on 17.03.

The fixes for the issues related to this PR are available in two commits in a new PR. Feel free to cherry-pick them for your own use.

@seanparsons
Copy link
Contributor

@therealpxc I think we've got our wires crossed, I'm referring to the commits that fix this issue that are in the PR. The stuff you're linking to is from March.

Mic92 pushed a commit that referenced this pull request May 22, 2017
sourced
(this fixes issue #25789:
#25789 (comment) and
the issue with git-annex mentioned here
#24314 (comment) )
Mic92 pushed a commit that referenced this pull request May 22, 2017
sourced
(this fixes issue #25789:
#25789 (comment) and
the issue with git-annex mentioned here
#24314 (comment) )

(cherry picked from commit 3f91e0d)
@jwoudenberg
Copy link
Member

jwoudenberg commented Aug 12, 2017

I was hoping someone could help me with my attempt to add pass autocompletions. By way of experiment I started by overriding the current pass derivation with a preInstall attribute that copies over the fish completions.

{
  pass_with_completions = lib.overrideDerivation pass (oldAttrs: {
    preInstall = ''
      # fish completions
      mkdir -p "$out/share/fish/vendor_completions.d"
      cp "src/completion/pass.fish-completion" "$out/share/fish/vendor_completions.d/pass.fish"
    '';
  });
}

In the above I have attempted following the same approach used for adding fzf in this PR, but I must be doing something wrong because fish is not picking up on the completions on its own. I do see the completions file appearing at /nix/store/<has>-password-store-1.7/share/fish/vendor_completions.d/pass.fish and when I source that file manually the completions start working. I'm running a fully updated version of channel 17.03. Any help would be greatly appreciated!

@therealpxc
Copy link
Contributor Author

Are you on NixOS, or Nix with non-NixOS?

@jwoudenberg
Copy link
Member

jwoudenberg commented Aug 12, 2017

I'm on NixOS. I'm not using Fish as a login shell.

@therealpxc
Copy link
Contributor Author

therealpxc commented Aug 12, 2017

Ah! So this feature depends on the NixOS module for fish, which is only active when programs.fish.enable = true; is set in your configuration.nix. You don't have to use fish as your login shell, but fish support for NixOS has to be enabled for this to work, because that's what produces /etc/fish/nixos-env-preinit.fish.

The reason is that the Nix environment has to be sourced from the set-environment file which NixOS produces for each generation of the system-- it doesn't have a single, stable location; it lives in the Nix store. Part of the purpose of /etc/fish/nixos-preinit-env.fish is to have a stable path which can be sourced, as well as to contain the logic for sourcing the set-environment file (which is a sh script, thus the fenv wrapping).

However, the good news the pass package already installs its completions to the right place, so as soon as you set programs.fish.enable = true;, you'll get the completions you want:

> string replace ' ' '\n' $fish_complete_path |grep vendor_completions.d                                        
/home/pxc/.nix-profile/share/fish/vendor_completions.d
/nix/var/nix/profiles/default/share/fish/vendor_completions.d
/run/current-system/sw/share/fish/vendor_completions.d
/nix/store/n4vzxw57hp4xiff8hqh538jnfpz3zi51-fish-2.6.0/share/fish/vendor_completions.d


> ls /run/current-system/sw/share/fish/vendor_completions.d/                                                      
docker.fish  pass.fish

> pass                                                                                                            
cp                                            (Command: copy existing password) 
edit                                 (Command: edit password using text editor)
find              (Command: find a password file or directory matching pattern)
generate                                       (Command: generate new password)
…and 31 more rows

@jwoudenberg
Copy link
Member

Thank you so much @therealpxc for solving my problem and taking the time to teach me something in the process. I appreciate it!

jwoudenberg added a commit to jwoudenberg/nix that referenced this pull request Aug 13, 2017
@therealpxc
Copy link
Contributor Author

No problem at all! I'm happy to know concretely that this feature I added has a user besides me. ;-)

I don't contribute as often as I'd like to, but I am intermittently working on some more Nix-related fish goodies, so with any luck we'll see more of each other. :-)

adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
fish: fix environment clobbering and improve initialization
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
sourced
(this fixes issue NixOS#25789:
NixOS#25789 (comment) and
the issue with git-annex mentioned here
NixOS#24314 (comment) )

(cherry picked from commit 3f91e0d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.