Skip to content

bpo-45337: Use the realpath of the new executable when creating a venv on Windows #28663

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 7, 2021
Merged

bpo-45337: Use the realpath of the new executable when creating a venv on Windows #28663

merged 6 commits into from
Oct 7, 2021

Conversation

zooba
Copy link
Member

@zooba zooba commented Sep 30, 2021 •
edited by bedevere-bot
Loading

Also warn if the realpath does not match the user-specified location.

https://bugs.python.org/issue45337

@zooba
bpo-45337: Use the realpath of the new executable when creating a ven…
fe6aaf7 
…v on Windows.

Also warn if the realpath does not match the user-specified location.
@zooba zooba added OS-windows needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Sep 30, 2021
@zooba zooba requested a review from vsajip as a code owner September 30, 2021 22:18
vsajip
vsajip reviewed Oct 1, 2021
View reviewed changes
Lib/test/test_venv.py
@@ -150,14 +150,17 @@ def test_prompt(self):
def test_upgrade_dependencies(self):
builder = venv.EnvBuilder()
bin_path = 'Scripts' if sys.platform == 'win32' else 'bin'
python_exe = 'python.exe' if sys.platform == 'win32' else 'python'
python_exe = os.path.split(sys.executable)[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that sys.executable will be called something other than python.exe? Perhaps python3.exe? AFAIK only python.exe and pythonw.exe will be added to the venv being created.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be. python_d.exe is the only one we support upstream, but I'd like it to be viable with other names (I have some private builds with alternate executable names).

But right now it doesn't work anyway. The venv launcher executable has hardcoded names in it, so it'll only work as python.exe, python_d.exe, pythonw.exe and pythonw_d.exe. Which means that this test is basically correct, unless we one day start running our test suite with a different executable name.

@zooba zooba added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 1, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zooba for commit 49fa664 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 1, 2021
@zooba
Copy link
Member Author

zooba commented Oct 1, 2021

I triggered the buildbots mainly to test the new one we just added, this PR is still very open for updates :)

@ambv
Copy link
Contributor

ambv commented Oct 5, 2021

I see what you're trying to do here. In fact, I can't find a way to do it any better myself. That being said, I have a (minor) concern.

Your introduction of context.env_cmd while keeping context.env_exe around makes it unclear where each one should be used. Maybe better naming of the new context variable could help here. As it stands now, the reader has little chance to notice the subtle difference, so there's some danger of a future regression.

@vsajip
Copy link
Member

vsajip commented Oct 6, 2021

Your introduction of context.env_cmd while keeping context.env_exe around makes it unclear where each one should be used

Perhaps just documenting explicitly what they're for/why they're used would help, in particular if any other issues in this area come up relating to the MS-Store version of Python.

@zooba
Copy link
Member Author

zooba commented Oct 6, 2021

Perhaps just documenting explicitly what they're for/why they're used would help, in particular if any other issues in this area come up relating to the MS-Store version of Python.

I referenced the bug as an example of why we'd need this - unfortunately, this side of things keeps changing on the OS side too often, so I'm very hesitant to pretend in our source code that it's a known thing.

Also changed the member name to env_exec_cmd, which is what I had originally, but didn't like how long it was.

@zooba zooba merged commit 6811fda into python:main Oct 7, 2021
@miss-islington
Copy link
Contributor

Thanks @zooba for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-28810 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 7, 2021
@zooba
Copy link
Member Author

zooba commented Oct 7, 2021

Hopefully there were no further concerns. We've got time to fix them up if so

@bedevere-bot
Copy link

GH-28811 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Oct 7, 2021
@zooba zooba deleted the bpo-45337 branch October 7, 2021 20:26
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 7, 2021
@zooba @miss-islington
bpo-45337: Use the realpath of the new executable when creating a ven…
64fe4a8 
…v on Windows (pythonGH-28663)

(cherry picked from commit 6811fda)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 7, 2021
@zooba @miss-islington
bpo-45337: Use the realpath of the new executable when creating a ven…
742a814 
…v on Windows (pythonGH-28663)

(cherry picked from commit 6811fda)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington added a commit that referenced this pull request Oct 7, 2021
@miss-islington @zooba
bpo-45337: Use the realpath of the new executable when creating a ven…
eabca6e 
…v on Windows (GH-28663)

(cherry picked from commit 6811fda)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington added a commit that referenced this pull request Oct 7, 2021
@miss-islington @zooba
bpo-45337: Use the realpath of the new executable when creating a ven…
06935bd 
…v on Windows (GH-28663)

(cherry picked from commit 6811fda)

Co-authored-by: Steve Dower <steve.dower@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsajip vsajip vsajip left review comments

Assignees
No one assigned
Labels
OS-windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zooba @bedevere-bot @ambv @vsajip @miss-islington @the-knights-who-say-ni