fish: 2.2.0 -> 2.3.0

[rnhmjoj]

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
  • OS X
  • 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.

Things to do

• Run tests. Fish seems to work as a login shell however there may still be something to be patched. Are there tests to run to make sure everything is fine?
• Fix warnings at startup. The PATH is populated with non existing directories and fish is complaining:
  
  PATH is being set here config.system.build.setEnvironment. Why are all these variable here in the first place? kde4, gtk, qt, firefox, infinality are not installed.

Notes

I removed shellAliases because status is a fish builtin command and replacing it with a custom definition is really a bad idea. Also previously start=systemctl start broke one of my script (zsh) and it took me a while to figure that out. In my opinion these aliases should be an example, not a default value.
EDIT: I submitted another PR #15598 to fix the issue. Fish will build but it won't work properly until the other changes are merged.

I also removed old patches that are not needed anymore.

[mention-bot]

By analyzing the blame information on this pull request, we identified @jgillich, @nathan7 and @mstarzyk to be potential reviewers

[jgillich]

I agree that we should get rid of the systemctl aliases, but this is definitely not the correct solution, config.environment.shellAliases should apply to all shells. Please create a new issue or PR for that.

[edef1c]

👍 on splitting out the alias changes — while I'd like them to go too, they don't belong here in a package update PR, and belong in the wider discussion of #15598.
As a more package update relevant concern, but not a very immediate one, we might want to check on hidden deps like the | tr and such I patched up recently, especially when we upgrade to newer versions of fish.

[vcunat]

The alias issue should be gone now. The conflict is probably just because of me replacing man_db by man-db. It seems to work fine for me, though I don't have it set as login shell.

[layus]

I have done some work in parallel, so here are the bits that you could use.
Pcre2 is now updated to work with fish. You can use it as soon as #15672 lands.

 { stdenv, fetchurl, ncurses, nettools, python, which, groff, gettext, man-db,
-  bc, libiconv, coreutils, gnused, kbd, utillinux, glibc }:
+  bc, libiconv, coreutils, gnused, kbd, utillinux, glibc, pcre2 }:

# ...

-  buildInputs = [ ncurses libiconv ];
+  configureFlags = [ "--without-included-pcre2" ];
+
+  buildInputs = [ ncurses libiconv pcre2 ];

We have also hit an issue with TERM{,INFO{,_DIRS}} variables, as fish cannot (and does not want to) update itself once it has started its interactive mode. We therefore need to export these vars soon enough for fish to catch them, or wait for some fix upstream. See #15384 and fish-shell/fish-shell#3060

[rnhmjoj]

@vcunat Yes, that was it. I've been using it for a couple days and found no issues so far. The warnings also do not appear on my system.
EDIT: It seem the path is verified the first time it's sourced, when the existing environment it's passed to the shell it's not; so the warning only appear in a login shell.
@layus Thanks, I've added your changes.

[rnhmjoj]

The environment is initialized here: programs/environment.nix. These variables should be set only if the respective package is installed.

[layus]

Is there no way to silence fish warnings? It is not because it produces warnings that it is right to do so. Setting env vars to nonexistent dirs is perfectly valid, and covers cases where the dirs are not yet mounted, or provisionned later.
In the case of /home/fish/.nix-profile/sbin for example, it makes sense to define the path even if not yet available, because a nix-env -i later during the execution of the system may install executables there.

I really think we should fix or work around fish here, not NixOS modules.

[vcunat]

Adding 2>/dev/null silences the warnings. We might add it, as I don't see what (other) useful error could set PATH produce.

[astsmtl]

Let's suppress this check for now.

[rnhmjoj]

Using make test you can run fish_tests, however it seem most of the are failing due to purity issues.

[layus]

I am working on the tests. Most errors are caused by the assumption on executable presence and location. (Missing xxd & ul commands, /bin/echo not found, etc) and locale. I think I see the end of it. Hang tight :-).

[layus]

Here is how to fix the tests on linux

