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

Always store _OLD_VIRTUAL_PATH in a sh compat format #904

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ViViDboarder
Copy link

ViViDboarder commented May 22, 2016

If in a subshell from within fish, the PATH variable will be converted to the proper format for sh, but none of the custom path-like variables will. This causes errors because it sets the PATH to an invalid value when activating.

This patch makes activate.fish always store the old path using : deliminators and then converts it back to fish format later.

Issue reproduction

Simplest example:

PATH converts correctly

fish $ echo $PATH
  /bin /usr/local/bin
fish $ bash -c 'echo $PATH'
  /bin:/usr/local/bin

OLD_PATH does not

fish $ set -x OLD_PATH $PATH
fish $ echo $OLD_PATH
  /bin /usr/local/bin
fish $ bash -c 'echo $OLD_PATH'
  /bin/usr/local/bin

There you will see the OLD_PATH is not a valid path for sh.


This was migrated from #827 to reparent it to the master branch. Please see original pull request for any previous discussion.

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Apr 12, 2018

@dstufft any chance someone can review this?

@gaborbernat gaborbernat force-pushed the pypa:master branch 6 times, most recently from 2b3f876 to 9dfcb43 Oct 22, 2018

@gaborbernat
Copy link
Contributor

gaborbernat left a comment

can you please add some tests to validate this, thanks!

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Dec 16, 2018

@gaborbernat are there existing tests I can add to? I can't find any fish tests here, maybe I'm overlooking something.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 16, 2018

No fish tests yet (there are powershell/xonosh ones from which you can draw inspiration) would need to set up fish into the CI as part of this as such.

ViViDboarder added some commits Feb 26, 2016

Always store _OLD_VIRTUAL_PATH in a sh compat format
When using a subshell from fish while within a virtualenv the
_OLD_VIRTUAL_PATH variable ends up in fish format and not the format of
the subshell. This breaks the first step of the activate script where it
attepts to deactivate the previous environment.

This patch will handle conversion to and from sh and fish formats within
the activate.fish script

@ViViDboarder ViViDboarder force-pushed the ViViDboarder:src-is-valid-src-again-rebase branch from bfc6ad1 to 4eae680 Dec 17, 2018

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Dec 17, 2018

Updated and added a test.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 17, 2018

@ViViDboarder please add a changelog for this, not sure why the CI did not trigger here.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 17, 2018

ViViDboarder added some commits Dec 17, 2018

Fix lint
Remove line that didn't lint as we don't need it.
@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 17, 2018

run tox -e fix_lint to fix the final failure 😁

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Dec 17, 2018

phew Done.

Installed the hooks too so that won't happen again.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 17, 2018

There's a bug at the moment which causes shell tests not available to be silently skipped in CI. I'll need to fix this tomorrow, but this could bring up some platform incompatibilities, and we probably need to provision fish for the CI. Only after if tests pass we're done. Leave it with me for now, unless you want to add fish provisioning into the CI.

@gaborbernat gaborbernat self-assigned this Dec 17, 2018

@gaborbernat gaborbernat added the tests label Dec 17, 2018

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Dec 17, 2018

Ah. I thought that might need to be added somewhere.

FWIW, it passes for me locally because I have fish installed. I'm not sure where to add it to your build configuration since I've never worked with Azure. If you let me know where to make the change, I'm happy to do that. Otherwise I can leave it to you.

Thanks for the feedback!

@ViViDboarder ViViDboarder force-pushed the ViViDboarder:src-is-valid-src-again-rebase branch from 679d39e to 67bc2a1 Dec 18, 2018

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Dec 18, 2018

Got it!

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 18, 2018

Hmm, seems you did not mark the PR as edit-able. Will close and reopen from another branch.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Dec 18, 2018

See #1258.

@gaborbernat gaborbernat added this to the 16.2 milestone Dec 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment