From 637770600fe23d9cb51091d9c53a408205677727 Mon Sep 17 00:00:00 2001 From: Jonathan Schwender <55576758+jschwe@users.noreply.github.com> Date: Thu, 12 Sep 2024 04:21:41 +0200 Subject: [PATCH] libservo: Improve finding python (#33385) * libservo: Improve finding python Servo is built in a virtual environment, which sets `VIRTUAL_ENV` to the base path of the venv. PYTHON3 is only set if mach or the user set it. Additional changes: - Use Path / var_os for Paths instead of strings. In General using Path APIs is preferable, since in rare cases valid paths may not be valid utf-8. - Don't search for python 3.8 anymore, since we require a newer version anyway. - Don't add the .exe suffix anymore, since Command::new() will take care of that. Signed-off-by: Jonathan Schwender Signed-off-by: Jonathan Schwender * script: Improve finding python Synchronize `find_python` in scripts build.rs with the version in libservo. Signed-off-by: Jonathan Schwender * Apply suggestions from code review Co-authored-by: Martin Robinson Signed-off-by: Jonathan Schwender <55576758+jschwe@users.noreply.github.com> Signed-off-by: Jonathan Schwender * Fix finding venv python on windows - On Windows the venv scripts and python binaries are in the `Scripts` subdirectory instead of `bin`. - We shouldn't check if the executable in the venv binary dir exists, since the actual file could have a `.exe` suffix. We don't need to consider the `.exe` suffix for the filename, since `Command` will handle that for us. We also anyway check the validity of the candidate file, by running `$candidate --version`. Signed-off-by: Jonathan Schwender --------- Signed-off-by: Jonathan Schwender Signed-off-by: Jonathan Schwender Signed-off-by: Jonathan Schwender <55576758+jschwe@users.noreply.github.com> Co-authored-by: Martin Robinson --- components/script/build.rs | 71 +++++++++++++++++++++++++++----------- components/servo/build.rs | 65 ++++++++++++++++++++++------------ 2 files changed, 93 insertions(+), 43 deletions(-) diff --git a/components/script/build.rs b/components/script/build.rs index 2bc449dcdef..ea598ae57ff 100644 --- a/components/script/build.rs +++ b/components/script/build.rs @@ -64,26 +64,55 @@ impl<'a> phf_shared::PhfHash for Bytes<'a> { } } -fn find_python() -> String { - env::var("PYTHON3").ok().unwrap_or_else(|| { - let candidates = if cfg!(windows) { - ["python.exe", "python"] - } else { - ["python3", "python"] - }; - for &name in &candidates { - if Command::new(name) - .arg("--version") - .output() - .ok() - .map_or(false, |out| out.status.success()) - { - return name.to_owned(); - } +/// Tries to find a suitable python +/// +/// Algorithm +/// 1. Trying to find python3/python in $VIRTUAL_ENV (this should be from Servo's venv) +/// 2. Checking PYTHON3 (set by mach) +/// 3. Falling back to the system installation. +/// +/// Note: This function should be kept in sync with the version in `components/servo/build.rs` +fn find_python() -> PathBuf { + let mut candidates = vec![]; + if let Some(venv) = env::var_os("VIRTUAL_ENV") { + let bin_directory = PathBuf::from(venv).join("bin"); + + let python3 = bin_directory.join("python3"); + if python3.exists() { + candidates.push(python3); } - panic!( - "Can't find python (tried {})! Try fixing PATH or setting the PYTHON3 env var", - candidates.join(", ") - ) - }) + let python = bin_directory.join("python"); + if python.exists() { + candidates.push(python); + } + }; + if let Some(python3) = env::var_os("PYTHON3") { + let python3 = PathBuf::from(python3); + if python3.exists() { + candidates.push(python3); + } + } + + let system_python = ["python3", "python"].map(PathBuf::from); + candidates.extend_from_slice(&system_python); + + for name in &candidates { + // Command::new() allows us to omit the `.exe` suffix on windows + if Command::new(&name) + .arg("--version") + .output() + .is_ok_and(|out| out.status.success()) + { + return name.to_owned(); + } + } + let candidates = candidates + .into_iter() + .map(|c| c.into_os_string()) + .collect::>(); + panic!( + "Can't find python (tried {:?})! Try enabling Servo's Python venv, \ + setting the PYTHON3 env var or adding python3 to PATH.", + candidates.join(", ".as_ref()) + ) } diff --git a/components/servo/build.rs b/components/servo/build.rs index 6920fbcfbb1..65f751b89cb 100644 --- a/components/servo/build.rs +++ b/components/servo/build.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; use std::{env, fs}; @@ -23,26 +23,47 @@ fn main() { fs::write(path, output.stdout).unwrap(); } -fn find_python() -> String { - env::var("PYTHON3").ok().unwrap_or_else(|| { - let candidates = if cfg!(windows) { - ["python3.8.exe", "python38.exe", "python.exe"] - } else { - ["python3.8", "python3", "python"] - }; - for &name in &candidates { - if Command::new(name) - .arg("--version") - .output() - .ok() - .map_or(false, |out| out.status.success()) - { - return name.to_owned(); - } +/// Tries to find a suitable python +/// +/// Algorithm +/// 1. Trying to find python3/python in $VIRTUAL_ENV (this should be from servos venv) +/// 2. Checking PYTHON3 (set by mach) +/// 3. Falling back to the system installation. +/// +/// Note: This function should be kept in sync with the version in `components/script/build.rs` +fn find_python() -> PathBuf { + let mut candidates = vec![]; + if let Some(venv) = env::var_os("VIRTUAL_ENV") { + // See: https://docs.python.org/3/library/venv.html#how-venvs-work + let bin_dir = if cfg!(windows) { "Scripts" } else { "bin" }; + let bin_directory = PathBuf::from(venv).join(bin_dir); + candidates.push(bin_directory.join("python3")); + candidates.push(bin_directory.join("python")); + } + if let Some(python3) = env::var_os("PYTHON3") { + candidates.push(PathBuf::from(python3)); + } + + let system_python = ["python3", "python"].map(PathBuf::from); + candidates.extend_from_slice(&system_python); + + for name in &candidates { + // Command::new() allows us to omit the `.exe` suffix on windows + if Command::new(&name) + .arg("--version") + .output() + .is_ok_and(|out| out.status.success()) + { + return name.to_owned(); } - panic!( - "Can't find python (tried {})! Try fixing PATH or setting the PYTHON3 env var", - candidates.join(", ") - ) - }) + } + let candidates = candidates + .into_iter() + .map(|c| c.into_os_string()) + .collect::>(); + panic!( + "Can't find python (tried {:?})! Try enabling Servo's Python venv, \ + setting the PYTHON3 env var or adding python3 to PATH.", + candidates.join(", ".as_ref()) + ) }