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

Improve Python executable name discovery when using alternative implementations #7649

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Sep 23, 2024

There are two parts to this.

The first is a restructuring and refactoring. We had some debt around expected executable name generation, which we address here by consolidating into a single function that generates a combination of names. This includes a bit of extra code around free-threaded variants because this was written on top of #7431 — I'll rebase that on top of this.

The second addresses some bugs around alternative implementations. Notably, uv python list does not discovery executables with alternative implementation names. Now, we properly generate all of the executable names for VersionRequest::Any (originally implemented in #7508) to properly show all the implementations we can find:

❯ cargo run -q -- python list --no-python-downloads
cpython-3.12.6-macos-aarch64-none     /opt/homebrew/opt/python@3.12/bin/python3.12 -> ../Frameworks/Python.framework/Versions/3.12/bin/python3.12
cpython-3.11.10-macos-aarch64-none    /opt/homebrew/opt/python@3.11/bin/python3.11 -> ../Frameworks/Python.framework/Versions/3.11/bin/python3.11
cpython-3.9.6-macos-aarch64-none      /Library/Developer/CommandLineTools/usr/bin/python3 -> ../../Library/Frameworks/Python3.framework/Versions/3.9/bin/python3
pypy-3.10.14-macos-aarch64-none       /opt/homebrew/bin/pypy3 -> ../Cellar/pypy3.10/7.3.17/bin/pypy3

While doing both of these changes, I ended up changing the priority of interpreter discovery slightly. For example, given that the executables are in the same directory, do we query python or python3.10 first when you request --python 3.10? Previously, we'd check python3.10 but I think that was an incorrect optimization. I think we should always prefer the bare name (i.e. python) first. Similarly, this applies to python and an executable for an alternative implementation like pypy. If it's not compatible with the request, we'll skip it anyway. We might have to query more interpreters with this approach but it seems rare.

Closes #7286 superseding #7508

@zanieb zanieb added the internal A refactor or improvement that is not user-facing label Sep 23, 2024
@zanieb zanieb changed the title Refactore VersionRequest::executable_names Refactor VersionRequest::executable_names Sep 23, 2024
@zanieb zanieb marked this pull request as ready for review September 23, 2024 20:20
Cow::Owned(format!("{name}3{extension}")),
)
};
if let Some(prerelease) = prerelease {
Copy link
Member

Choose a reason for hiding this comment

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

Confirming my understanding: given the logic right above this, does that mean --python 3.12a0 will major 3.12.1 (stable), as an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't follow, I think there's something missing from your comment.

Copy link
Member

@charliermarsh charliermarsh Sep 23, 2024

Choose a reason for hiding this comment

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

Will --python 3.12a0 match a Python interpreter with version 3.12.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be no changes to interpreter match checks here, just the executable names we consider when querying. So yeah --python 3.12a0 will result in us querying a Python executable with the name python3.12 and python3.12.0 etc. but I don't think we will use it if its version isn't actually 3.12.0a0

@zanieb zanieb changed the title Refactor VersionRequest::executable_names Improve Python executable name discovery when using alternative implementations Sep 23, 2024
@zanieb zanieb added bug Something isn't working and removed internal A refactor or improvement that is not user-facing labels Sep 23, 2024
@charliermarsh
Copy link
Member

So if the user runs --python 3.10, we query python for its metadata, and if it's a different version, we continue checking the next interpreter, etc.? I assume this was already true, and this PR just inverts the order such that python is first, per the summary. What's the motivation for that, though? When would it be incorrect?

// e.g. `pythonrc1` and `python3rc1` don't make sense
continue;
}
names.push(name.with_prerelease(*prerelease));
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if the borrow checker will let you, but if so, it might be a bit easier to write this with an iterator (like names.extend(...)) so that it's guaranteed that it doesn't loop infinitely (despite modifying names).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I could rewrite some of these as iterators, but I fought with the borrow checker a lot in here and I'd prefer to keep it simple. for i in 0..names.len() seems guaranteed to never loop infinitely since the range is evaluated at the start.

}

pub(crate) fn long_names() -> impl Iterator<Item = &'static str> {
["cpython", "pypy", "graalpy"].into_iter()
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for splitting these up? (Is it functional?)

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, you use long_names elsewhere (per PR summary, to find pypy).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, e.g., we don't want to look for interpreters with executables named cp but cp is a valid request name when parsing so I had to separate them.

