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

Use sys._base_executable #1345

Merged
merged 12 commits into from Apr 23, 2019

Conversation

Projects
None yet
3 participants
@pfmoore
Copy link
Member

commented Apr 15, 2019

This avoids problems with Python 3.7.3+ venv redirector on Windows. See #1339 for discussion of the issue.

pfmoore added some commits Apr 15, 2019

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

We may need to give up on Python 3.7.2, I suspect - if the test output is correct, it has the redirector that causes this issue, but no sys._base_executable. @zooba, is that correct? BTW, this illustrates the issue with semantic changes in point releases - it's an utter PITA to test this, as I need to keep installing different point releases (you can't have 3.7.1, 3.7.2 and 3.7.3 simultaneously installed).

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Tests seem OK except for Windows 3.7, which is 3.7.2, see above.

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

The CI link seems to have disappeared from this PR. @gaborbernat do you know why that might have happened? I'm going to try closing and re-opening to see if that triggers something.

@pfmoore pfmoore closed this Apr 16, 2019

@pfmoore pfmoore reopened this Apr 16, 2019

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Yeah, close and re-open sorted the checks issue. Weird.

@gaborbernat the only thing I can think of to do with the 3.7.2 issue is to skip the test if sys.version is 3.7.2, and accept that it's a core Python issue that affects that specific version only. Are you OK with that? Or do you have any suggestion of how I could handle that case better?

@gaborbernat
Copy link
Contributor

left a comment

Would be nice to have some link to the documentation of this semi-private thing.

@@ -1459,6 +1459,10 @@ def install_python(home_dir, lib_dir, inc_dir, bin_dir, site_packages, clear, sy
if sys.executable != py_executable:
# FIXME: could I just hard link?
executable = sys.executable
# If sys.executable is a redirector (for example, on Windows)
# get the real exe to copy using the semi-private sys._base_executable attribute)
if hasattr(sys, "_base_executable"):

This comment has been minimized.

Copy link
@gaborbernat

gaborbernat Apr 16, 2019

Contributor
executable = getattr(sys,  "_base_executable", executable)

Simpler?

This comment has been minimized.

Copy link
@pfmoore

pfmoore Apr 16, 2019

Author Member

Yeah - or even just executable = getattr(sys, "_base_executable", sys.executable) :-)

This comment has been minimized.

Copy link
@pfmoore

pfmoore Apr 16, 2019

Author Member

Regarding docs to the "semi-private thing", I don't think there are any :-( I'm going off what @zooba said over on #1339 - well, that plus the fact that there's no other way of getting the information, so private or not we don't have much choice other than to use this attribute.

I agree, though, that it would be nice if this attribute was documented.

This comment has been minimized.

Copy link
@gaborbernat

gaborbernat Apr 16, 2019

Contributor

Seems this is Python 3.7+ only. Link it to python/cpython#11745 as what implemented it.

This comment has been minimized.

Copy link
@gaborbernat

gaborbernat Apr 16, 2019

Contributor

Seems to be a site.py feature, maybe we should backport it to our site.py to make it a more general feature.

This comment has been minimized.

Copy link
@gaborbernat

gaborbernat Apr 16, 2019

Contributor

Sounds good to me 👍 Thanks!

This comment has been minimized.

Copy link
@zooba

zooba Apr 16, 2019

Contributor

Just FYI, the reason it's _base_executable is because it was added in 3.7.3 and isn't available in earlier versions. For 3.8, we could promote it to base_executable.

It should behave the same on macOS and Windows, right? Both platforms have scenarios where you can't just copy python.exe around and expect it to work, which is why the redirectors exist on both.

This comment has been minimized.

Copy link
@pfmoore

pfmoore Apr 16, 2019

Author Member

It doesn't seem to be working the same in my tests (the OSX test was failing with what looked like an infinite recursion of running the same exe) but as I don't have access to an OSX machine, I can't do much other than fire speculative changes at CI. For now, I'm ignoring OSX, I'll come back to it later.

