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 unconditionally display manpath when fish version is 2.7 or later #3728

Merged
merged 3 commits into from Mar 12, 2019

Conversation

Projects
None yet
4 participants
@gregory-nisbet
Copy link
Contributor

commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 25, 2019

Related to #3671

@gregory-nisbet gregory-nisbet force-pushed the gregory-nisbet:fish-manpath branch 2 times, most recently from 1dec7a2 to 5415de4 Jan 26, 2019

@gregory-nisbet

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

@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.

@gregory-nisbet gregory-nisbet force-pushed the gregory-nisbet:fish-manpath branch from 5415de4 to f6ac3c5 Jan 26, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

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 gregory-nisbet changed the title fish unconditionally display manpath fish unconditionally display manpath when fish version is 2.7 or later Feb 23, 2019

@gregory-nisbet

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

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

@rjbou

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2019

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.

@rjbou rjbou added this to the 2.1.0 milestone Feb 28, 2019

@gregory-nisbet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

Thanks! It's a go for me!

@AltGr AltGr merged commit 71464d4 into ocaml:master Mar 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rjbou rjbou referenced this pull request Mar 27, 2019

Merged

2.0.4 backported commits #3805

@rjbou rjbou modified the milestones: 2.1.0, 2.0.4 Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.