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

Xonsh Support #1206

Merged
merged 25 commits into from Nov 6, 2018

Conversation

Projects
None yet
4 participants
@scopatz
Copy link
Contributor

scopatz commented Sep 28, 2018

This aims to add xonsh support to virtualenv.

I am not sure how to generate the top-level ACTIVATE_XSH variable in virtualenv.py, and this probably needs to be tested, but otherwise, should be good to go!

Feedback welcome!

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Sep 28, 2018

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Sep 28, 2018

And feel free to push directly to this branch.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Sep 28, 2018

Tests?

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Sep 28, 2018

@gaborbernat - any recommendations on how to add tests?

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Sep 28, 2018

@gaborbernat - also I know this will fail because of the ACTIVATE_XSH issue.

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Sep 28, 2018

This seems like the kind of thing where xonsh needs to be added to the matrix in Travis (i.e. all tests should be run using xonsh as the shell), though I don't know if tox will handle that as expected..

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Sep 28, 2018

Let me have a better look at things and I'll get back to you in a few weeks with some proper advice.

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Sep 29, 2018

I'm not sure we should add activate scripts for yet more shells. It's already hard enough to find people with sufficient expertise to maintain feature parity across all the shells we do support (sh, csh, fish, cmd, powershell).

Rather than this, would anyone be interested in a PR to allow users to locally customise the activation scripts installed in a new virtualenv? I'd be willing to write such a PR if it meant that we focused on only supporting a limited number of shells (and basic functionality for them) in the supplied scripts, leaving users with different needs to use the customisation option to handle them.

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Sep 29, 2018

@pfmoore I think there's a pretty strong case for including xonsh in the list of supported shells considering the nature of xonsh as a "shell but with Python". Not to volunteer him or anything, but I think that given that @scopatz is a xonsh maintainer and an active participant in the Python community, the question of finding a domain expert is somewhat moot in this case.

That said, I do agree that it doesn't necessarily scale to have to maintain support for every shell in the upstream package, so providing some clear hooks for "shell support plugins" (and possibly breaking out all current shell support into individual plugin packages maintained by domain experts) seems like a net win.

If the timeline for "provide hooks to allow a 'xonsh support' plugin" is on the order of a few months, then I think it probably makes sense to wait. If the timeline is on the order of a year or more, I think it makes sense in the short term to add xonsh as a supported shell and then possibly use it as a test case for moving support from within virtualenv to a separate package.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Sep 29, 2018

I think let's add this now. In parallel we can then afterwards come up with an extensible API people can use to add their own, and then we can deprecate all this.

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Sep 29, 2018

If the timeline for "provide hooks to allow a 'xonsh support' plugin" is on the order of a few months,

I have an approach in mind, so it's more like a matter of a week or two. However, I'm not thinking of anything elaborate like plugin support (at least not what I imagine by that) but rather just something to let users add custom scripts of their own. I wasn't proposing anything that would allow an "official" xonsh plugin that could be distributed centrally and would link into the user's virtualenv installation. (That's not to say that isn't something that someone could develop, just it's not what I was proposing).

the question of finding a domain expert is somewhat moot

My point was more that issues like #1166 and #1168 would be stuck needing to get input (and probably code) from all the different shell experts, otherwise we'd end up with different behaviour (which is the issue #1168 is flagging). That makes it harder to move on any of these types of issue (I'm generally against the approach of "let's fix the ones I know how to, and leave users of other environments to raise separate PRs to cover the rest").

I think let's add this now. In parallel we can then afterwards come up with an extensible API

I'm OK with that, I don't see the maintenance burden issue as a crisis, just something we should consider when making the decision.

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Oct 2, 2018

Hi All, thanks for your feedback. I am very happy to keep maintaining this (and not likely to go anywhere, as @pganssle points out 😉) I just need to know what to do here to get this to work.

@@ -1452,6 +1452,7 @@ def install_activate(home_dir, bin_dir, prompt=None):
'activate.bat': ACTIVATE_BAT,
'deactivate.bat': DEACTIVATE_BAT,
'activate.ps1': ACTIVATE_PS,
'activate.xsh': ACTIVATE_XSH,

This comment has been minimized.

@pganssle

pganssle Oct 18, 2018

Member

I think ACTIVATE_XSH needs to actually be defined, though apparently all the others are defined here, which is some crazy nonsense I don't understand.

This comment has been minimized.

@pfmoore

pfmoore Oct 18, 2018

Member

The ACTIVATE_xxx variables hold base64-encoded copies of the actual activate scripts. They are embedded in the virtualenv.py script so that we don't need to ship them as separate files (the virtualenv_embedded directory isn't shipped as part of the wheel).