Do you know who is responsible for the redirector on OSX (I assume you are the person for the Windows one)? While it's low-level technical detail, I really think that this should be better documented, as at the moment it's a minefield for anyone trying to go beyond the "end user" level. I'm happy to write some docs, but I'd need to discuss the details with the people who know the reasoning behind what's going on.

This comment has been minimized.

Copy link
@zooba

zooba Apr 16, 2019

Contributor

I assume it's @ned-deily, or at least he can pass the blame along to someone else.

Unfortunately, I think if all this had been properly documented, it would simply be preventing us from fixing fatal bugs in released versions, so I'm not so keen to lock in the implementation as a public interface. It's an end user level tool, and so far only virtualenv has any need to go beyond this that I'm aware of.

This comment has been minimized.

Copy link
@pfmoore

pfmoore Apr 16, 2019

Author Member

only virtualenv has any need to go beyond this that I'm aware of

Well, certainly if virtualenv can solve the "creating one virtual environment from within another" issue, then that fixes a big chunk of the problems. But there are a lot of tools out there that for better or worse introspect the layout of virtual environments1, and that means that either the implementation just gets locked in in an ad-hoc fashion2 or we need to expose/document APIs that we can control.

For now, though, I'm going to concentrate on trying to get running virtualenv from a venv working on Windows again3 and leave the wider picture for another day.


1 For example, a lot of code relies on the Python binary being Scripts/python.exe on Windows and bin/python on Unix.
2 Assuming no-one is willing to break a load of longstanding code on the basis of "it's not documented so even though we didn't give you access to information you needed, it's your fault".
3 And probably slightly improved, with sys.real_prefix pointing at the original interpreter rather than the intermediate venv.

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Just FYI, the reason it's _base_executable is because it was added in 3.7.3

Sorry, I should have been clearer, I understand the reason for not exposing it in 3.7.3, I'm less comfortable with the switch to redirectors in a point release, and then not reverting that change when it was found to cause issues. But I saw the bpo discussions go past and didn't comment on them at the time, so that's as much on me as anyone. It's just a shame that leaves us with issues like pipxproject/pipx#123 and #1339 here.

If I can get a working approach on this PR, at least for Windows (AFAIK, there was no change during 3.7.x for other platforms, so any issues there isn't new), then I'm happy that the immediate problem is dealt with (even if it has to be done by classing 3.7.2 as not fixable).

BTW, does anyone know how to find out the timescale for Azure updating their Python on Windows to 3.7.3? I'd rather not spend ages trying to get CI working here using 3.7.2 if it's going to be switching to 3.7.3 before the change gets merged (without CI, I don't see that we can reasonably support special casing for 3.7.2, conversely I'd rather not merge a 3.7.3-only fix if we don't have CI for it).

@gaborbernat

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

I'll make sure it passes on Mac if you get a Windows working.

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

I think I might have found why Mac was breaking - there's another undocumented variable __PYVENV_LAUNCHER__ that affects the core interpreter, it looks like it may force it to redirect back to the venv executable, which causes an infinite loop.

Just testing a fix now, and if it works I'll tidy everything up and add some documentation (regardless of what @zooba thinks, this needs writing up somewhere to save the next poor soul who has to work out what's going on, so if it's not going to be documented in the core, we should document it here, if only as "this is what we found out and are relying on").

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

I think this is now good to go. I've just added some docs.

@gaborbernat @zooba could you review the code and see if there are any improvements that could be made? The tests seem to work for non-Windows platforms, I think that's because the existing support is fine for cases where there's no redirector.

@gaborbernat gaborbernat self-assigned this Apr 20, 2019

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@gaborbernat Do you want to merge this? I can do it, but I wasn't sure I should as you'd assigned it to yourself.

@gaborbernat gaborbernat merged commit 2c1e45a into pypa:master Apr 23, 2019

1 check passed

pypa.virtualenv #pypa.virtualenv_20190417.05 succeeded
Details

@pfmoore pfmoore deleted the pfmoore:fix_1339 branch Apr 23, 2019

@pfmoore

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Excellent, thanks!

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.