From ac49dec4a20bd884e55e296c939cd8f21cc6d056 Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 6 Feb 2024 20:28:30 +0100 Subject: [PATCH] Multiple entries in PUFFIN_PYTHON_PATH for windows tests (#1254) There are no binary installers for the latests patch versions of cpython for windows, and building them is hard. As an alternative, we download python-build-standanlone cpythons and put them into `/bin`. On unix, we can symlink `pythonx.y.z` into this directory and point `PUFFIN_PYTHON_PATH` to it. On windows, all pythons are called `python.exe` and they don't like being linked. Instead, we add the path to each directory containing a `python.exe` to `PUFFIN_PYTHON_PATH`, similar to the regular `PATH`. The python discovery on windows was extended to respect `PUFFIN_PYTHON_PATH` where needed. These changes mean that we don't need to (sym)link pythons anymore and could drop that part to the script. 435 tests run: 389 passed (21 slow), 46 failed, 1 skipped --- crates/puffin-interpreter/src/python_query.rs | 26 ++++++++- crates/puffin/tests/common/mod.rs | 57 ++++++++++++++++++- crates/puffin/tests/pip_compile_scenarios.rs | 15 ++++- scripts/scenarios/templates/compile.mustache | 15 ++++- 4 files changed, 108 insertions(+), 5 deletions(-) diff --git a/crates/puffin-interpreter/src/python_query.rs b/crates/puffin-interpreter/src/python_query.rs index 61fd3821e..cfaa02170 100644 --- a/crates/puffin-interpreter/src/python_query.rs +++ b/crates/puffin-interpreter/src/python_query.rs @@ -1,5 +1,6 @@ //! Find a user requested python version/interpreter. +use std::env; use std::path::PathBuf; use std::process::Command; @@ -37,6 +38,19 @@ pub fn find_requested_python(request: &str) -> Result { let formatted = PathBuf::from(format!("python{request}")); Interpreter::find_executable(&formatted) } else if cfg!(windows) { + if let Some(python_overwrite) = env::var_os("PUFFIN_PYTHON_PATH") { + for path in env::split_paths(&python_overwrite) { + if path + .as_os_str() + .to_str() + // Good enough since we control the bootstrap directory + .is_some_and(|path| path.contains(&format!("@{request}"))) + { + return Ok(path); + } + } + } + if let [major, minor] = versions.as_slice() { find_python_windows(*major, *minor)?.ok_or(Error::NoSuchPython { major: *major, @@ -58,16 +72,22 @@ pub fn find_requested_python(request: &str) -> Result { } /// Pick a sensible default for the python a user wants when they didn't specify a version. +/// +/// We prefer the test overwrite `PUFFIN_PYTHON_PATH` if it is set, otherwise `python3`/`python` or +/// `python.exe` respectively. #[instrument] pub fn find_default_python() -> Result { + let current_dir = env::current_dir()?; let python = if cfg!(unix) { - which::which("python3") + which::which_in("python3", env::var_os("PUFFIN_PYTHON_PATH"), current_dir) .or_else(|_| which::which("python")) .map_err(|_| Error::NoPythonInstalledUnix)? } else if cfg!(windows) { // TODO(konstin): Is that the right order, or should we look for `py --list-paths` first? With the current way // it works even if the python launcher is not installed. - if let Ok(python) = which::which("python.exe") { + if let Ok(python) = + which::which_in("python.exe", env::var_os("PUFFIN_PYTHON_PATH"), current_dir) + { python } else { installed_pythons_windows()? @@ -87,6 +107,8 @@ pub fn find_default_python() -> Result { /// The command takes 8ms on my machine. TODO(konstin): Implement to read python /// installations from the registry instead. fn installed_pythons_windows() -> Result, Error> { + // TODO(konstin): We're not checking PUFFIN_PYTHON_PATH here, no test currently depends on it. + // TODO(konstin): Special case the not found error let output = info_span!("py_list_paths") .in_scope(|| Command::new("py").arg("--list-paths").output()) diff --git a/crates/puffin/tests/common/mod.rs b/crates/puffin/tests/common/mod.rs index 3961917ab..fb763e05d 100644 --- a/crates/puffin/tests/common/mod.rs +++ b/crates/puffin/tests/common/mod.rs @@ -1,7 +1,6 @@ // The `unreachable_pub` is to silence false positives in RustRover. #![allow(dead_code, unreachable_pub)] -use std::env; use std::path::{Path, PathBuf}; use std::process::Output; @@ -92,9 +91,65 @@ pub fn venv_to_interpreter(venv: &Path) -> PathBuf { } } +/// If bootstrapped python build standalone pythons exists in `/bin`, +/// return the paths to the directories containing the python binaries (i.e. as paths that +/// `which::which_in` can use). +/// +/// Use `scripts/bootstrap/install.py` to bootstrap. +/// +/// Python versions are sorted from newest to oldest. +pub fn bootstrapped_pythons() -> Option> { + // Current dir is `/crates/puffin`. + let bootstrapped_pythons = std::env::current_dir() + .unwrap() + .parent() + .unwrap() + .parent() + .unwrap() + .join("bin") + .join("versions"); + let Ok(bootstrapped_pythons) = fs_err::read_dir(bootstrapped_pythons) else { + return None; + }; + + let mut bootstrapped_pythons: Vec = bootstrapped_pythons + .map(Result::unwrap) + .filter(|entry| entry.metadata().unwrap().is_dir()) + .map(|entry| { + if cfg!(unix) { + entry.path().join("install").join("bin") + } else if cfg!(windows) { + entry.path().join("install") + } else { + unimplemented!("Only Windows and Unix are supported") + } + }) + .collect(); + bootstrapped_pythons.sort(); + // Prefer the most recent patch version. + bootstrapped_pythons.reverse(); + Some(bootstrapped_pythons) +} + /// 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 { + let python = if let Some(bootstrapped_pythons) = bootstrapped_pythons() { + bootstrapped_pythons + .into_iter() + // Good enough since we control the directory + .find(|path| path.to_str().unwrap().contains(&format!("@{python}"))) + .expect("Missing python bootstrap version") + .join(if cfg!(unix) { + "python3" + } else if cfg!(windows) { + "python.exe" + } else { + unimplemented!("Only Windows and Unix are supported") + }) + } else { + PathBuf::from(python) + }; let venv = temp_dir.child(".venv"); Command::new(get_bin()) .arg("venv") diff --git a/crates/puffin/tests/pip_compile_scenarios.rs b/crates/puffin/tests/pip_compile_scenarios.rs index 5b66b205b..0e86026c9 100644 --- a/crates/puffin/tests/pip_compile_scenarios.rs +++ b/crates/puffin/tests/pip_compile_scenarios.rs @@ -5,6 +5,7 @@ //! #![cfg(all(feature = "python", feature = "pypi"))] +use std::env; use std::path::PathBuf; use std::process::Command; @@ -17,7 +18,7 @@ use fs_err::os::unix::fs::symlink as symlink_file; use fs_err::os::windows::fs::symlink_file; use predicates::prelude::predicate; -use common::{get_bin, puffin_snapshot, TestContext, INSTA_FILTERS}; +use common::{bootstrapped_pythons, get_bin, puffin_snapshot, TestContext, INSTA_FILTERS}; use puffin_interpreter::find_requested_python; mod common; @@ -27,6 +28,18 @@ pub(crate) fn create_bin_with_executables( temp_dir: &assert_fs::TempDir, python_versions: &[&str], ) -> Result { + if let Some(bootstrapped_pythons) = bootstrapped_pythons() { + let selected_pythons = bootstrapped_pythons.into_iter().filter(|path| { + python_versions.iter().any(|python_version| { + // Good enough since we control the directory + path.to_str() + .unwrap() + .contains(&format!("@{python_version}")) + }) + }); + return Ok(env::join_paths(selected_pythons)?.into()); + } + let bin = temp_dir.child("bin"); fs_err::create_dir(&bin)?; for request in python_versions { diff --git a/scripts/scenarios/templates/compile.mustache b/scripts/scenarios/templates/compile.mustache index 2f8d25c8d..1515e87c4 100644 --- a/scripts/scenarios/templates/compile.mustache +++ b/scripts/scenarios/templates/compile.mustache @@ -5,6 +5,7 @@ //! #![cfg(all(feature = "python", feature = "pypi"))] +use std::env; use std::path::PathBuf; use std::process::Command; @@ -17,7 +18,7 @@ use fs_err::os::unix::fs::symlink as symlink_file; use fs_err::os::windows::fs::symlink_file; use predicates::prelude::predicate; -use common::{get_bin, puffin_snapshot, TestContext, INSTA_FILTERS}; +use common::{bootstrapped_pythons, get_bin, puffin_snapshot, TestContext, INSTA_FILTERS}; use puffin_interpreter::find_requested_python; mod common; @@ -27,6 +28,18 @@ pub(crate) fn create_bin_with_executables( temp_dir: &assert_fs::TempDir, python_versions: &[&str], ) -> Result { + if let Some(bootstrapped_pythons) = bootstrapped_pythons() { + let selected_pythons = bootstrapped_pythons.into_iter().filter(|path| { + python_versions.iter().any(|python_version| { + // Good enough since we control the directory + path.to_str() + .unwrap() + .contains(&format!("@{python_version}")) + }) + }); + return Ok(env::join_paths(selected_pythons)?.into()); + } + let bin = temp_dir.child("bin"); fs_err::create_dir(&bin)?; for request in python_versions {