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.

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

Fix PATH-based Python discovery on Windows #2712

Merged
merged 5 commits into from
Apr 27, 2024
Merged

Fix PATH-based Python discovery on Windows #2712

merged 5 commits into from
Apr 27, 2024

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Apr 27, 2024

This PR:

  • Resolves Latest release no longer respects PATH on Windows #2711
  • Addresses another issue found during debugging which is that the PR that introduced the regression took out a check to make sure that executables exist before trying to gather data about the interpreter
  • Added an optimization so that the entire discovery process prevents duplicate interpreter calls rather than just the PATH fallback at the end

gaborbernat
gaborbernat requested changes Apr 27, 2024
View reviewed changes
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.

Tests please.

@ofek ofek force-pushed the windows-executables branch from ba38caa to 6910339 Compare April 27, 2024 02:21
@ofek
Copy link
Contributor Author

ofek commented Apr 27, 2024

FYI macos-latest is having a staggered rollout to ARM and it looks like it happened here. Would you like me to downgrade the runner or drop 3.7 finally now?

@gaborbernat
Copy link
Contributor

Sure 👍

@ofek
Copy link
Contributor Author

ofek commented Apr 27, 2024

Drop 3.7?

@gaborbernat
Copy link
Contributor

Keep 3.7.

@ofek ofek force-pushed the windows-executables branch from 62f0e67 to 34eaeac Compare April 27, 2024 15:18
gaborbernat
gaborbernat requested changes Apr 27, 2024
View reviewed changes
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.

Looks good but the CI is falling .

ofek
ofek commented Apr 27, 2024
View reviewed changes
src/virtualenv/info.py
@@ -48,6 +48,10 @@ def fs_supports_symlink():
return _CAN_SYMLINK


def fs_path_id(path: str) -> str:
return path.casefold() if fs_is_case_sensitive() else path
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fine but FYI in future if absolutely necessary the more proper and less performant way would be to stat the file and use the tuple: (stat.st_dev, stat.st_ino)

@ofek
Copy link
Contributor Author

ofek commented Apr 27, 2024

Any idea why that job timed out after 20 minutes?

@ofek
Copy link
Contributor Author

ofek commented Apr 27, 2024

oh thanks for restarting that!

@ofek ofek mentioned this pull request Apr 27, 2024
Closed
@gaborbernat gaborbernat merged commit cbbf465 into pypa:main Apr 27, 2024
47 checks passed
@ofek ofek deleted the windows-executables branch April 27, 2024 18:49
@ofek ofek mentioned this pull request Aug 8, 2024
Closed
@musicinmybrain musicinmybrain mentioned this pull request Aug 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaborbernat gaborbernat gaborbernat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Latest release no longer respects PATH on Windows
2 participants
@ofek @gaborbernat