Skip to content

Refactor find_uv_bin and add a better error message#14182

Merged
zanieb merged 1 commit intomainfrom
zb/ref-find
Aug 7, 2025
Merged

Refactor find_uv_bin and add a better error message#14182
zanieb merged 1 commit intomainfrom
zb/ref-find

Conversation

@zanieb
Copy link
Copy Markdown
Member

@zanieb zanieb commented Jun 21, 2025

Follows #14181

Two goals here

  • Remove duplicated logic and make the search order clear
  • Resolve user confusion around the searched directories; we previously only displayed the last attempt, which we rarely expect to be relevant

@zanieb zanieb added the enhancement New feature or improvement to existing functionality label Jun 21, 2025
@zanieb zanieb temporarily deployed to uv-test-registries June 21, 2025 12:00 — with GitHub Actions Inactive
@zanieb zanieb requested a review from konstin June 27, 2025 17:46
@zanieb zanieb marked this pull request as ready for review June 27, 2025 17:46
Copy link
Copy Markdown
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

I'm reviewing the combined changes in #14184

@zanieb zanieb force-pushed the zb/base-prefix-find branch from db356ab to 6956f7e Compare July 18, 2025 12:59
@zanieb zanieb force-pushed the zb/base-prefix-find branch from 6956f7e to 88146e8 Compare August 6, 2025 15:41
@zanieb zanieb temporarily deployed to uv-test-registries August 6, 2025 15:47 — with GitHub Actions Inactive
@zanieb zanieb force-pushed the zb/base-prefix-find branch 3 times, most recently from 37cc3c0 to ef649ba Compare August 7, 2025 03:59
zanieb added a commit that referenced this pull request Aug 7, 2025
Adds test cases to unblock

- #14181
- #14182
- #14184
- #14184
- tox-dev/pre-commit-uv#70

We use a package with a symlink to the Python module to get a mock
installation of uv without building (or packaging) the uv binary. This
lets us test real patterns like `uv pip install --prefix` without
encoding logic about where things are placed during those installs.

---------

Co-authored-by: konstin <konstin@mailbox.org>
@zanieb zanieb force-pushed the zb/base-prefix-find branch from ef649ba to 82d52db Compare August 7, 2025 12:14
@zanieb zanieb temporarily deployed to uv-test-registries August 7, 2025 12:43 — with GitHub Actions Inactive
@zanieb zanieb force-pushed the zb/base-prefix-find branch from 82d52db to 7d5595f Compare August 7, 2025 13:11
@zanieb zanieb force-pushed the zb/ref-find branch 2 times, most recently from e875b9d to 16ce3d6 Compare August 7, 2025 13:44
@zanieb zanieb temporarily deployed to uv-test-registries August 7, 2025 13:54 — with GitHub Actions Inactive
@zanieb zanieb temporarily deployed to uv-test-registries August 7, 2025 18:10 — with GitHub Actions Inactive
Base automatically changed from zb/base-prefix-find to main August 7, 2025 18:40
@zanieb zanieb force-pushed the zb/ref-find branch 2 times, most recently from 31fc441 to 2bd9a39 Compare August 7, 2025 18:41
@zanieb zanieb temporarily deployed to uv-test-registries August 7, 2025 18:43 — with GitHub Actions Inactive
Comment on lines +603 to +624
// Add filtering for the bin directory of the base interpreter path
let bin_dir = if cfg!(windows) {
// On Windows, the Python executable is in the root, not the bin directory
executable
.canonicalize()
.unwrap()
.parent()
.unwrap()
.join("Scripts")
} else {
executable
.canonicalize()
.unwrap()
.parent()
.unwrap()
.to_path_buf()
};
filters.extend(
Self::path_patterns(bin_dir)
.into_iter()
.map(|pattern| (pattern.to_string(), format!("[PYTHON-BIN-{version}]"))),
);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is new, the rest is just moved up to ensure it comes first in the filter order.

@zanieb zanieb merged commit b1a036c into main Aug 7, 2025
93 checks passed
@zanieb zanieb deleted the zb/ref-find branch August 7, 2025 20:10
zanieb added a commit that referenced this pull request Aug 7, 2025
#14184)

Follows #14182

Adds support for the case described at
#10194 (comment)

This also happens to fix both `--with` requirement test cases, which
should close tox-dev/pre-commit-uv#70
raise FileNotFoundError(path)
raise UvNotFound(
f"Could not find the uv binary in any of the following locations:\n"
f"{os.linesep.join(f' - {target}' for target in seen)}\n"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are we using both os.linesep and \n in the same message?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We shouldn't be, thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Confused this isn't rendered in the snapshots?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, that's just a trailing newline. This was just easier than writing a literal \n inside the f-string? Do you want me to use linesep everywhere?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would just use \n everywhere.

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.

3 participants