diff --git a/pkgs/shells/fish/default.nix b/pkgs/shells/fish/default.nix
index 6dd8d52..0061a04 100644
--- a/pkgs/shells/fish/default.nix
+++ b/pkgs/shells/fish/default.nix
@@ -1,7 +1,7 @@
 { stdenv, fetchurl, coreutils, utillinux,
   nettools, kbd, bc, which, gnused, gnugrep,
   nettools, kbd, bc, which, gnused, gnugrep,
   groff, man-db, glibc, libiconv, pcre2,
-  gettext, ncurses, python
+  gettext, ncurses, python, vim, glibcLocales
 }:

 with stdenv.lib;
@@ -27,6 +27,37 @@ stdenv.mkDerivation rec {
     python which groff gettext
   ] ++ optional (!stdenv.isDarwin) man-db;

+  postPatch = ''
+    # /bin/{echo,ls} do not exist, but /bin/sh will
+    # These executables are tested for their executable bit, never executed
+    sed -e "s!/bin/\(echo\|ls\)!/bin/sh!" -i src/fish_tests.cpp tests/test6.in
+
+    # Really use our wicked PATH.
+    sed -i '/highlight_shell/ s/env_vars_snapshot_t(/&env_vars_snapshot_t::highlighting_keys/' src/fish_tests.cpp
+
+    sed -e 's|xxd |${vim}/bin/xxd |' -i tests/*.in
+
+    # Do not assume `-e /bin/echo`.
+    sed -e 's|/bin/echo |command echo |' -i tests/test1.in
+
+  '' + optionalString stdenv.isLinux ''
+    sed -e "s| ul| ${utillinux}/bin/ul|" \
+        -i "share/functions/__fish_print_help.fish"
+    for cur in share/functions/*.fish; do
+      sed -e "s|/usr/bin/getent|${glibc.bin}/bin/getent|" -i "$cur"
+    done
+  '';
+
+  doCheck = true;
+  TERM = "xterm";
+  TERMINFO = "${ncurses}/share/terminfo";
+  LC_ALL = "en_US.UTF-8";
+  LOCALE_ARCHIVE = stdenv.lib.optionalString stdenv.isLinux
+    "${glibcLocales}/lib/locale/locale-archive";
+  checkPhase = ''
+    make test HOME=$TMPDIR
+  '';
+
   postInstall = ''
     sed -r "s|command grep|command ${gnugrep}/bin/grep|" \
         -i "$out/share/fish/functions/grep.fish"
@@ -54,14 +88,6 @@ stdenv.mkDerivation rec {
     sed -e "s|clear;|${ncurses.out}/bin/clear;|" \
         -i "$out/share/fish/functions/fish_default_key_bindings.fish" \

-  '' + optionalString stdenv.isLinux ''
-    sed -e "s| ul| ${utillinux}/bin/ul|" \
-        -i "$out/share/fish/functions/__fish_print_help.fish"
-    for cur in $out/share/fish/functions/*.fish; do
-      sed -e "s|/usr/bin/getent|${glibc.bin}/bin/getent|" \
-          -i "$cur"
-    done
-
   '' + optionalString (!stdenv.isDarwin) ''
     sed -i "s|(hostname\||(${nettools}/bin/hostname\||"           \
            "$out/share/fish/functions/fish_prompt.fish"

[rnhmjoj]

@layus Unfortunately it's still failing for me:

Error: Test failed on line 1941: is_potential_path(L"/usr", wds, PATH_REQUIRE_DIR)

[layus]

It's now pushed on https://github.com/layus/nixpkgs/commits/fix-strip-tests. Nothing special, just to be sure that we are speaking of the exact same code.
Did you use a chroot build ?

It is difficult to do remote debug for these issues. Doing it locally is already a pain.

[rnhmjoj]

I have only applied your patch. Yes, I did, I also tried building a vm.

[layus]

Let's skip the tests for this release then, it is really tough work (took me many hours already), and it seems unreliable. I have just tested on my remote server, and the build tests fails with error messages I supposedly fixed in this patch.

[rnhmjoj]

Ok, it's not really that important to have tests: my main concern was leaving something unpatched but I couldn't find any other issue so far. I think this can be merged now.