Skip to content

Fix PATH-based Python discovery on Windows#2712

Merged
gaborbernat merged 5 commits into
pypa:mainfrom
ofek:windows-executables
Apr 27, 2024
Merged

Fix PATH-based Python discovery on Windows#2712
gaborbernat merged 5 commits into
pypa:mainfrom
ofek:windows-executables

Conversation

@ofek

@ofek ofek commented Apr 27, 2024

Copy link
Copy Markdown
Contributor

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

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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

ofek commented Apr 27, 2024

Copy link
Copy Markdown
Contributor Author

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

gaborbernat commented Apr 27, 2024

Copy link
Copy Markdown
Contributor

Sure 👍

@ofek

ofek commented Apr 27, 2024

Copy link
Copy Markdown
Contributor Author

Drop 3.7?

@gaborbernat

gaborbernat commented Apr 27, 2024

Copy link
Copy Markdown
Contributor

Keep 3.7.

gaborbernat
gaborbernat requested changes Apr 27, 2024
View reviewed changes
Comment thread .github/workflows/check.yml
@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

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread src/virtualenv/info.py


def fs_path_id(path: str) -> str:
return path.casefold() if fs_is_case_sensitive() else path

@ofek ofek Apr 27, 2024

Copy link
Copy Markdown
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

ofek commented Apr 27, 2024

Copy link
Copy Markdown
Contributor Author

Any idea why that job timed out after 20 minutes?

@ofek

ofek commented Apr 27, 2024

Copy link
Copy Markdown
Contributor Author

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
@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