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? "### );