bin/rebuild-script.py rebuilds virtualenv.py from the files in the virtualenv_embedded directory. Some fiddling will be needed to add ACTIVATE_XSH to that mechanism (but I don't know precisely what).

This comment has been minimized.

@scopatz

scopatz Oct 19, 2018

Contributor

@pganssle - you mentioned you had this working somewhere?

This comment has been minimized.

@pganssle

pganssle Oct 19, 2018

Member

@scopatz I made a PR against your branch here

pganssle and others added some commits Oct 18, 2018

Merge pull request #1 from pganssle/xonsh_activate
Add base64-encoded xonsh script
@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 19, 2018

I'm not seeing any shell-specific tests in the test suite. How are these intended to be tested?

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Oct 19, 2018

Yeah, I was just looking at that. I can certainly translate test_activate.sh to xonsh if you would like.

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

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Oct 31, 2018

I have updated this, resolving conflicts from master

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Oct 31, 2018

thanks can you add a test similar to what we have for powerscript for activation?

@gaborbernat
Copy link
Contributor

gaborbernat left a comment

Also, run tox -e embed,fix_lint when you've made the changes to automatically update embedding and formatting. Add tests as showcased in https://github.com/pypa/virtualenv/blob/master/tests/activation/test_powershell_activation.py

Show resolved Hide resolved src/virtualenv.py Outdated
Show resolved Hide resolved virtualenv_embedded/activate.xsh Outdated
@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Nov 2, 2018

Could be that on Windows the Path is not set, maybe try to make it first pass by having absolute path by re-using sys.executable.

scopatz added some commits Nov 6, 2018

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Nov 6, 2018

I think that this is finally all green and ready to go!

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Nov 6, 2018

@scopatz just one more thing, let's add a changelog entry announcing it 🎉

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Nov 6, 2018

@gaborbernat, sure thing! Where should I make those edits at? docs/changes.rst?

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Nov 6, 2018

@schopatz yes

Show resolved Hide resolved setup.py Outdated
Show resolved Hide resolved setup.py Outdated

scopatz added some commits Nov 6, 2018

Show resolved Hide resolved src/virtualenv.py Outdated
def test_activate_with_xonsh(tmpdir, monkeypatch):
monkeypatch.chdir(tmpdir)
home_dir, _, __, bin_dir = virtualenv.path_locations(str(tmpdir.join("env")))
virtualenv.create_environment(home_dir, no_pip=True, no_setuptools=True, no_wheel=True)

This comment has been minimized.

@gaborbernat

gaborbernat Nov 6, 2018

Contributor

how is this test passing under python 2 🤔 if xonosh cannot run with it

This comment has been minimized.

@scopatz

scopatz Nov 6, 2018

Contributor

The test seems to be appropriately skipped on linux, windows, and mac on Python 2.7. Are you seeing something different?

This comment has been minimized.

@gaborbernat

gaborbernat Nov 6, 2018

Contributor

Oh I understand, it's not installed on python 2.7 so it's skipped. This works by chance as https://github.com/pypa/virtualenv/blob/master/tests/lib/__init__.py#L14 should mean we never skip inside the CI 😮 please add some logic inside the needs_xonosh to always skip if xonosh is not installed.

This comment has been minimized.

@scopatz

scopatz Nov 6, 2018

Contributor

Ok! Does it look good now?

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Nov 6, 2018

The azure failure is an report_coverage Job failure, which is unrelated to anything I have done here, I believe.

scopatz added some commits Nov 6, 2018

@gaborbernat gaborbernat merged commit 3fbfeb7 into pypa:master Nov 6, 2018

@scopatz

This comment has been minimized.

Copy link
Contributor

scopatz commented Nov 6, 2018

Woohoo! Thanks for all the help here!

@scopatz scopatz deleted the scopatz:xonsh branch Nov 6, 2018

@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