fish unconditionally display manpath when fish version is 2.7 or later

[gregory-nisbet]

Hello, I would like feedback on this change and advice on how to write a test for it. I briefly looked through the test directory and saw some scaffolding for "fake packages", but could not figure out what the right way to set up a "fake switch" is for testing purposes.

My system opam packaged by Manjaro (version 2.0.1, sha256sum 2b76b39ed8d53923b81985b618d4ef13fca1eabb7110bb477c7f9a3e4ae01767) does not produce a fish shell fragment for the MANPATH environment variable, causing opam to erroneously report that the environment is out of sync.

As a workaround, I have been using the following fish function. setenv is a valid fish command and the quoting strategy used by OpamConfigEnv.print_csh_env seems to work for fish if the paths contain sensible characters.

function opam_env
  eval (opam config env --shell=csh)
end

I removed a conditional and changed it so that OpamConfigEnv.print_fish_env always prints the MANPATH, since none of the other print_*_env functions appear to have dedicated logic for the MANPATH environment variable.

I tested it by evaling fragments generated by the development version of opam and checking for the presence or absence of warnings.

~/.../opam> setenv MANPATH '/'
Exit 1

~/.../opam> opam switch
#   switch   compiler                    description
    4.05.0   ocaml-base-compiler.4.05.0  4.05.0
->  default  ocaml-system.4.07.1         default

[WARNING] The environment is not in sync with the current switch.
          You should run: eval (opam env)

~/.../opam> eval (~/.../opam/opam/_build/install/default/bin/opam config env --shell=fish)

~/.../opam  > opam switch
#   switch   compiler                    description
    4.05.0   ocaml-base-compiler.4.05.0  4.05.0
->  default  ocaml-system.4.07.1         default

No tests failed as a result of this change. At least one test probably should have failed, though.

Thanks in advance,
Greg

[rjbou]

Thanks for the proposal!

This special case for MANPATH has been introducing because otherwise it broke MANPATH with old fish version, cf. #3005 and especially this comment.
I don't know if fish 2.6 is yet largely used, from what I see, it is still the default on debian stable.

On your proposal, changes will bring back to errored MANPATH setting (cf. 74f8bbd). Is there another way in fish to set MANPATH to avoid opam not up to date message and that don't break old fish versions?

No tests failed as a result of this change. At least one test probably should have failed, though.

opam have an included test suite that tests only main operation (install, pin, update, upgrade, etc.). Shell features are not tested automatically, only via manual testing.

[rjbou]

Related to #3671

[gregory-nisbet]

@rjbou

I changed it to use an awk fragment to check the fish version.
fish does not appear to have the ability to manipulate strings very well by itself. Please let me know if another solution (such as using sh) would be better than using awk.

[dra27]

I’m not a fish user, but a few observations:

• It looks like $FISH_VERSION might have been removed in v2.7?
• It also looks as though the format of the version can include some other stuff (e.g. dev branch names) though not clear if that matters
• awk feels like using a pile driver to crack a nut - what about fish 2.3’s string command? I don’t know how you detect the absence of a command in fish but the absence of string obviously implies <2.6!
• calling awk is portably awkward on Cygwin, but the intersection of native-Windows Cygwin-fish using OCaml developers I expect to be very small!

[gregory-nisbet]

Hi everyone,

I looked at the fish changelog here: https://fishshell.com/release_notes.html and decided to test for the presence of the disown command (introduced in 2.7) using string match introduced in 2.3.

The command in structured in such a way that fish versions prior to 2.3 will attempt to execute a nonexistent string command and exit abnormally so the MANPATH won't be set.

Unfortunately, I can't redirect the stderr output of failing to execute a nonexistent command to /dev/null, for example:

> builtin qqch 2>/dev/null 
fish: Unknown builtin “qqch”
builtin qqch 2>/dev/null
        ^

> builtin qqch 1>/dev/null 2>/dev/null
fish: Unknown builtin “qqch”
builtin qqch 1>/dev/null 2>/dev/null
        ^

This means that old fishes (prior to 2.3) will get stray output on stderr, but it shouldn't affect the functionality. I don't know a good way to fix it besides redirecting stderr to /dev/null inside the fish fragment which seems weird and gross.

[gregory-nisbet]

@rjbou @dra27 please let me know if there's anything else I should do.

[rjbou]

Thanks for the changes!
From release notes, disown builtin command has been added in 2.6.0, as MANPATH issue fixed in 2.7.0, maybe use another new builtin command to check, as argparse?
Also, a proposal to not have fish 2.3.0 users have the error string message: use grep instead of string match. On my 2.4.0 fish (untested on other versions)

~> builtin -n | grep argparse > /dev/null 2>&1
~> echo $status
1
~> builtin -n | grep string > /dev/null 2>&1
~> echo $status
0

Having error messages (from shell) is an unwanted behaviour, even if it is sometimes just cosmetic, but in this case, when shell hook is enabled opam env would print an error message at each command prompt.
Note that I'm not a fish user :) maybe some other command than argparse or grep suits better, but it needs to not produce any superfluous messages.

[gregory-nisbet]

@rjbou All right. We should be ready to go. I went with argparse and grep, but wrapped grep in a /bin/sh so it doesn't produce any stray output if grep doesn't exist.

[rjbou]

Thanks! It's a go for me!