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

Fix python path resolution on linux #1196

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

Gitznik
Copy link
Contributor

@Gitznik Gitznik commented Jan 8, 2024

  • I have added an entry to docs/changelog.md

Summary of changes

Only use the logic introduced in #1168 to fix a path resolution issue on windows when pipx is running on a Windows machine.
This fixes #1195

Test plan

When no venv for python3.9 exists, but python is on the PATH (in my case via pyenv):

pipx run --python=python3.9 pycowsay moo

Before this change leads to the error described in #1195 , and works after this change.

Tested by running

pipx run --python=python3.9 pycowsay moo

@robinbowes
Copy link

robinbowes commented Jan 9, 2024

Would love to see this merged & released!

Copy link

@robinbowes robinbowes left a comment

Choose a reason for hiding this comment

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

LGTM

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 merge conflict.

@gaborbernat gaborbernat marked this pull request as draft January 10, 2024 20:54
@Gitznik Gitznik marked this pull request as ready for review January 11, 2024 17:54
@Gitznik Gitznik requested a review from gaborbernat January 11, 2024 17:54
@Gitznik
Copy link
Contributor Author

Gitznik commented Jan 11, 2024

Test please and merge conflict.

I added some tests. I could not come up with a good way of reproducing this exact issue in a test suite without requiring multiple python versions to be installed on the runner, which I think would not be great for local execution. Let me know if you have other ideas @gaborbernat.

BTW we could set os.path.realpath to fail if it can't resolve the path (at the moment it does not fail, but instead creates the invalid path on unix based systems), but I was unsure if that would break a lot of things on windows.

@gaborbernat gaborbernat merged commit 2441503 into pypa:main Jan 12, 2024
14 checks passed
hswong3i pushed a commit to alvistack/pypa-pipx that referenced this pull request Jan 13, 2024
@ewjoachim
Copy link
Member

@gaborbernat thanks for merging ! In this message, @Gitznik mentionned that they were expecting to change one line, but I think you merged before they had an opportunity to do that (well, the tool was broken, so it's a good decision whatsoever).

Do we want to make a follow-up PR with the suggested simplification, or drop it altogether?

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.

Failing to create venv when passing --python version on linux
5 participants