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

Search for all python3.x in PATH #5148

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Search for all python3.x in PATH #5148

merged 3 commits into from
Jul 18, 2024

Conversation

konstin
Copy link
Member

@konstin konstin commented Jul 17, 2024

Search for all python3.x minor versions in PATH, skipping those we already know we can use.

For example, let's say python and python3 are Python 3.10. When a user requests >= 3.11, we still need to find a python3.12 in PATH. We do so with a regex matcher.

Fixes #4709

Search for all `python3.x` minor versions, skipping those we already know we can use.

For example. let's say `python` and `python3` are Python 3.10. When a user requests `>= 3.11`, we still need to find a `python3.12` in PATH.

Fixes #4709
@konstin konstin added the enhancement New feature or improvement to existing functionality label Jul 17, 2024
@konstin konstin requested a review from zanieb July 17, 2024 15:51
// Filter out interpreter we already know have a too low minor version.
let minor = path
.file_name()
.and_then(|filename| filename.to_str())
Copy link
Member Author

Choose a reason for hiding this comment

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

It's silly we can't use capture groups for this but the which api is too locked down for that

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should just not use which and we should iterate over the executables in the directory manually. A regex feels heavier than it needs to be. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an implementation i used to have (without which it's much better than the current one), but it failed because which doesn't expose Checker so we'd have to vendor that code from them to check whether we can use a specific file.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with vendoring that code if you are.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added is_executable

@zanieb
Copy link
Member

zanieb commented Jul 17, 2024

Thanks for tackling this one!

@zanieb
Copy link
Member

zanieb commented Jul 18, 2024

Should we be using a regex at all now? Or can we just use some simple string parsing?

@konstin
Copy link
Member Author

konstin commented Jul 18, 2024

I find the regex easier to follow along

@zanieb
Copy link
Member

zanieb commented Jul 18, 2024

I worry a bit about performance since this isn't included in any benchmarks, but trust your intuition on it.

@konstin
Copy link
Member Author

konstin commented Jul 18, 2024

I'm confident that the regex is fast. Andrew has done great performance work there :)

@zanieb
Copy link
Member

zanieb commented Jul 18, 2024

@BurntSushi notorious for writing slow code hehe

@konstin konstin merged commit 7beae77 into main Jul 18, 2024
52 checks passed
@konstin konstin deleted the konsti/search-for-any-minor branch July 18, 2024 15:00
@BurntSushi
Copy link
Member

In terms of perf, this is also including regex compilation, which is definitely not known for being fast. This might actually be a case where regex-lite could be faster overall. Another potential problem is if the function being added here is called more than once for a particular set of inputs. Then you're rebuilding the regex each time.

I don't mean to suggest anything should change though. I wouldn't be surprised if the perf difference was marginal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv sync fails when interpreter is not named python[3]
3 participants