From 561e33e353279965f9c54dc71dbebb3834126a20 Mon Sep 17 00:00:00 2001 From: konsti Date: Thu, 8 Feb 2024 16:38:00 -0500 Subject: [PATCH] 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. --- crates/puffin-interpreter/src/interpreter.rs | 87 ++++---- crates/puffin-interpreter/src/lib.rs | 27 ++- crates/puffin-interpreter/src/python_query.rs | 187 +++++++++++++----- .../puffin-interpreter/src/python_version.rs | 9 + crates/puffin/src/commands/venv.rs | 22 +-- crates/puffin/tests/common/mod.rs | 31 ++- crates/puffin/tests/venv.rs | 4 +- 7 files changed, 232 insertions(+), 135 deletions(-) diff --git a/crates/puffin-interpreter/src/interpreter.rs b/crates/puffin-interpreter/src/interpreter.rs index 8c2c12ece..9bdef18b5 100644 --- a/crates/puffin-interpreter/src/interpreter.rs +++ b/crates/puffin-interpreter/src/interpreter.rs @@ -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 + Into + Copy>( requested: R, - ) -> Result { - 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, 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()) diff --git a/crates/puffin-interpreter/src/lib.rs b/crates/puffin-interpreter/src/lib.rs index 1ecff8be8..aeae933f5 100644 --- a/crates/puffin-interpreter/src/lib.rs +++ b/crates/puffin-interpreter/src/lib.rs @@ -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), } diff --git a/crates/puffin-interpreter/src/python_query.rs b/crates/puffin-interpreter/src/python_query.rs index 90a9c2473..33abd0893 100644 --- a/crates/puffin-interpreter/src/python_query.rs +++ b/crates/puffin-interpreter/src/python_query.rs @@ -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 = 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 { +pub fn find_requested_python( + request: &str, + platform: &Platform, + cache: &Cache, +) -> Result, Error> { let versions = request .splitn(3, '.') .map(str::parse::) .collect::, _>>(); - 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 { /// 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 { +pub fn find_default_python(platform: &Platform, cache: &Cache) -> Result { let current_dir = env::current_dir()?; let python = if cfg!(unix) { which::which_in( @@ -115,7 +172,9 @@ pub fn find_default_python() -> Result { } 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 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) "###); diff --git a/crates/puffin-interpreter/src/python_version.rs b/crates/puffin-interpreter/src/python_version.rs index 702b2a56e..e2567e029 100644 --- a/crates/puffin-interpreter/src/python_version.rs +++ b/crates/puffin-interpreter/src/python_version.rs @@ -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; diff --git a/crates/puffin/src/commands/venv.rs b/crates/puffin/src/commands/venv.rs index bd1117d66..f19c6eaa7 100644 --- a/crates/puffin/src/commands/venv.rs +++ b/crates/puffin/src/commands/venv.rs @@ -16,7 +16,7 @@ use puffin_client::{FlatIndex, FlatIndexClient, RegistryClientBuilder}; use puffin_dispatch::BuildDispatch; use puffin_fs::NormalizedDisplay; use puffin_installer::NoBinary; -use puffin_interpreter::{find_default_python, find_requested_python, Interpreter}; +use puffin_interpreter::{find_default_python, find_requested_python, Error}; use puffin_resolver::InMemoryIndex; use puffin_traits::{BuildContext, InFlight, SetupPyStrategy}; @@ -44,10 +44,6 @@ pub(crate) async fn venv( #[derive(Error, Debug, Diagnostic)] enum VenvError { - #[error("Failed to extract Python interpreter info")] - #[diagnostic(code(puffin::venv::interpreter))] - Interpreter(#[source] puffin_interpreter::Error), - #[error("Failed to create virtualenv")] #[diagnostic(code(puffin::venv::creation))] Creation(#[source] gourgeist::Error), @@ -75,15 +71,15 @@ async fn venv_impl( mut printer: Printer, ) -> miette::Result { // Locate the Python interpreter. - let base_python = if let Some(python_request) = python_request { - find_requested_python(python_request).into_diagnostic()? - } else { - find_default_python().into_diagnostic()? - }; - let platform = Platform::current().into_diagnostic()?; - let interpreter = - Interpreter::query(&base_python, &platform, cache).map_err(VenvError::Interpreter)?; + let interpreter = if let Some(python_request) = python_request { + find_requested_python(python_request, &platform, cache) + .into_diagnostic()? + .ok_or(Error::NoSuchPython(python_request.to_string())) + .into_diagnostic()? + } else { + find_default_python(&platform, cache).into_diagnostic()? + }; writeln!( printer, diff --git a/crates/puffin/tests/common/mod.rs b/crates/puffin/tests/common/mod.rs index e9f74c35b..d7640cdbe 100644 --- a/crates/puffin/tests/common/mod.rs +++ b/crates/puffin/tests/common/mod.rs @@ -10,11 +10,12 @@ use assert_cmd::assert::{Assert, OutputAssertExt}; use assert_cmd::Command; use assert_fs::assert::PathAssert; use assert_fs::fixture::PathChild; -use assert_fs::TempDir; #[cfg(unix)] use fs_err::os::unix::fs::symlink as symlink_file; #[cfg(windows)] use fs_err::os::windows::fs::symlink_file; +use platform_host::Platform; +use puffin_cache::Cache; use regex::Regex; use puffin_interpreter::find_requested_python; @@ -42,15 +43,15 @@ pub const INSTA_FILTERS: &[(&str, &str)] = &[ #[derive(Debug)] pub struct TestContext { - pub temp_dir: TempDir, - pub cache_dir: TempDir, + pub temp_dir: assert_fs::TempDir, + pub cache_dir: assert_fs::TempDir, pub venv: PathBuf, } impl TestContext { pub fn new(python_version: &str) -> Self { - let temp_dir = TempDir::new().expect("Failed to create temp dir"); - let cache_dir = TempDir::new().expect("Failed to create temp dir"); + let temp_dir = assert_fs::TempDir::new().expect("Failed to create temp dir"); + let cache_dir = assert_fs::TempDir::new().expect("Failed to create temp dir"); let venv = create_venv(&temp_dir, &cache_dir, python_version); Self { temp_dir, @@ -142,7 +143,11 @@ pub fn bootstrapped_pythons() -> Option> { /// Create a virtual environment named `.venv` in a temporary directory with the given /// Python version. Expected format for `python` is "python". -pub fn create_venv(temp_dir: &TempDir, cache_dir: &TempDir, python: &str) -> PathBuf { +pub fn create_venv( + temp_dir: &assert_fs::TempDir, + cache_dir: &assert_fs::TempDir, + python: &str, +) -> PathBuf { let python = if let Some(bootstrapped_pythons) = bootstrapped_pythons() { bootstrapped_pythons .into_iter() @@ -200,12 +205,18 @@ pub fn create_bin_with_executables( let bin = temp_dir.child("bin"); fs_err::create_dir(&bin)?; - for request in python_versions { - let executable = find_requested_python(request)?; - let name = executable + for &request in python_versions { + let interpreter = find_requested_python( + request, + &Platform::current().unwrap(), + &Cache::temp().unwrap(), + )? + .ok_or(puffin_interpreter::Error::NoSuchPython(request.to_string()))?; + let name = interpreter + .sys_executable() .file_name() .expect("Discovered executable must have a filename"); - symlink_file(&executable, bin.child(name))?; + symlink_file(interpreter.sys_executable(), bin.child(name))?; } Ok(bin.canonicalize()?) } diff --git a/crates/puffin/tests/venv.rs b/crates/puffin/tests/venv.rs index a84dc3afa..393bc5dbe 100644 --- a/crates/puffin/tests/venv.rs +++ b/crates/puffin/tests/venv.rs @@ -165,7 +165,7 @@ fn create_venv_unknown_python_minor() -> Result<()> { ----- stdout ----- ----- stderr ----- - × Couldn't find `python3.15` in PATH. Is this Python version installed? + × No Python 3.15 In `PATH`. Is Python 3.15 installed? "### ); } @@ -205,7 +205,7 @@ fn create_venv_unknown_python_patch() -> Result<()> { ----- stdout ----- ----- stderr ----- - × Couldn't find `python3.8.0` in PATH. Is this Python version installed? + × No Python 3.8.0 In `PATH`. Is Python 3.8.0 installed? "### );