allow --relocatable to work on Windows

[jpenney]

Addresses #49

[pfmoore]

Nice trick. This seems to work with the windows launcher for Python (py.exe) supplied with Python 3.3+. Presumably it also works with setuptools exe wrappers (I haven't tried that, but I assume you have...)?

Could you update the patch to remove the comment "Also this does not currently work on Windows." from the documentation? (docs/index.rst)

[jpenney]

Thanks. I'll update that doc file. I haven't yet tried it with 3.3, but I can. I don't see any mention of py.exe in virtualenv.py, am I missing something?

[pfmoore]

Nope, Python 3.3 comes with a new py.exe command which runs "the appropriate Python" (see PEP 397 for the gory details). One thing it does is interpret #! lines in Python files. I tested the format you're using and it seems that py.exe works fine with it, so that's a bonus (as I say, I assume your main interest is scripts which use the setuptools exe wrapper). The whole py.exe thing is transparent to virtualenv, so nothing is needed in there.

The main point is that your patch will handle more than just setuptools-style scripts, which is good.

[jpenney]

Ah, thank you for the clarification.

Yes, I was focusing on the setuptools exe wrappers, which don't currently work correctly with --relocatable because:

• fixup_scripts never matched windows style shebang in the first place
• the new shebang fixup_scripts was inserting (once I fixed the above) broke the exe wrappers (I fixed this using os.environ['COMSPEC'] similar to how env is used on non-windows)

I've tested this on XP (32-bit) and Win7 (32-bit and 64-bit) under Python 2.6.6 and 2.7.3 (as well as verifying that the output didn't change under a non-windows install via Mac OS X). If there's something else I should specifically verify please let me know, and if I can, I will.

[pfmoore]

Checking further, the whole relocatable feature seems to be broken on Python 3. relative_script() uses execfile, which was removed in Python 3, so the whole thing simply doesn't work. I know @carljm isn't keen on the whole idea of --relocatable, and as things stand, with the feature not working on Python 3 anyway, I'd be reluctant to merge this.

Maybe if this pull request was updated to include a fix for Python 3 it would be worth another look. Although I have to question how useful the feature is in general, if nobody has even noticed so far that it doesn't work on Python 3...

[jpenney]

Any chance that's as simple as doing this?

change:
execfile(activate_this, dict(__file__=activate_this));
to:
exec(open(activate_this).read(), dict(__file__=activate_this));

[pfmoore]

Possibly :-) I've had to jump through all sorts of hoops in the past to cover edge cases like encodings, but they are quite possibly not relevant in this case. Presumably the exec(open()) version still works on Python 2?

[jpenney]

I ran the activate line though the 2to3 tool from python 3.3.0 which changes:

execfile(activate_this, dict(__file__=activate_this))

to

exec(compile(open(activate_this).read(), activate_this, 'exec'), dict(__file__=activate_this))

While it seems the output from that should work in 2.x I did see some rumblings about 2.x not working as well with this code as using execfile directly, so I think execfile should probably still be used for 2.x.

Unfortunately it doesn't look like calling lib2to3 from python is supported, so I guess this would do:

activate = "import os; activate_this=os.path.join(os.path.dirname(os.path.realpath(__file__)), 'activate_this.py'); execfile(activate_this, dict(__file__=activate_this)); del os, activate_this"
if sys.version_info[0] > 2:
    # ran activate through 2to3 (from 3.3.0)
    activate = "import os; activate_this=os.path.join(os.path.dirname(os.path.realpath(__file__)), 'activate_this.py'); exec(compile(open(activate_this).read(), activate_this, 'exec'), dict(__file__=activate_this)); del os, activate_this"

