Skip to content

Commit

Permalink
Apply system Python filtering to executable name requests (#4309)
Browse files Browse the repository at this point in the history
Executable name requests were being treated as explicit requests to
install into system environments, but I don't think it should be as it's
implicit what environment you'll end up in. Following #4308, we allow
multiple executables to be found so we can filter here.

Concretely, this means `--system` is required to install into a system
environment discovered with e.g. `--python=python`. The flag is still
not required for cases where we're not mutating environment.
  • Loading branch information
zanieb committed Aug 19, 2024
1 parent bf59be2 commit 84b382f
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 5 deletions.
14 changes: 9 additions & 5 deletions crates/uv-python/src/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -717,11 +717,15 @@ pub fn find_python_installations<'a>(
if preference.allows(PythonSource::SearchPath) {
debug!("Checking for Python interpreter at {request}");
Box::new(
python_interpreters_with_executable_name(name, cache).map(|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
}),
python_interpreters_with_executable_name(name, cache)
.filter(move |result| {
result_satisfies_environment_preference(result, environments)
})
.map(|result| {
result
.map(PythonInstallation::from_tuple)
.map(FindPythonResult::Ok)
}),
)
} else {
Box::new(std::iter::once(Err(Error::SourceNotAllowed(
Expand Down
42 changes: 42 additions & 0 deletions crates/uv-python/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1609,6 +1609,48 @@ mod tests {
"We should find the `bar` executable"
);

// With [`EnvironmentPreference::OnlyVirtual`], we should not allow the interpreter
let result = context.run(|| {
find_python_installation(
&PythonRequest::parse("bar"),
EnvironmentPreference::ExplicitSystem,
PythonPreference::OnlySystem,
&context.cache,
)
})?;
assert!(
matches!(result, Err(PythonNotFound { .. })),
"We should not allow a system interpreter; got {result:?}"
);

// Unless it's a virtual environment interpreter
let mut context = TestContext::new()?;
let python = context.tempdir.child("foo").join("bar");
TestContext::create_mock_interpreter(
&python,
&PythonVersion::from_str("3.10.0").unwrap(),
ImplementationName::default(),
false, // Not a system interpreter
)?;
context.add_to_search_path(context.tempdir.child("foo").to_path_buf());

let python = context
.run(|| {
find_python_installation(
&PythonRequest::parse("bar"),
EnvironmentPreference::ExplicitSystem,
PythonPreference::OnlySystem,
&context.cache,
)
})
.unwrap()
.unwrap();
assert_eq!(
python.interpreter().python_full_version().to_string(),
"3.10.0",
"We should find the `bar` executable"
);

Ok(())
}

Expand Down

0 comments on commit 84b382f

Please sign in to comment.