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

[guahki]

• I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Only use the logic introduced in #1168 to fix a path resolution issue on windows when the given path already is absolute. This avoids prepending the cwd to relative paths.
This fixes #1205.

Test plan

Tested by running the following command on a windows machine with python3.12.exe in PATH (which errored before):

pipx run --python=python3.12.exe pycowsay moo

[Gitznik]

IMO we should not merge this before someone was able to replicate this still fixes the path redirection issue.

Also please fix the failing tests and add tests covering this new logic.

[dechamps]

IMO we should not merge this before someone was able to replicate this still fixes the path redirection issue.

I just tested 02f5fe2 myself and can confirm there is no regression - #1164 is still fixed on that commit.

[Gitznik]

IMO we should not merge this before someone was able to replicate this still fixes the path redirection issue.

I just tested 02f5fe2 myself and can confirm there is no regression - #1164 is still fixed on that commit.

Thanks for verifying!

[gaborbernat]

You need to make the CI pass before we do any more review.

[guahki]

I dived into the source code and had a totally new idea how/where to fix the original issue: I think it is quite logical to resolve the root path when calculating the venv paths. Small caveat is, that the shared lib paths have to be recomputed after creating the lib, as the first computation might no correctly resolve the redirected paths (I think, due to the fact, that the class is instantiated at import time and not even the pipx folder might exist at this early time).

I added comments in the interesting places.

@Gitznik what do you think about this?

@dechamps I tested this with your issue #1164 and for the tests I did, it worked. But of course I invite you to heavily test this commit and share your findings here!

PS: I know, that some tests might be failing. But I wanted to first get your feedback, before I spend time fixing the tests for nothing.

[Gitznik]

I think this only works on a subset of the problem. In pipx/venv.py line 157, we would still use the provided python version, not the one that is resolved via get_venv_paths. Can you test if this still works if you explicitly pass a python version that is subject to path redirection via the --python argument? @guahki

[guahki]

Can you test if this still works if you explicitly pass a python version that is subject to path redirection via the --python argument?

I tried to test this and did not manage to find any issue. Using a MS Store Python (via PATH lookup or direct path) in --python did work for me.

I tried at least

• pipx run --python python3.12.exe pycowsay moo
• pipx run --python C:\Users\<snip>\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.12_qbz5n2kfra8p0\python.exe pycowsay moo

I think this only works on a subset of the problem. In pipx/venv.py line 157, we would still use the provided python version, not the one that is resolved via get_venv_paths.

I'm not 100% sure, how this whole path redirection magic works for MS Store Apps. But as far as I understand, the line you quote is no problem, as the python executable loaded here is aware of it's path redirection magic and handles that correctly by itself, independent of us (pipx) giving it a helping hand.

As far as I understand (and also the hypothesis by @dechamps expressed in #1164 (comment)), the issue arises only when calling a non-MS-Store executable living inside the redirected path by it's "unredirected" name. Then this executable thinks, it lives in (and searches for other files relative to) the unredirected path, when in reality it should use the redirected one. And this executable (due to not being part of a MS Store bundle) does not transparently redirect the paths on OS level.

[guahki]

However, as discussed in #1203 (comment), using different Python versions than the one used by pipx itself can not work at all with MS Store Python and the location of pipx's venv folder inside AppData\Local

[Gitznik]

As far as I understand (and also the hypothesis by @dechamps expressed in #1164 (comment)), the issue arises only when calling a non-MS-Store executable living inside the redirected path by it's "unredirected" name. Then this executable thinks, it lives in (and searches for other files relative to) the unredirected path, when in reality it should use the redirected one. And this executable (due to not being part of a MS Store bundle) does not transparently redirect the paths on OS level.

Thanks for double checking. In that case I think this is the preferable fix as it only resolves the path exactly where the issue occurs.

However, as discussed in #1203 (comment), using different Python versions than the one used by pipx itself can not work at all with MS Store Python and the location of pipx's venv folder inside AppData\Local

So this only fixes passing the python version that is already used anyways by default as it's the one used by pipx?

[guahki]

So this only fixes passing the python version that is already used anyways by default as it's the one used by pipx?

Yes and no.

Yes: in the "new" directory placement (version 1.3.0 and newer) it is inherently impossible to use another python version. I think I will open another issue about this. Nevertheless, I could give this version as full path as well as just --python python3.12.exe which does a PATH lookup.

And no: when a legacy (pre 1.3.0) directory is present, I could use any of my installed python versions (MS Store Python as well as non-MS Store one) by lookup in the PATH as well as giving the full path. The only thing I could not test quickly, was using the py-launcher (as I do not use it on this system), but the change here should obviously not create any issues with it (and your new pipx/interpreter:find_python_interpreter handled this quite nice).

[Gitznik]

In that case I think this is a good fix. Please fix the failing test and then mark ready for review 👍 I think the thing tested there is no longer relevant because you removed the realpath logic from the run_subprocess function.

Also please add a test to get_venv_path that verifies the results. I don't think we can actually fully reproduce the original error there as it has too many assumptions on how python is installed on the runner, but maybe you can just create a file in a directory that is subject to path redirection and check if it resolves correctly? (actually maybe we can just pretty much copy this example from the python docs into a test)

[dechamps]

maybe you can just create a file in a directory that is subject to path redirection and check if it resolves correctly?

I am not aware of any way to manually set up this kind of "path redirection" mechanism on Windows outside of an MS Store app. Back when I wrote #1168 I tried to write a regression test in the form of a GitHub Actions workflow that installed Python from the MS Store, but I hit a wall in the form of Cyberboss/install-winget#1. It would sure be nice to catch MS Store Python regressions automatically, but it's not clear how that can be done.

[Gitznik]

I am not aware of any way to manually set up this kind of "path redirection" mechanism on Windows outside of an MS Store app.

So the linked example from the python docs only applies if executed via a MS store python version?

[dechamps]

As far as I'm aware, yes.

[Gitznik]

As reproducing the issue in a test seems unlikely, please fix the failing CI and then this is ready for review.

The discussion on moving away from using AppDir by default on windows lives at #1247

[guahki]

please fix the failing CI and then this is ready for review

I fixed the CI by just deleting the two test functions, testing the path resolution in the run_subprocess function. As discussed, I do not know, what (or better how) to test these changes in a better way.

Thanks for all your help, moderation and understanding @Gitznik!

[Gitznik]

Looks good to me. Thanks for all the investigation @guahki

stale