I have tested this let me create a relocatable env under 3.3.0, which I was able to move and still call the scripts in the bin dir as expected. Does this belong in this pull request (which @pnasrat requested in #49), or should I file a separate one?

[pfmoore]

Personally, I'd rather see a single pull request that covered all of the various issues with --relocate, as it would be easier to review. @pnasrat @carljm what do you think?

[carljm]

I do think --relocatable is an ugly hack, but since we already have it, I have no a priori objection to tweaking it so it works on Python 3 and/or Windows. I haven't looked at this PR in detail or tested it, and probably won't.

[pnasrat]

Indeed if we have a feature it should work or degrade gracefully across all
supported platforms.

that being said working code can go in, even if incomplete as not all
contributors have platform access. But if we strive for better test
coverage it should be easily applicable when someone has access.

Paul
On Mar 7, 2013 6:29 PM, "Carl Meyer" notifications@github.com wrote:

I do think --relocatable is an ugly hack, but since we already have it, I
have no a priori objection to tweaking it so it works on Python 3 and/or
Windows. I haven't looked at this PR in detail or tested it, and probably
won't.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/401#issuecomment-14593469
.

[pfmoore]

OK, thanks. This looks good now, I'll do a review of the latest changes before applying it. One point is that on Windows, this relies on at least some python being on %PATH% - which isn't the case by default. I'll update the documentation with an extra note about this, though.

[pfmoore]

In order to make this testable on Python 3, can you add the execfile fix to the patch?

I made the fix manually, and I still hit issues:

1. Creating a virtualenv using python3 virtualenv.py -p python2 xxx; python3 virtualenv.py xxx --relocatable puts the wrong version of activate in because the check is on sys.version_info for the python running virtualenv, not for the python in the virtualenv being relocated. So I'd much rather see a single version that works on both python 2 and 3. Can you find out more about what you think is wrong with exec(open()) for python 2?
2. You don't need to call path_locations() in fixup_scripts(), as it was called already in make_environment_relocatable(). Just pass bin_dir as an extra argument.
3. activate_this doesn't work properly for scripts (like pip!!!) that run subprocesses using sys.executable. Consider a case where you have your system python on %PATH% and that doesn't have setuptools installed. Then do pip install virtualenv. The install fails because setuptools isn't available when pip runs [sys.executable setup.py]. If you activate the virtualenv then sys.executable is the virtualenv's python, and things are probably OK - I didn't test that (but your patch doesn't make the activate scripts relocatable, which is a separate issue).

Points 1 and 2 are issues with this patch, so should be addressed. Point 3 is more of a general example of the bugginess of relocatable virtualenvs.

If you fix points 1 and 2, I'm willing to apply the patch as it at least makes --relocatable less broken on Windows. But I'm not comfortable with the whole feature in general, as it is still broken in a lot of situations.

I am considering adding a deprecation warning to --relocatable, with the intention of removing it in a future version.

[jpenney]

I knew in order for the suggested python3 fix to work the unit tests need to be updated too.

2 is easy enough to fix. 1 and 3 already exist without this patch (fixup_scripts used sys.version elsewhere already). Still, I'll take a look and see what I can do. For 3, pip does work works if activate supports --relocatable (see #236), but I hadn't tested pip specifically without. Still, that's also a pre-existing issue, so it's not a new issue.

I have been using a version of this patch combined with #236 for over a year for what it's worth (probably not much).

I'll try and see if I can capture some of the cases encountered in some new unit tests.

[pfmoore]

The patch to address 3 seems pretty complex, from when I last looked at it, which is why I don't want to complicate this issue with it for now.

For 1, if you're not happy providing a fix, I'm willing to leave it as is, and raise it as a separate item. Depending on what we end up implementing, I'll update the docs with some extra caveats/warnings (and I'll add a note that there's a possibility that this feature will be deprecated in future).

[jpenney]

I'll look into 1 and see what, if anything, I can come up with.

[pfmoore]

Cool, thanks. As I said, I'd be happy to use exec(open()) on both Python 2 and 3, unless you can come up with a concrete reason why it doesn't work on 2. I'm pretty sure I've seen it advised for writing cross-version code before. (It has issues running scripts with arbitrary encodings, but as we control activate_this.py, that's not a problem for us.

[jpenney]

I'm pretty sure the encoding issue was the only one I saw mentioned as well, so if you feel comfortable, I'll go with that.

[pfmoore]

OK, I'll merge this then do the other bits of tidy up I mentioned as a followup.