for i in 0..names.len() {
for implementation in ImplementationName::long_names() {
let name = names[i].with_name(implementation);
names.push(name);
Copy link
Member

Choose a reason for hiding this comment

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

So this adds... pypy, pypy3, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! For each implementation.

@zanieb
Copy link
Member Author

zanieb commented Sep 23, 2024

So if the user runs --python 3.10, we query python for its metadata, and if it's a different version, we continue checking the next interpreter, etc.? I assume this was already true, and this PR just inverts the order such that python is first, per the summary. What's the motivation for that, though? When would it be incorrect?

Yes, this was already true. The things that's changing here is the order of interpreters queried within a single directory. The motivation is consistency and simplicity. It's more complicated to explain and to implement if we want to change the ordering depending on the request given, especially as we add more variants of requests. Here, the ordering is consistent across all requests, we just include more names depending on the request. The motivation for having the original ordering was that it could be more performant to check python3.10 before python if Python 3.10 was requested.

As an elucidating case, if you have python which is Python 3.10.1 and python3.10 which is Python 3.10.2 and ran python in your shell you'd get 3.10.1. Generally the feedback we've gotten is that we should do our best to prioritize matching the experience of running python from the shell so I think it's more "correct" to use check python first but it's pretty ambiguous what the proper resolution order is there so I want to do what's simplest.

@zanieb
Copy link
Member Author

zanieb commented Sep 23, 2024

I'm going to merge because it's blocking some other work, but if you have any concerns following this I'm happy to talk through them.

@zanieb zanieb merged commit 0dea932 into main Sep 23, 2024
58 checks passed
@zanieb zanieb deleted the zb/executable-names-ref branch September 23, 2024 22:17
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Oct 7, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.4.15` -> `0.4.18` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.4.18`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0418)

[Compare Source](astral-sh/uv@0.4.17...0.4.18)

##### Enhancements

-   Allow multiple source entries for each package in `tool.uv.sources` ([#&#8203;7745](astral-sh/uv#7745))
-   Add `.gitignore` file to `uv build` output directory ([#&#8203;7835](astral-sh/uv#7835))
-   Disable jemalloc on FreeBSD ([#&#8203;7780](astral-sh/uv#7780))
-   Respect `PAGER` env var when paging in `uv help` command ([#&#8203;5511](astral-sh/uv#5511))
-   Support `uv run -m foo` to run a module ([#&#8203;7754](astral-sh/uv#7754))
-   Use a top-level output directory for `uv build` in workspaces ([#&#8203;7813](astral-sh/uv#7813))
-   Update `uv init --package` command to match project name ([#&#8203;7670](astral-sh/uv#7670))
-   Add a custom suggestion for `uv add dotenv` ([#&#8203;7799](astral-sh/uv#7799))
-   Add detailed errors for `tool.uv.sources` deserialization failures ([#&#8203;7823](astral-sh/uv#7823))
-   Improve error message copy for failed builds ([#&#8203;7849](astral-sh/uv#7849))
-   Use `serde-untagged` to improve some untagged enum error messages ([#&#8203;7822](astral-sh/uv#7822))
-   Use build failure hints for `dotenv` errors, rather than in `uv add` ([#&#8203;7825](astral-sh/uv#7825))

##### Configuration

-   Add `UV_NO_SYNC` environment variable ([#&#8203;7752](astral-sh/uv#7752))

##### Bug fixes

-   Accept `git+` prefix in `tool.uv.sources` ([#&#8203;7847](astral-sh/uv#7847))
-   Allow spaces in path requirements ([#&#8203;7767](astral-sh/uv#7767))
-   Avoid reusing cached downloaded binaries with `--no-binary` ([#&#8203;7772](astral-sh/uv#7772))
-   Correctly trims values during wheel WHEEL file parsing ([#&#8203;7770](astral-sh/uv#7770))
-   Fix `uv tree --invert` for platform dependencies ([#&#8203;7808](astral-sh/uv#7808))
-   Fix encoding mismatch between python child process and uv ([#&#8203;7757](astral-sh/uv#7757))
-   Reject self-dependencies in `uv add` ([#&#8203;7766](astral-sh/uv#7766))
-   Respect `tool.uv.environments` for legacy virtual workspace roots ([#&#8203;7824](astral-sh/uv#7824))
-   Retain empty extras on workspace members ([#&#8203;7762](astral-sh/uv#7762))
-   Use file stem when parsing cached wheel names ([#&#8203;7773](astral-sh/uv#7773))

##### Rust API

-   Make `FlatDistributions` public ([#&#8203;7833](astral-sh/uv#7833))

##### Documentation

-   Fix table of contents sizing ([#&#8203;7751](astral-sh/uv#7751))
-   GitLab Integration documentation ([#&#8203;6857](astral-sh/uv#6857))
-   Update documentation to setup-uv@v3 ([#&#8203;7807](astral-sh/uv#7807))
-   Use `uv publish` instead of twine in docs ([#&#8203;7837](astral-sh/uv#7837))
-   Fix typo in `projects.md` ([#&#8203;7784](astral-sh/uv#7784))

### [`v0.4.17`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0417)

[Compare Source](astral-sh/uv@0.4.16...0.4.17)

##### Enhancements

-   Add `uv build --all` to build all packages in a workspace ([#&#8203;7724](astral-sh/uv#7724))
-   Add support for `uv init --script` ([#&#8203;7565](astral-sh/uv#7565))
-   Add support for upgrading build environment for installed tools (`uv tool upgrade --python`) ([#&#8203;7605](astral-sh/uv#7605))
-   Initialize a Git repository in `uv init` ([#&#8203;5476](astral-sh/uv#5476))
-   Respect `--quiet` flag in `uv build` ([#&#8203;7674](astral-sh/uv#7674))
-   Add context message before listing available tools in `uvx` ([#&#8203;7641](astral-sh/uv#7641))

##### Bug fixes

-   Don't create Python bytecode files during interpreter discovery ([#&#8203;7707](astral-sh/uv#7707))
-   Escape glob patterns in workspace member discovery ([#&#8203;7709](astral-sh/uv#7709))
-   Avoid prefetching source distributions with unbounded lower-bound ranges ([#&#8203;7683](astral-sh/uv#7683))

##### Documentation

-   Add `uv build` and `uv publish` to features overview ([#&#8203;7716](astral-sh/uv#7716))
-   Add documentation on cache versioning ([#&#8203;7693](astral-sh/uv#7693))
-   Spell out the names of the Docker images for easier copy-paste ([#&#8203;7706](astral-sh/uv#7706))
-   Document uv-with-Jupyter workflows ([#&#8203;7625](astral-sh/uv#7625))
-   Note that `uv lock --upgrade-package` retains locked versions ([#&#8203;7694](astral-sh/uv#7694))

### [`v0.4.16`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0416)

[Compare Source](astral-sh/uv@0.4.15...0.4.16)

##### Enhancements

-   Add `uv publish` ([#&#8203;7475](astral-sh/uv#7475))
-   Add a `--project` argument to run a command from a project directory ([#&#8203;7603](astral-sh/uv#7603))
-   Display Python implementation when creating environments ([#&#8203;7652](astral-sh/uv#7652))
-   Implement trusted publishing for `uv publish` ([#&#8203;7548](astral-sh/uv#7548))
-   Respect lockfile preferences for `--with` requirements ([#&#8203;7627](astral-sh/uv#7627))
-   Unhide the `--directory` option ([#&#8203;7653](astral-sh/uv#7653))
-   Allow requesting free-threaded Python interpreters ([#&#8203;7431](astral-sh/uv#7431))
-   Show a dedicated PubGrub hint for `--unsafe-best-match` ([#&#8203;7645](astral-sh/uv#7645))
-   Add resolver error checking for conflicting distributions ([#&#8203;7595](astral-sh/uv#7595))

##### Bug fixes

-   Avoid adding double-newlines for CRLF ([#&#8203;7640](astral-sh/uv#7640))
-   Avoid retaining forks when `requires-python` range changes ([#&#8203;7624](astral-sh/uv#7624))
-   Determine if pre-release Python downloads should be allowed using the version specifiers ([#&#8203;7638](astral-sh/uv#7638))
-   Fix `link-mode=clone` for directories on Linux ([#&#8203;7620](astral-sh/uv#7620))
-   Improve Python executable name discovery when using alternative implementations ([#&#8203;7649](astral-sh/uv#7649))
-   Require opt-in to use alternative Python implementations ([#&#8203;7650](astral-sh/uv#7650))
-   Use the first pre-release discovered when only pre-release Python versions are available ([#&#8203;7666](astral-sh/uv#7666))

##### Documentation

-   Document environment variable that disables printing of virtual environment name in prompt ([#&#8203;7648](astral-sh/uv#7648))
-   Remove double whitespaces from the code ([#&#8203;7623](astral-sh/uv#7623))
-   Use anchorlinks rather than permalinks ([#&#8203;7626](astral-sh/uv#7626))

##### Preview features

-   Add build backend scaffolding ([#&#8203;7662](astral-sh/uv#7662))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv python list doesn't find pypy
2 participants