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

Pass real paths when running subprocesses #1168

Merged
merged 3 commits into from
Dec 28, 2023
Merged

Conversation

dechamps
Copy link
Contributor

@dechamps dechamps commented Dec 24, 2023

Summary of changes

Fixes #1164 by making sure real paths are passed to executables that are not subject to path redirection.

Test plan

winget install python --source=msstore
pip install .
python -m pipx ensurepath
pipx install pycowsay
pycowsay

The above fails without this PR with the error shown in #1164, but succeeds with this PR.

I tried to add a GitHub Actions job that does the above so that any future regressions will be caught, but unfortunately installing Microsoft Store apps in a GitHub Actions workflow does not appear to be easy (see e.g. Cyberboss/install-winget#1).

uranusjr
uranusjr previously approved these changes Dec 25, 2023
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Seems harmless enough.

@dechamps
Copy link
Contributor Author

I looked at the test failure and it turns out that my PR in its current state breaks every OS except Windows 😅 One thing I did not realize is that, on every OS except Windows, the python binary in the venv is actually a symlink, and Python expects argv[0] to contain the path to the symlink (not the real path) so that it can resolve the venv. My PR results in pipx resolving the venv symlink manually and calling python with the real path in argv[0]. This results in Python being called without any venv, which is obviously wrong.

This is a bit of a catch-22: on Windows using MS Store Python, argv[0] cannot be the redirected path (that doesn't work), but on *nix, it has to be the redirected path (i.e. the symlink).

I'll see if I can come up with a fix that doesn't assume too much about the particular implementation details of venvs on each platform.

@dechamps
Copy link
Contributor Author

Okay I managed to make it work on every OS by only using the real path if the executable is not a symlink. The basic idea is that if the target is a symlink then the target might care about argv[0] being the symlink, so we pass the symlink untouched. If, however, it is not a symlink, then using the real path is a no-op, except on MS Store Python, where it will fix argv[0] so that the venv's Scripts/python.exe receives its real path and is able to locate its own venv. (Phew!)

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Test please and changelog entry.

src/pipx/util.py Outdated Show resolved Hide resolved
@dechamps
Copy link
Contributor Author

dechamps commented Dec 28, 2023

@gaborbernat

Test please

These code paths are already covered, and both branches are taken in GitHub Actions workflows (the non-symlink branch is taken on Windows, and the symlink branch is taken on every other OS).

If you want me to add a regression test for #1164, well, I tried to do that, but sadly it's not as easy as it seems because I was unable to get MS Store Python to install in a GitHub Actions Windows runner (see Cyberboss/install-winget#1 for one approach I tried).

changelog entry

Done.

src/pipx/util.py Outdated Show resolved Hide resolved
This gets rid of the following warning when using the Microsoft Store
version of Python:

  The system cannot find the path specified.
  Failed to delete C:\Users\etien\AppData\Local\pipx\pipx\trash. You may need to delete it manually.

See also pypa#1164
This code dates all the way back to
7cb6561. It's not clear why this code
decides to spawn an rmdir process on Windows instead of simply calling
`shutil.rmtree()` directly.
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat enabled auto-merge (squash) December 28, 2023 16:07
@gaborbernat gaborbernat merged commit 81c2fa5 into pypa:main Dec 28, 2023
14 checks passed
dechamps added a commit to dechamps/videojitter that referenced this pull request Dec 28, 2023
This reverts commit 9e62270.

The pipx issue was fixed in pypa/pipx#1168 and the fix made it into pipx
1.4.0.
guahki pushed a commit to guahki/pipx that referenced this pull request Feb 2, 2024
by moving the whole os.path.realpath logic introduced in pypa#1168 to
pipx.util:get_venv_paths
guahki pushed a commit to guahki/pipx that referenced this pull request Feb 2, 2024
by moving the whole os.path.realpath logic introduced in pypa#1168 to
pipx.util:get_venv_paths
guahki pushed a commit to guahki/pipx that referenced this pull request Feb 10, 2024
by moving the whole os.path.realpath logic introduced in pypa#1168 to
pipx.util:get_venv_paths
Gitznik added a commit that referenced this pull request Feb 19, 2024
…ws (#1232)

* Fix path resolution for python executables looked up in PATH on windows

by moving the whole os.path.realpath logic introduced in #1168 to
pipx.util:get_venv_paths

* Fix failing tests by deleting no longer relevant tests

---------

Co-authored-by: guahki <[email protected]>
Co-authored-by: Robert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pipx unable to create venv when using Python from Microsoft Store
3 participants