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

Allow discovering virtual environments from the first interpreter found on the PATH #11218

Merged
merged 1 commit into from
Feb 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions crates/uv-python/src/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ pub enum PythonSource {
DiscoveredEnvironment,
/// An executable was found in the search path i.e. `PATH`
SearchPath,
/// The first executable found in the search path i.e. `PATH`
SearchPathFirst,
/// An executable was found in the Windows registry via PEP 514
Registry,
/// An executable was found in the known Microsoft Store locations
Expand Down Expand Up @@ -331,7 +333,14 @@ fn python_executables_from_installed<'a>(

let from_search_path = iter::once_with(move || {
python_executables_from_search_path(version, implementation)
.map(|path| Ok((PythonSource::SearchPath, path)))
.enumerate()
.map(|(i, path)| {
if i == 0 {
Ok((PythonSource::SearchPathFirst, path))
} else {
Ok((PythonSource::SearchPath, path))
}
})
})
.flatten();

Expand Down Expand Up @@ -1049,7 +1058,10 @@ pub(crate) fn find_python_installation(
// If the interpreter has a default executable name, e.g. `python`, and was found on the
// search path, we consider this opt-in to use it.
let has_default_executable_name = installation.interpreter.has_default_executable_name()
&& installation.source == PythonSource::SearchPath;
&& matches!(
installation.source,
PythonSource::SearchPath | PythonSource::SearchPathFirst
);

// If it's a pre-release and pre-releases aren't allowed, skip it — but store it for later
// since we'll use a pre-release if no other versions are available.
Expand Down Expand Up @@ -1601,6 +1613,7 @@ impl PythonSource {
match self {
Self::Managed | Self::Registry | Self::MicrosoftStore => false,
Self::SearchPath
| Self::SearchPathFirst
| Self::CondaPrefix
| Self::BaseCondaPrefix
| Self::ProvidedPath
Expand All @@ -1613,7 +1626,13 @@ impl PythonSource {
/// Whether an alternative Python implementation from this source can be used without opt-in.
pub(crate) fn allows_alternative_implementations(self) -> bool {
match self {
Self::Managed | Self::Registry | Self::SearchPath | Self::MicrosoftStore => false,
Self::Managed
| Self::Registry
| Self::SearchPath
// TODO(zanieb): We may want to allow this at some point, but when adding this variant
// we want compatibility with existing behavior
| Self::SearchPathFirst
| Self::MicrosoftStore => false,
Self::CondaPrefix
| Self::BaseCondaPrefix
| Self::ProvidedPath
Expand All @@ -1629,15 +1648,20 @@ impl PythonSource {
/// environment; pragmatically, that's not common and saves us from querying a bunch of system
/// interpreters for no reason. It seems dubious to consider an interpreter in the `PATH` as a
/// target virtual environment if it's not discovered through our virtual environment-specific
/// patterns.
/// patterns. Instead, we special case the first Python executable found on the `PATH` with
/// [`PythonSource::SearchPathFirst`], allowing us to check if that's a virtual environment.
/// This enables targeting the virtual environment with uv by putting its `bin/` on the `PATH`
/// without setting `VIRTUAL_ENV` — but if there's another interpreter before it we will ignore
/// it.
pub(crate) fn is_maybe_virtualenv(self) -> bool {
match self {
Self::ProvidedPath
| Self::ActiveEnvironment
| Self::DiscoveredEnvironment
| Self::CondaPrefix
| Self::BaseCondaPrefix
| Self::ParentInterpreter => true,
| Self::ParentInterpreter
| Self::SearchPathFirst => true,
Self::Managed | Self::SearchPath | Self::Registry | Self::MicrosoftStore => false,
}
}
Expand All @@ -1651,6 +1675,7 @@ impl PythonSource {
| Self::ProvidedPath
| Self::Managed
| Self::SearchPath
| Self::SearchPathFirst
| Self::Registry
| Self::MicrosoftStore => true,
Self::ActiveEnvironment | Self::DiscoveredEnvironment => false,
Expand Down Expand Up @@ -2062,6 +2087,7 @@ impl VersionRequest {
| PythonSource::DiscoveredEnvironment
| PythonSource::ActiveEnvironment => Self::Any,
PythonSource::SearchPath
| PythonSource::SearchPathFirst
| PythonSource::Registry
| PythonSource::MicrosoftStore
| PythonSource::Managed => Self::Default,
Expand Down Expand Up @@ -2473,6 +2499,7 @@ impl fmt::Display for PythonSource {
Self::CondaPrefix | Self::BaseCondaPrefix => f.write_str("conda prefix"),
Self::DiscoveredEnvironment => f.write_str("virtual environment"),
Self::SearchPath => f.write_str("search path"),
Self::SearchPathFirst => f.write_str("first executable in the search path"),
Self::Registry => f.write_str("registry"),
Self::MicrosoftStore => f.write_str("Microsoft Store"),
Self::Managed => f.write_str("managed installations"),
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-python/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ mod tests {
matches!(
interpreter,
PythonInstallation {
source: PythonSource::SearchPath,
source: PythonSource::SearchPathFirst,
interpreter: _
}
),
Expand Down Expand Up @@ -936,7 +936,7 @@ mod tests {
matches!(
python,
PythonInstallation {
source: PythonSource::SearchPath,
source: PythonSource::SearchPathFirst,
interpreter: _
}
),
Expand Down Expand Up @@ -2427,7 +2427,7 @@ mod tests {
matches!(
python,
PythonInstallation {
source: PythonSource::SearchPath,
source: PythonSource::SearchPathFirst,
interpreter: _
}
),
Expand Down Expand Up @@ -2479,7 +2479,7 @@ mod tests {
matches!(
python,
PythonInstallation {
source: PythonSource::SearchPath,
source: PythonSource::SearchPathFirst,
interpreter: _
}
),
Expand Down
40 changes: 38 additions & 2 deletions crates/uv/tests/it/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7608,12 +7608,48 @@ fn install_incompatible_python_version_interpreter_broken_in_path() -> Result<()
perms.set_mode(0o755);
fs_err::set_permissions(&python, perms)?;

// Request Python 3.12; which should fail
Copy link
Member Author

Choose a reason for hiding this comment

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

This test case regresses, undoing the work from #11143 if the interpreter is at the front of the PATH. We don't see the regression in the diff because I added a new test case below which retains the previous error message, i.e., we do the right thing if the broken interpreter is not at the front of the PATH. We can do better, but the regression this pull requests fixes seems more important to address?

// Put the broken interpreter _before_ the other interpreters in the PATH
let path = std::env::join_paths(
std::iter::once(context.bin_dir.to_path_buf())
.chain(std::env::split_paths(&context.python_path())),
)
.unwrap();

// Request Python 3.12, which should fail since the virtual environment does not have a matching
// version.
// Since the broken interpreter is at the front of the PATH, this query error should be raised
uv_snapshot!(context.filters(), context.pip_install()
.arg("-p").arg("3.12")
.arg("anyio")
// In tests, we ignore `PATH` during Python discovery so we need to add the context `bin`
.env("UV_TEST_PYTHON_PATH", path.as_os_str()), @r###"
success: false
exit_code: 2
----- stdout -----

----- stderr -----
error: Failed to inspect Python interpreter from first executable in the search path at `[BIN]/python3`
Caused by: Querying Python at `[BIN]/python3` failed with exit status exit status: 1

[stderr]
error: intentionally broken python executable
"###
);

// Put the broken interpreter _after_ the other interpreters in the PATH
let path = std::env::join_paths(
std::env::split_paths(&context.python_path())
.chain(std::iter::once(context.bin_dir.to_path_buf())),
)
.unwrap();

// Since the broken interpreter is not at the front of the PATH, the query error should not be
// raised
uv_snapshot!(context.filters(), context.pip_install()
.arg("-p").arg("3.12")
.arg("anyio")
// In tests, we ignore `PATH` during Python discovery so we need to add the context `bin`
.env("UV_TEST_PYTHON_PATH", context.bin_dir.as_os_str()), @r###"
.env("UV_TEST_PYTHON_PATH", path.as_os_str()), @r###"
success: false
exit_code: 2
----- stdout -----
Expand Down
50 changes: 50 additions & 0 deletions crates/uv/tests/it/python_find.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,56 @@ fn python_find_venv() {

----- stderr -----
"###);

// Or at the front of the PATH
#[cfg(not(windows))]
uv_snapshot!(context.filters(), context.python_find().env(EnvVars::UV_TEST_PYTHON_PATH, child_dir.join(".venv").join("bin").as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----
[TEMP_DIR]/child/.venv/[BIN]/python

----- stderr -----
"###);

// This holds even if there are other directories before it in the path, as long as they do
// not contain a Python executable
#[cfg(not(windows))]
{
let path = std::env::join_paths(&[
context.temp_dir.to_path_buf(),
child_dir.join(".venv").join("bin"),
])
.unwrap();

uv_snapshot!(context.filters(), context.python_find().env(EnvVars::UV_TEST_PYTHON_PATH, path.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----
[TEMP_DIR]/child/.venv/[BIN]/python

----- stderr -----
"###);
}

// But, if there's an executable _before_ the virtual environment — we prefer that
#[cfg(not(windows))]
{
let path = std::env::join_paths(
std::env::split_paths(&context.python_path())
.chain(std::iter::once(child_dir.join(".venv").join("bin"))),
)
.unwrap();

uv_snapshot!(context.filters(), context.python_find().env(EnvVars::UV_TEST_PYTHON_PATH, path.as_os_str()), @r###"
success: true
exit_code: 0
----- stdout -----
[PYTHON-3.11]

----- stderr -----
"###);
}
}

#[cfg(unix)]
Expand Down
Loading