More in general (doesn't have to be addressed in this PR), it seems like most of these sed invocations don't do anything special and could be replaced by substituteInPlace.

The included completions still use global awk:

$ grep -r " awk" results/fish/share/fish/completions/ | tail -n3
results/fish/share/fish/completions/heroku.fish:    heroku domains | awk '{if (NR>1) print $1}'
results/fish/share/fish/completions/heroku.fish:    heroku ps | awk -F':' '{if (NR>1) print $1}'
results/fish/share/fish/completions/heroku.fish:    heroku releases | awk '{if (NR>1) print $1}'

good catch, thanks!

Actually all of this patching feels a bit brittle. Maybe we could add a test of sorts, that runs all of the functions and completions through fish -n in an isolated environment, checking that they parse successfully? That should hopefully cover any referenced commands that we don't know of, at least those that are statically known.

I had some success with this script:

#!/usr/bin/env nix-shell
#! nix-shell -p fish -p findutils -p coreutils -i bash
fish_path=$(nix eval --raw '(with import <nixpkgs> {}; (lib.makeBinPath [ fish ]))')

fakebin=$(mktemp -d)

find result/share/fish/functions -type f -name '*.fish' -print0 | while read -d $'\0' f
do
  env PATH="$fish_path" fish -n "$f" && echo "file $f: ok" || echo "file $f: failure"
done

find result/share/fish/completions -type f -name '*.fish' -print0 | while read -d $'\0' f
do
  cmd=$(basename -s '.fish' "$f")
  touch "$fakebin/$cmd"
  chmod a+x "$fakebin/$cmd"
  env PATH="$fish_path:$fakebin" fish -n "$f" && echo "file $f: ok" || echo "file $f: failure"
  rm "$fakebin/"*
done

It also produces some false positives, mostly due to cross references of the functions / completions. Still it looks like we are overlooking quite a few references: uname ls dircolors seq sh type tail perl cut uniq

Ok, just notices that all of these references are from coreutils, except for perl which is only needed by the completion for ack (which in turn has perl as a runtime dependency, so I guess that should be fine).
Maybe we want to assume people have coreutils in their PATH when running fish (could also be enforced by the fish module on nixos)?

Edit: or put coreutils into fish's PATH by wrapping.

I'm against wrapping fish's PATH. What if a user has a great aversion to coreutils for whatever reason, but is happy with busybox? They'd then have to either overlay fish, or suck it up. I'm sure there are other situations where one would not want coreutils on their PATH. There was a time when I wanted to just wrap fish with every package it references, but that then taints the user's environment with binaries they might not want or need (not to mention having a super long PATH that could one day reach PATH_MAX). It's inelegant.

Even if we add coreutils to the fish module's package list, what about people on non-NixOS who don't have access to NixOS modules? We can't enforce this this way.

The current situation of fish isn't ideal, but wrapping is not the solution. My plan is to, someday in the future, hack up a bash script similar to what you have above that generates a patch that can be run through substituteAll, which would allow us to remove all of the nasty seding. Maybe a first step would be doing this by hand, and attempting to script it later.

I'm against wrapping fish's PATH.

Well it could be done in such a way that any externally specified binaries are preferred (by postfixing). Also the current state is, that we patch in cut from coreutils, so it will be in the closure anyway (even if its not used for the other binaries). But yes, that also has some drawbacks.

Even if we add coreutils to the fish module's package list, what about people on non-NixOS who don't have access to NixOS modules? We can't enforce this this way.

They would use whatever their distro / setup gives them. I agree its not a perfect solution either.

The current situation of fish isn't ideal, but wrapping is not the solution. My plan is to, someday in the future, hack up a bash script similar to what you have above that generates a patch that can be run through substituteAll, which would allow us to remove all of the nasty seding. Maybe a first step would be doing this by hand, and attempting to script it later.

As far as automating it goes, I am totally in favor of that = )

Still I think should go with sed-ing for things like awk for the time being. Its not even in coreutils and somewhat rarely used these days, so it may very well be missing on some systems..

I'm not worried about it being in the closure, but I am worried about it tainting PATH (if we start wrapping every binary from every completion or function, PATH would grow to the point that we could conceivably reach PATH_MAX).

I think the current approach (with sed) is the way to go until I get around to my substituteAll plan.

@cole-h is this ready to merge?

Yep. I just forgot to re-approve. Sorry!

Yep. I just forgot to re-approve. Sorry!

Cool, thanks!

I gave the patching issue some more thought. I think a sane thing to do would be to wrap fish to put coreutils and friends in PATH, but make the list easily configurable (say as an argument given to the fish derivation / package).
Would that be acceptable to you @cole-h ?

I also had the same idea once upon a time (modulo the configurable list) but after a conversation with adisbladis and emily on IRC, came to the conclusion that that would be undesirable.

Logs start here: https://logs.nix.samueldr.com/nixos/2020-04-29#3388524.