From 77c6e6fa6ce21d98305bed8f285befc210eb7961 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 17 Dec 2023 10:41:57 -0500 Subject: [PATCH] Add support for `reinstall` to editable packages (#674) Closes https://github.com/astral-sh/puffin/issues/673. --- crates/puffin-cli/src/commands/pip_install.rs | 2 +- crates/puffin-cli/src/commands/pip_sync.rs | 9 ++-- crates/puffin-cli/tests/pip_sync.rs | 42 +++++++++++++++---- crates/puffin-dispatch/src/lib.rs | 2 +- crates/puffin-installer/src/plan.rs | 27 ++++++++++-- 5 files changed, 64 insertions(+), 18 deletions(-) diff --git a/crates/puffin-cli/src/commands/pip_install.rs b/crates/puffin-cli/src/commands/pip_install.rs index 393c56d24133..be8dab558108 100644 --- a/crates/puffin-cli/src/commands/pip_install.rs +++ b/crates/puffin-cli/src/commands/pip_install.rs @@ -308,8 +308,8 @@ async fn install( extraneous: _, } = InstallPlan::from_requirements( &resolution.requirements(), - reinstall, editables, + reinstall, &index_urls, cache, venv, diff --git a/crates/puffin-cli/src/commands/pip_sync.rs b/crates/puffin-cli/src/commands/pip_sync.rs index 3136065c2056..5444089ee7ab 100644 --- a/crates/puffin-cli/src/commands/pip_sync.rs +++ b/crates/puffin-cli/src/commands/pip_sync.rs @@ -62,7 +62,7 @@ pub(crate) async fn pip_sync( pub(crate) async fn sync_requirements( requirements: &[Requirement], reinstall: &Reinstall, - editables: &[EditableRequirement], + editable_requirements: &[EditableRequirement], link_mode: LinkMode, index_urls: IndexUrls, no_build: bool, @@ -92,8 +92,8 @@ pub(crate) async fn sync_requirements( extraneous, } = InstallPlan::from_requirements( requirements, + editable_requirements, reinstall, - editables, &index_urls, cache, &venv, @@ -108,13 +108,14 @@ pub(crate) async fn sync_requirements( && extraneous.is_empty() && editables.is_empty() { - let s = if requirements.len() == 1 { "" } else { "s" }; + let num_requirements = requirements.len() + editable_requirements.len(); + let s = if num_requirements == 1 { "" } else { "s" }; writeln!( printer, "{}", format!( "Audited {} in {}", - format!("{} package{}", requirements.len(), s).bold(), + format!("{num_requirements} package{s}").bold(), elapsed(start.elapsed()) ) .dimmed() diff --git a/crates/puffin-cli/tests/pip_sync.rs b/crates/puffin-cli/tests/pip_sync.rs index becb600656b9..2a43ec6256fe 100644 --- a/crates/puffin-cli/tests/pip_sync.rs +++ b/crates/puffin-cli/tests/pip_sync.rs @@ -6,7 +6,6 @@ use std::process::Command; use anyhow::{Context, Result}; use assert_cmd::prelude::*; use assert_fs::prelude::*; -use assert_fs::TempDir; use indoc::indoc; use insta_cmd::_macro_support::insta; use insta_cmd::{assert_cmd_snapshot, get_cargo_bin}; @@ -2092,8 +2091,8 @@ fn reinstall_package() -> Result<()> { #[test] fn install_editable() -> Result<()> { - let temp_dir = TempDir::new()?; - let cache_dir = TempDir::new()?; + let temp_dir = assert_fs::TempDir::new()?; + let cache_dir = assert_fs::TempDir::new()?; let venv = create_venv_py312(&temp_dir, &cache_dir); let requirements_txt = temp_dir.child("requirements.txt"); @@ -2106,6 +2105,7 @@ fn install_editable() -> Result<()> { " })?; + // Install the editable packages. let filter_path = requirements_txt.display().to_string(); let filters = INSTA_FILTERS .iter() @@ -2113,7 +2113,7 @@ fn install_editable() -> Result<()> { .copied() .collect::>(); insta::with_settings!({ - filters => filters + filters => filters.clone() }, { assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .arg("pip-sync") @@ -2138,7 +2138,33 @@ fn install_editable() -> Result<()> { "###); }); - // Make sure we have the right base case + // Reinstall the editable packages. + insta::with_settings!({ + filters => filters.clone() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-sync") + .arg(requirements_txt.path()) + .arg("--reinstall-package") + .arg("poetry-editable") + .arg("--cache-dir") + .arg(cache_dir.path()) + .env("VIRTUAL_ENV", venv.as_os_str()) + .env("CARGO_TARGET_DIR", "../../../target/target_install_editable"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Built 1 editable in [TIME] + Uninstalled 1 package in [TIME] + Installed 1 package in [TIME] + - poetry-editable==0.1.0 + + poetry-editable @ ../../scripts/editable-installs/poetry_editable + "###); + }); + + // Make sure we have the right base case. let python_source_file = "../../scripts/editable-installs/maturin_editable/python/maturin_editable/__init__.py"; let python_version_1 = indoc! {r" @@ -2155,7 +2181,7 @@ fn install_editable() -> Result<()> { "#}; check_command(&venv, command, &temp_dir); - // Edit the sources + // Edit the sources. let python_version_2 = indoc! {r" from .maturin_editable import * version = 2 @@ -2171,7 +2197,7 @@ fn install_editable() -> Result<()> { "#}; check_command(&venv, command, &temp_dir); - // Don't create a git diff + // Don't create a git diff. fs_err::write(python_source_file, python_version_1)?; let filters = INSTA_FILTERS @@ -2194,7 +2220,7 @@ fn install_editable() -> Result<()> { ----- stdout ----- ----- stderr ----- - Audited 2 packages in [TIME] + Audited 4 packages in [TIME] "###); }); diff --git a/crates/puffin-dispatch/src/lib.rs b/crates/puffin-dispatch/src/lib.rs index 93ef102e3136..d38fce2f4afb 100644 --- a/crates/puffin-dispatch/src/lib.rs +++ b/crates/puffin-dispatch/src/lib.rs @@ -141,8 +141,8 @@ impl BuildContext for BuildDispatch { editables: _, } = InstallPlan::from_requirements( &resolution.requirements(), - &Reinstall::None, &[], + &Reinstall::None, &self.index_urls, self.cache(), venv, diff --git a/crates/puffin-installer/src/plan.rs b/crates/puffin-installer/src/plan.rs index 6c231339888b..b2dd617f71b6 100644 --- a/crates/puffin-installer/src/plan.rs +++ b/crates/puffin-installer/src/plan.rs @@ -48,8 +48,8 @@ impl InstallPlan { /// need to be downloaded, and those that should be removed. pub fn from_requirements( requirements: &[Requirement], - reinstall: &Reinstall, editable_requirements: &[EditableRequirement], + reinstall: &Reinstall, index_urls: &IndexUrls, cache: &Cache, venv: &Virtualenv, @@ -72,10 +72,29 @@ impl InstallPlan { // Remove any editable requirements. for editable in editable_requirements { - if site_packages.remove_editable(editable.raw()).is_some() { - debug!("Treating editable requirement as immutable: {editable}"); - } else { + // Check if the package should be reinstalled. A reinstall involves (1) purging any + // cached distributions, and (2) marking any installed distributions as extraneous. + // For editables, we don't cache installations, so there's nothing to purge; and since + // editable installs lack a package name, we first lookup by URL, and then by name. + let reinstall = match reinstall { + Reinstall::None => false, + Reinstall::All => true, + Reinstall::Packages(packages) => site_packages + .get_editable(editable.raw()) + .is_some_and(|distribution| packages.contains(distribution.name())), + }; + + if reinstall { + if let Some(distribution) = site_packages.remove_editable(editable.raw()) { + reinstalls.push(distribution); + } editables.push(editable.clone()); + } else { + if site_packages.remove_editable(editable.raw()).is_some() { + debug!("Treating editable requirement as immutable: {editable}"); + } else { + editables.push(editable.clone()); + } } }