Validate instead of discovering python patch version (#1266)

Contrary to our prior assumption, we can't reliably select a specific
patch version. With the deadsnakes PPA for example, `python3.12` is
installed into `PATH`, but `python3.12.1` isn't. Based on the assumption
(or rather, observation) that users have a single python patch version
per python minor version installed, generally the latest, we only check
if the installed patch version matches the selected patch version, if
any, instead of search for one.

In the process, i deduplicated the python discovery logic.
This commit is contained in:
konsti 2024-02-08 16:38:00 -05:00 committed by GitHub
parent 1dc9904f8c
commit 561e33e353
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 232 additions and 135 deletions

View file

@ -16,9 +16,8 @@ use puffin_cache::{Cache, CacheBucket, CachedByTimestamp, Freshness, Timestamp};
use puffin_fs::write_atomic_sync;
use crate::python_platform::PythonPlatform;
use crate::python_query::find_python_windows;
use crate::virtual_env::detect_virtual_env;
use crate::{Error, PythonVersion};
use crate::{find_requested_python, Error, PythonVersion};
/// A Python executable and its associated platform markers.
#[derive(Debug, Clone)]
@ -158,54 +157,42 @@ impl Interpreter {
}
};
let platform = PythonPlatform::from(platform.to_owned());
if let Some(venv) = detect_virtual_env(&platform)? {
let executable = platform.venv_python(venv);
let interpreter = Self::query(&executable, &platform.0, cache)?;
// Check if the venv Python matches.
let python_platform = PythonPlatform::from(platform.to_owned());
if let Some(venv) = detect_virtual_env(&python_platform)? {
let executable = python_platform.venv_python(venv);
let interpreter = Self::query(&executable, &python_platform.0, cache)?;
if version_matches(&interpreter) {
return Ok(Some(interpreter));
}
};
if cfg!(unix) {
if let Some(python_version) = python_version {
let requested = format!(
"python{}.{}",
python_version.major(),
python_version.minor()
);
if let Ok(executable) = Interpreter::find_executable(&requested) {
debug!("Resolved {requested} to {}", executable.display());
let interpreter = Interpreter::query(&executable, &platform.0, cache)?;
if version_matches(&interpreter) {
return Ok(Some(interpreter));
}
// Look for the requested version with by search for `python{major}.{minor}` in `PATH` on
// Unix and `py --list-paths` on Windows.
if let Some(python_version) = python_version {
if let Some(interpreter) =
find_requested_python(&python_version.string, platform, cache)?
{
if version_matches(&interpreter) {
return Ok(Some(interpreter));
}
}
}
if let Ok(executable) = Interpreter::find_executable("python3") {
// Python discovery failed to find the requested version, maybe the default Python in PATH
// matches?
if cfg!(unix) {
if let Some(executable) = Interpreter::find_executable("python3")? {
debug!("Resolved python3 to {}", executable.display());
let interpreter = Interpreter::query(&executable, &platform.0, cache)?;
let interpreter = Interpreter::query(&executable, &python_platform.0, cache)?;
if version_matches(&interpreter) {
return Ok(Some(interpreter));
}
}
} else if cfg!(windows) {
if let Some(python_version) = python_version {
if let Some(path) =
find_python_windows(python_version.major(), python_version.minor())?
{
let interpreter = Interpreter::query(&path, &platform.0, cache)?;
if version_matches(&interpreter) {
return Ok(Some(interpreter));
}
}
}
if let Ok(executable) = Interpreter::find_executable("python.exe") {
let interpreter = Interpreter::query(&executable, &platform.0, cache)?;
if let Some(executable) = Interpreter::find_executable("python.exe")? {
let interpreter = Interpreter::query(&executable, &python_platform.0, cache)?;
if version_matches(&interpreter) {
return Ok(Some(interpreter));
}
@ -217,20 +204,22 @@ impl Interpreter {
Ok(None)
}
/// Find the Python interpreter in `PATH`, respecting `PUFFIN_PYTHON_PATH`.
///
/// Returns `Ok(None)` if not found.
pub fn find_executable<R: AsRef<OsStr> + Into<OsString> + Copy>(
requested: R,
) -> Result<PathBuf, Error> {
if let Some(isolated) = std::env::var_os("PUFFIN_TEST_PYTHON_PATH") {
if let Ok(cwd) = std::env::current_dir() {
which::which_in(requested, Some(isolated), cwd)
.map_err(|err| Error::from_which_error(requested.into(), err))
} else {
which::which_in_global(requested, Some(isolated))
.map_err(|err| Error::from_which_error(requested.into(), err))
.and_then(|mut paths| paths.next().ok_or(Error::PythonNotFound))
}
) -> Result<Option<PathBuf>, Error> {
let result = if let Some(isolated) = std::env::var_os("PUFFIN_TEST_PYTHON_PATH") {
which::which_in(requested, Some(isolated), std::env::current_dir()?)
} else {
which::which(requested).map_err(|err| Error::from_which_error(requested.into(), err))
which::which(requested)
};
match result {
Err(which::Error::CannotFindBinaryPath) => Ok(None),
Err(err) => Err(Error::WhichError(requested.into(), err)),
Ok(path) => Ok(Some(path)),
}
}
@ -276,6 +265,12 @@ impl Interpreter {
u8::try_from(minor).expect("invalid minor version")
}
/// Return the patch version of this Python version.
pub fn python_patch(&self) -> u8 {
let minor = self.markers.python_full_version.version.release()[2];
u8::try_from(minor).expect("invalid patch version")
}
/// Returns the Python version as a simple tuple.
pub fn python_tuple(&self) -> (u8, u8) {
(self.python_major(), self.python_minor())

View file

@ -2,14 +2,16 @@ use std::ffi::OsString;
use std::io;
use std::path::PathBuf;
use pep440_rs::Version;
use thiserror::Error;
use puffin_fs::NormalizedDisplay;
pub use crate::cfg::Configuration;
pub use crate::interpreter::Interpreter;
pub use crate::python_query::{find_default_python, find_requested_python};
pub use crate::python_version::PythonVersion;
pub use crate::virtual_env::Virtualenv;
use crate::Error::WhichError;
mod cfg;
mod interpreter;
@ -40,14 +42,16 @@ pub enum Error {
},
#[error("Failed to run `py --list-paths` to find Python installations. Is Python installed?")]
PyList(#[source] io::Error),
#[error("No Python {major}.{minor} found through `py --list-paths`. Is Python {major}.{minor} installed?")]
NoSuchPython { major: u8, minor: u8 },
#[cfg(windows)]
#[error("No Python {0} found through `py --list-paths`. Is Python {0} installed?")]
NoSuchPython(String),
#[cfg(unix)]
#[error("No Python {0} In `PATH`. Is Python {0} installed?")]
NoSuchPython(String),
#[error("Neither `python` nor `python3` are in `PATH`. Is Python installed?")]
NoPythonInstalledUnix,
#[error("Could not find `python.exe` in PATH and `py --list-paths` did not list any Python versions. Is Python installed?")]
NoPythonInstalledWindows,
#[error("Patch versions cannot be requested on Windows")]
PatchVersionRequestedWindows,
#[error("{message}:\n--- stdout:\n{stdout}\n--- stderr:\n{stderr}\n---")]
PythonSubcommandOutput {
message: String,
@ -60,15 +64,6 @@ pub enum Error {
Cfg(#[from] cfg::Error),
#[error("Error finding `{}` in PATH", _0.to_string_lossy())]
WhichError(OsString, #[source] which::Error),
#[error("Couldn't find `{}` in PATH. Is this Python version installed?", _0.to_string_lossy())]
NotInPath(OsString),
}
impl Error {
pub(crate) fn from_which_error(requested: OsString, source: which::Error) -> Self {
match source {
which::Error::CannotFindBinaryPath => Self::NotInPath(requested),
_ => WhichError(requested, source),
}
}
#[error("Interpreter at `{}` has the wrong patch version. Expected: {}, actual: {}", _0.normalized_display(), _1, _2)]
PatchVersionMismatch(PathBuf, String, Version),
}

View file

@ -5,6 +5,8 @@ use std::path::PathBuf;
use std::process::Command;
use once_cell::sync::Lazy;
use platform_host::Platform;
use puffin_cache::Cache;
use regex::Regex;
use tracing::{info_span, instrument};
@ -19,66 +21,121 @@ static PY_LIST_PATHS: Lazy<Regex> = Lazy::new(|| {
Regex::new(r"(?mR)^ -(?:V:)?(\d).(\d+)-?(?:arm)?(?:\d*)\s*\*?\s*(.*)$").unwrap()
});
/// Find a user requested python version/interpreter.
/// Find a python version/interpreter of a specific version.
///
/// Supported formats:
/// * `-p 3.10` searches for an installed Python 3.10 (`py --list-paths` on Windows, `python3.10` on Linux/Mac).
/// Specifying a patch version is not supported.
/// * `-p 3.10` searches for an installed Python 3.10 (`py --list-paths` on Windows, `python3.10` on
/// Linux/Mac). Specifying a patch version is not supported.
/// * `-p python3.10` or `-p python.exe` looks for a binary in `PATH`.
/// * `-p /home/ferris/.local/bin/python3.10` uses this exact Python.
///
/// When the user passes a patch version (e.g. 3.12.1), we currently search for a matching minor
/// version (e.g. `python3.12` on unix) and error when the version mismatches, as a binary with the
/// patch version (e.g. `python3.12.1`) is often not in `PATH` and we make the simplifying
/// assumption that the user has only this one patch version installed.
#[instrument]
pub fn find_requested_python(request: &str) -> Result<PathBuf, Error> {
pub fn find_requested_python(
request: &str,
platform: &Platform,
cache: &Cache,
) -> Result<Option<Interpreter>, Error> {
let versions = request
.splitn(3, '.')
.map(str::parse::<u8>)
.collect::<Result<Vec<_>, _>>();
if let Ok(versions) = versions {
Ok(Some(if let Ok(versions) = versions {
// `-p 3.10` or `-p 3.10.1`
if cfg!(unix) {
let formatted = PathBuf::from(format!("python{request}"));
Interpreter::find_executable(&formatted)
} else if cfg!(windows) {
if let [major, minor] = versions.as_slice() {
if let Some(python_overwrite) = env::var_os("PUFFIN_TEST_PYTHON_PATH") {
let executable_dir = env::split_paths(&python_overwrite).find(|path| {
path.as_os_str()
.to_str()
// Good enough since we control the bootstrap directory
.is_some_and(|path| path.contains(&format!("@{request}")))
});
return if let Some(path) = executable_dir {
Ok(path.join(if cfg!(unix) {
"python3"
} else if cfg!(windows) {
"python.exe"
} else {
unimplemented!("Only Windows and Unix are supported")
}))
} else {
Err(Error::NoSuchPython {
major: *major,
minor: *minor,
})
};
if let [_major, _minor, requested_patch] = versions.as_slice() {
let formatted = PathBuf::from(format!("python{}.{}", versions[0], versions[1]));
let Some(executable) = Interpreter::find_executable(&formatted)? else {
return Ok(None);
};
let interpreter = Interpreter::query(&executable, platform, cache)?;
if interpreter.python_patch() != *requested_patch {
return Err(Error::PatchVersionMismatch(
executable,
request.to_string(),
interpreter.python_version().clone(),
));
}
find_python_windows(*major, *minor)?.ok_or(Error::NoSuchPython {
major: *major,
minor: *minor,
})
interpreter
} else {
Err(Error::PatchVersionRequestedWindows)
let formatted = PathBuf::from(format!("python{request}"));
let Some(executable) = Interpreter::find_executable(&formatted)? else {
return Ok(None);
};
Interpreter::query(&executable, platform, cache)?
}
} else if cfg!(windows) {
if let Some(python_overwrite) = env::var_os("PUFFIN_TEST_PYTHON_PATH") {
let executable_dir = env::split_paths(&python_overwrite).find(|path| {
path.as_os_str()
.to_str()
// Good enough since we control the bootstrap directory
.is_some_and(|path| path.contains(&format!("@{request}")))
});
return if let Some(path) = executable_dir {
let executable = path.join(if cfg!(unix) {
"python3"
} else if cfg!(windows) {
"python.exe"
} else {
unimplemented!("Only Windows and Unix are supported")
});
Ok(Some(Interpreter::query(&executable, platform, cache)?))
} else {
Ok(None)
};
}
match versions.as_slice() {
[major] => {
let Some(executable) = installed_pythons_windows()?
.into_iter()
.find(|(major_, _minor, _path)| major_ == major)
.map(|(_, _, path)| path)
else {
return Ok(None);
};
Interpreter::query(&executable, platform, cache)?
}
[major, minor] => {
let Some(executable) = find_python_windows(*major, *minor)? else {
return Ok(None);
};
Interpreter::query(&executable, platform, cache)?
}
[major, minor, requested_patch] => {
let Some(executable) = find_python_windows(*major, *minor)? else {
return Ok(None);
};
let interpreter = Interpreter::query(&executable, platform, cache)?;
if interpreter.python_patch() != *requested_patch {
return Err(Error::PatchVersionMismatch(
executable,
request.to_string(),
interpreter.python_version().clone(),
));
}
interpreter
}
_ => unreachable!(),
}
} else {
unimplemented!("Only Windows and Unix are supported")
}
} else if !request.contains(std::path::MAIN_SEPARATOR) {
// `-p python3.10`; Generally not used on windows because all Python are `python.exe`.
Interpreter::find_executable(request)
let Some(executable) = Interpreter::find_executable(request)? else {
return Ok(None);
};
Interpreter::query(&executable, platform, cache)?
} else {
// `-p /home/ferris/.local/bin/python3.10`
Ok(fs_err::canonicalize(request)?)
}
let executable = fs_err::canonicalize(request)?;
Interpreter::query(&executable, platform, cache)?
}))
}
/// Pick a sensible default for the python a user wants when they didn't specify a version.
@ -86,7 +143,7 @@ pub fn find_requested_python(request: &str) -> Result<PathBuf, Error> {
/// We prefer the test overwrite `PUFFIN_TEST_PYTHON_PATH` if it is set, otherwise `python3`/`python` or
/// `python.exe` respectively.
#[instrument]
pub fn find_default_python() -> Result<PathBuf, Error> {
pub fn find_default_python(platform: &Platform, cache: &Cache) -> Result<Interpreter, Error> {
let current_dir = env::current_dir()?;
let python = if cfg!(unix) {
which::which_in(
@ -115,7 +172,9 @@ pub fn find_default_python() -> Result<PathBuf, Error> {
} else {
unimplemented!("Only Windows and Unix are supported")
};
return Ok(fs_err::canonicalize(python)?);
let base_python = fs_err::canonicalize(python)?;
let interpreter = Interpreter::query(&base_python, platform, cache)?;
return Ok(interpreter);
}
/// Run `py --list-paths` to find the installed pythons.
@ -192,8 +251,12 @@ pub(crate) fn find_python_windows(major: u8, minor: u8) -> Result<Option<PathBuf
mod tests {
use std::fmt::Debug;
use insta::{assert_display_snapshot, assert_snapshot};
use insta::assert_display_snapshot;
#[cfg(unix)]
use insta::assert_snapshot;
use itertools::Itertools;
use platform_host::Platform;
use puffin_cache::Cache;
use crate::python_query::find_requested_python;
use crate::Error;
@ -205,26 +268,49 @@ mod tests {
}
#[test]
#[cfg(unix)]
fn no_such_python_version() {
let request = "3.1000";
let result = find_requested_python(
request,
&Platform::current().unwrap(),
&Cache::temp().unwrap(),
)
.unwrap()
.ok_or(Error::NoSuchPython(request.to_string()));
assert_snapshot!(
format_err(find_requested_python("3.1000")),
@"Couldn't find `3.1000` in PATH. Is this Python version installed?"
format_err(result),
@"No Python 3.1000 In `PATH`. Is Python 3.1000 installed?"
);
}
#[test]
#[cfg(unix)]
fn no_such_python_binary() {
let request = "python3.1000";
let result = find_requested_python(
request,
&Platform::current().unwrap(),
&Cache::temp().unwrap(),
)
.unwrap()
.ok_or(Error::NoSuchPython(request.to_string()));
assert_display_snapshot!(
format_err(find_requested_python("python3.1000")),
@"Couldn't find `python3.1000` in PATH. Is this Python version installed?"
format_err(result),
@"No Python python3.1000 In `PATH`. Is Python python3.1000 installed?"
);
}
#[cfg(unix)]
#[test]
fn no_such_python_path() {
let result = find_requested_python(
"/does/not/exists/python3.12",
&Platform::current().unwrap(),
&Cache::temp().unwrap(),
);
assert_display_snapshot!(
format_err(find_requested_python("/does/not/exists/python3.12")), @r###"
format_err(result), @r###"
failed to canonicalize path `/does/not/exists/python3.12`
Caused by: No such file or directory (os error 2)
"###);
@ -233,6 +319,11 @@ mod tests {
#[cfg(windows)]
#[test]
fn no_such_python_path() {
let result = find_requested_python(
r"C:\does\not\exists\python3.12",
&Platform::current().unwrap(),
&Cache::temp().unwrap(),
);
insta::with_settings!({
filters => vec![
// The exact message is host language dependent
@ -240,7 +331,7 @@ mod tests {
]
}, {
assert_display_snapshot!(
format_err(find_requested_python(r"C:\does\not\exists\python3.12")), @r###"
format_err(result), @r###"
failed to canonicalize path `C:\does\not\exists\python3.12`
Caused by: The system cannot find the path specified. (os error 3)
"###);

View file

@ -1,5 +1,6 @@
use pep440_rs::Version;
use pep508_rs::{MarkerEnvironment, StringVersion};
use std::ops::Deref;
use std::str::FromStr;
use crate::Interpreter;
@ -7,6 +8,14 @@ use crate::Interpreter;
#[derive(Debug, Clone)]
pub struct PythonVersion(StringVersion);
impl Deref for PythonVersion {
type Target = StringVersion;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl FromStr for PythonVersion {
type Err = String;