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

Fix Fish shell assignments on array env variables #2169

Merged
merged 2 commits into from Jun 3, 2015

Conversation

hugomg
Copy link

@hugomg hugomg commented May 29, 2015

  • Correctly handle the case of the variable being unset
    • This fixes the "missing manpages" bug.
  • Do not put quotes around array expansions:
    set -gx PATH $PATH "/blah" # Good
    set -gx PATH "$PATH" "/blah" # Bad
  • Add logic for CDPATH, for future-proofing
    • It is the only other special array variable we have
      to potentially worry about.

* Correctly handle the case of the variable being unset
  - This fixes the "missing manpages" bug.

* Do not put quotes around array expansions:
    set -gx PATH  $PATH  "/blah"  # Good
    set -gx PATH "$PATH" "/blah"  # Bad

* Add logic for CDPATH, for future-proofing
  - It is the only other special array variable we have
    to potentially worry about.
@hugomg
Copy link
Author

hugomg commented May 30, 2015

Hmm, just noticed that there are also assignments on "/src/client/opamConfigCommand.ml". Dunno what would be the best way to proceed without duplicating a lot of code.

* Use "source" instead of "."

* Handle CDPATH in `opam config env`
@hugomg
Copy link
Author

hugomg commented May 31, 2015

I couldn't fully understand why there is separate logic for generating the shell scripts that go on variables.fish and on opam config env. Right now, the code that generates variables.fish can handle assignments with variable expansions on the RHS (things like PATH="$PATH:/blah") and the code for opam config env does NOT and assumes that the directory names in the path lists are specified in full.

If this difference between the two functions is not a problem then I think this set of patches should fix #2165

@AltGr
Copy link
Member

AltGr commented Jun 1, 2015

Great, thanks!
opam config env is different because it can afford to be a bit more clever: it expands $PATH, detects previous instances of opam paths tied to the same opam root and replaces them. And re-sets the variable in full.
variables.fish is much simpler, and is normally loaded only once at shell init, so expanding directly over $PATH using shell expansion is fine. The fish issue had already been (mostly?) fixed for opam config env, but not the variables.fish variant, it seems.

@avsm
Copy link
Member

avsm commented Jun 1, 2015

This also seems like a 1.2.3 point fix candidate?

AltGr added a commit that referenced this pull request Jun 3, 2015
Fix Fish shell assignments on array env variables
@AltGr AltGr merged commit df51212 into ocaml:master Jun 3, 2015
@hugomg hugomg deleted the fish-variables-PR branch June 12, 2015 15:28
@johshoff
Copy link

I'd like to support @avsm's suggestion to make a point release. This breaks the configuration of fish shells when installing the latest release, 1.2.2 (released a year ago, so I'm assuming this is what most package managers have).

@cartazio
Copy link

when is this being merged? @avsm ?
seems like its still biting folks

@avsm
Copy link
Member

avsm commented Jan 31, 2018

It's included in the forthcoming opam 2.0 release, which has just been tagged as rc1

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.

None yet

5 participants