Use sys.executable as python root path (#431)

Previously, we were assuming that `which <python>` return the path to
the python executable. This is not true when using pyenv shims, which
are bash scripts. Instead, we have to use `sys.executable`. Luckily,
we're already querying the python interpreter and can do it in that
pass.

We are also not allowed to cache the execution of the python interpreter
through the shim because pyenv might change the target. As a heuristic,
we check whether `sys.executable`, the real binary, is the same our
canonicalized `which` result.

---------

Co-authored-by: Zanie Blue <contact@zanie.dev>

crates/gourgeist/src/bare.rs

@@ -3,14 +3,15 @@
 use std::io;
 use std::io::{BufWriter, Write};
 
-use camino::{Utf8Path, Utf8PathBuf};
+use camino::{FromPathError, Utf8Path, Utf8PathBuf};
 use fs_err as fs;
 #[cfg(unix)]
 use fs_err::os::unix::fs::symlink;
 use fs_err::File;
-use puffin_interpreter::InterpreterInfo;
 use tracing::info;
 
+use puffin_interpreter::InterpreterInfo;
+
 /// The bash activate scripts with the venv dependent paths patches out
 const ACTIVATE_TEMPLATES: &[(&str, &str)] = &[
     ("activate", include_str!("activator/activate")),
@@ -35,31 +36,31 @@ fn write_cfg(f: &mut impl Write, data: &[(&str, String); 8]) -> io::Result<()> {
 
 /// Absolute paths of the virtualenv
 #[derive(Debug)]
-pub(crate) struct VenvPaths {
+pub struct VenvPaths {
     /// The location of the virtualenv, e.g. `.venv`
     #[allow(unused)]
-    pub(crate) root: Utf8PathBuf,
+    pub root: Utf8PathBuf,
 
     /// The python interpreter.rs inside the virtualenv, on unix `.venv/bin/python`
     #[allow(unused)]
-    pub(crate) interpreter: Utf8PathBuf,
+    pub interpreter: Utf8PathBuf,
 
     /// The directory with the scripts, on unix `.venv/bin`
     #[allow(unused)]
-    pub(crate) bin: Utf8PathBuf,
+    pub bin: Utf8PathBuf,
 
     /// The site-packages directory where all the packages are installed to, on unix
     /// and python 3.11 `.venv/lib/python3.11/site-packages`
     #[allow(unused)]
-    pub(crate) site_packages: Utf8PathBuf,
+    pub site_packages: Utf8PathBuf,
 }
 
 /// Write all the files that belong to a venv without any packages installed.
-pub(crate) fn create_bare_venv(
-    location: &Utf8Path,
-    base_python: &Utf8Path,
-    info: &InterpreterInfo,
-) -> io::Result<VenvPaths> {
+pub fn create_bare_venv(location: &Utf8Path, info: &InterpreterInfo) -> io::Result<VenvPaths> {
+    let base_python: &Utf8Path = info
+        .sys_executable()
+        .try_into()
+        .map_err(|err: FromPathError| err.into_io_error())?;
     if location.exists() {
         if location.join("pyvenv.cfg").is_file() {
             info!("Removing existing directory");

crates/gourgeist/src/lib.rs

@@ -1,14 +1,14 @@
 use std::io;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
-use camino::{Utf8Path, Utf8PathBuf};
+use camino::{FromPathError, Utf8Path};
 use thiserror::Error;
 
 pub use interpreter::parse_python_cli;
 use platform_host::PlatformError;
 use puffin_interpreter::{InterpreterInfo, Virtualenv};
 
-use crate::bare::create_bare_venv;
+pub use crate::bare::create_bare_venv;
 
 mod bare;
 mod interpreter;
@@ -19,21 +19,15 @@ pub enum Error {
     IO(#[from] io::Error),
     #[error("Failed to determine python interpreter to use")]
     InvalidPythonInterpreter(#[source] Box<dyn std::error::Error + Sync + Send>),
-    #[error("{0} is not a valid UTF-8 path")]
-    NonUTF8Path(PathBuf),
     #[error(transparent)]
     Platform(#[from] PlatformError),
 }
 
 /// Create a virtualenv.
-pub fn create_venv(
-    location: impl Into<PathBuf>,
-    base_python: impl AsRef<Path>,
-    info: &InterpreterInfo,
-) -> Result<Virtualenv, Error> {
-    let location = Utf8PathBuf::from_path_buf(location.into()).map_err(Error::NonUTF8Path)?;
-    let base_python = Utf8Path::from_path(base_python.as_ref())
-        .ok_or_else(|| Error::NonUTF8Path(base_python.as_ref().to_path_buf()))?;
-    let paths = create_bare_venv(&location, base_python, info)?;
+pub fn create_venv(location: &Path, info: &InterpreterInfo) -> Result<Virtualenv, Error> {
+    let location: &Utf8Path = location
+        .try_into()
+        .map_err(|err: FromPathError| err.into_io_error())?;
+    let paths = create_bare_venv(location, info)?;
     Ok(Virtualenv::new_prefix(paths.root.as_std_path(), info))
 }

crates/gourgeist/src/main.rs

@@ -4,14 +4,15 @@ use std::time::Instant;
 
 use camino::Utf8PathBuf;
 use clap::Parser;
-use gourgeist::{create_venv, parse_python_cli};
-use platform_host::Platform;
-use puffin_interpreter::InterpreterInfo;
 use tracing::info;
 use tracing_subscriber::layer::SubscriberExt;
 use tracing_subscriber::util::SubscriberInitExt;
 use tracing_subscriber::{fmt, EnvFilter};
 
+use gourgeist::{create_bare_venv, parse_python_cli};
+use platform_host::Platform;
+use puffin_interpreter::InterpreterInfo;
+
 #[derive(Parser, Debug)]
 struct Cli {
     path: Option<Utf8PathBuf>,
@@ -25,8 +26,7 @@ fn run() -> Result<(), gourgeist::Error> {
     let python = parse_python_cli(cli.python)?;
     let platform = Platform::current()?;
     let info = InterpreterInfo::query(python.as_std_path(), platform, None).unwrap();
-    create_venv(location, &python, &info)?;
-
+    create_bare_venv(&location, &info)?;
     Ok(())
 }
 

crates/puffin-build/src/lib.rs

@@ -251,11 +251,7 @@ impl SourceBuild {
             None
         };
 
-        let venv = gourgeist::create_venv(
-            temp_dir.path().join(".venv"),
-            build_context.base_python(),
-            interpreter_info,
-        )?;
+        let venv = gourgeist::create_venv(&temp_dir.path().join(".venv"), interpreter_info)?;
 
         // There are packages such as DTLSSocket 0.1.16 that say
         // ```toml

crates/puffin-cli/src/commands/venv.rs

@@ -78,8 +78,9 @@ fn venv_impl(
 
     writeln!(
         printer,
-        "Using Python interpreter: {}",
-        format!("{}", base_python.display()).cyan()
+        "Using Python {} at {}",
+        interpreter_info.version(),
+        format!("{}", interpreter_info.sys_executable().display()).cyan()
     )
     .into_diagnostic()?;
 
@@ -95,8 +96,7 @@ fn venv_impl(
     .into_diagnostic()?;
 
     // Create the virtual environment.
-    gourgeist::create_venv(path, &base_python, &interpreter_info)
-        .map_err(VenvError::CreationError)?;
+    gourgeist::create_venv(path, &interpreter_info).map_err(VenvError::CreationError)?;
 
     Ok(ExitStatus::Success)
 }

crates/puffin-cli/tests/pip_sync.rs

@@ -62,6 +62,8 @@ fn install() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -105,6 +107,8 @@ fn install_copy() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -150,6 +154,8 @@ fn install_hardlink() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -195,6 +201,8 @@ fn install_many() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -238,6 +246,8 @@ fn noop() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -292,6 +302,8 @@ fn link() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -317,6 +329,8 @@ fn link() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -357,6 +371,8 @@ fn add_remove() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -422,6 +438,8 @@ fn install_sequential() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -480,6 +498,8 @@ fn upgrade() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -537,6 +557,8 @@ fn install_url() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -583,6 +605,8 @@ fn install_git_commit() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -629,6 +653,8 @@ fn install_git_tag() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -675,6 +701,8 @@ fn install_git_subdirectories() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -720,6 +748,8 @@ fn install_sdist() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -766,6 +796,8 @@ fn install_url_then_install_url() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -822,6 +854,8 @@ fn install_url_then_install_version() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -882,6 +916,8 @@ fn install_version_then_install_url() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();
@@ -987,6 +1023,8 @@ fn warn_on_yanked_version() -> Result<()> {
         .arg(venv.as_os_str())
         .arg("--cache-dir")
         .arg(cache_dir.path())
+        .arg("--python")
+        .arg("python3.12")
         .current_dir(&temp_dir)
         .assert()
         .success();

crates/puffin-cli/tests/snapshots/venv__create_venv.snap

@@ -4,13 +4,15 @@ info:
   program: puffin
   args:
     - venv
-    - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpn0fxWx/.venv
+    - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmprOsp0M/.venv
+    - "--python"
+    - python3.12
 ---
 success: true
 exit_code: 0
 ----- stdout -----
 
 ----- stderr -----
-Using Python interpreter: /usr/bin/python3
+Using Python 3.11 at [PATH]
 Creating virtual environment at: /home/ferris/project/.venv
 

crates/puffin-cli/tests/snapshots/venv__create_venv_defaults_to_cwd.snap

@@ -4,12 +4,14 @@ info:
   program: puffin
   args:
     - venv
+    - "--python"
+    - python3.12
 ---
 success: true
 exit_code: 0
 ----- stdout -----
 
 ----- stderr -----
-Using Python interpreter: /usr/bin/python3
+Using Python 3.11 at [PATH]
 Creating virtual environment at: .venv
 

crates/puffin-cli/tests/venv.rs

@@ -19,12 +19,15 @@ fn create_venv() -> Result<()> {
     insta::with_settings!({
         filters => vec![
             (r"Using Python interpreter: .+", "Using Python interpreter: /usr/bin/python3"),
+            (r"Using Python 3.12 at .+", "Using Python 3.11 at [PATH]"),
             (tempdir.to_str().unwrap(), "/home/ferris/project"),
         ]
     }, {
         assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
             .arg("venv")
             .arg(venv.as_os_str())
+            .arg("--python")
+            .arg("python3.12")
             .current_dir(&tempdir));
     });
 
@@ -41,11 +44,14 @@ fn create_venv_defaults_to_cwd() -> Result<()> {
     insta::with_settings!({
         filters => vec![
             (r"Using Python interpreter: .+", "Using Python interpreter: /usr/bin/python3"),
+            (r"Using Python 3.12 at .+", "Using Python 3.11 at [PATH]"),
             (tempdir.to_str().unwrap(), "/home/ferris/project"),
         ]
     }, {
         assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
             .arg("venv")
+            .arg("--python")
+            .arg("python3.12")
             .current_dir(&tempdir));
     });
 

crates/puffin-interpreter/src/get_interpreter_info.py

@@ -35,5 +35,6 @@ interpreter_info = {
     "markers": markers,
     "base_prefix": sys.base_prefix,
     "base_exec_prefix": sys.base_exec_prefix,
+    "sys_executable": sys.executable,
 }
 print(json.dumps(interpreter_info))

crates/puffin-interpreter/src/interpreter_info.rs

@@ -20,6 +20,7 @@ pub struct InterpreterInfo {
     pub(crate) markers: MarkerEnvironment,
     pub(crate) base_exec_prefix: PathBuf,
     pub(crate) base_prefix: PathBuf,
+    pub(crate) sys_executable: PathBuf,
 }
 
 impl InterpreterInfo {
@@ -42,6 +43,7 @@ impl InterpreterInfo {
             markers: info.markers,
             base_exec_prefix: info.base_exec_prefix,
             base_prefix: info.base_prefix,
+            sys_executable: info.sys_executable,
         })
     }
 
@@ -51,12 +53,14 @@ impl InterpreterInfo {
         markers: MarkerEnvironment,
         base_exec_prefix: PathBuf,
         base_prefix: PathBuf,
+        sys_executable: PathBuf,
     ) -> Self {
         Self {
             platform: PythonPlatform(platform),
             markers,
             base_exec_prefix,
             base_prefix,
+            sys_executable,
         }
     }
 }
@@ -91,6 +95,9 @@ impl InterpreterInfo {
     pub fn base_prefix(&self) -> &Path {
         &self.base_prefix
     }
+    pub fn sys_executable(&self) -> &Path {
+        &self.sys_executable
+    }
 
     /// Inject markers of a fake python version
     #[must_use]
@@ -116,6 +123,7 @@ pub(crate) struct InterpreterQueryResult {
     pub(crate) markers: MarkerEnvironment,
     pub(crate) base_exec_prefix: PathBuf,
     pub(crate) base_prefix: PathBuf,
+    pub(crate) sys_executable: PathBuf,
 }
 
 impl InterpreterQueryResult {
@@ -188,9 +196,13 @@ impl InterpreterQueryResult {
         debug!("Detecting markers for {}", executable.display());
         let info = Self::query(executable)?;
 
-        // Write to the cache.
-        if let Some(key) = key {
-            cacache::write_sync(cache, key, serde_json::to_vec(&info)?)?;
+        // If the executable is actually a pyenv shim, a bash script that redirects to the activated
+        // python, we're not allowed to cache the interpreter info
+        if executable == info.sys_executable {
+            // Write to the cache.
+            if let Some(key) = key {
+                cacache::write_sync(cache, key, serde_json::to_vec(&info)?)?;
+            }
         }
 
         Ok(info)

crates/puffin-resolver/tests/resolver.rs

@@ -76,6 +76,7 @@ async fn resolve(
             markers.clone(),
             PathBuf::from("/dev/null"),
             PathBuf::from("/dev/null"),
+            PathBuf::from("/dev/null"),
         ),
     };
     let resolver = Resolver::new(manifest, markers, tags, &client, &build_context);