Skip to content

Commit

Permalink
Add support for reinstall to editable packages (#674)
Browse files Browse the repository at this point in the history
Closes #673.
  • Loading branch information
charliermarsh authored Dec 17, 2023
1 parent 00e1c33 commit 77c6e6f
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 18 deletions.
2 changes: 1 addition & 1 deletion crates/puffin-cli/src/commands/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,8 @@ async fn install(
extraneous: _,
} = InstallPlan::from_requirements(
&resolution.requirements(),
reinstall,
editables,
reinstall,
&index_urls,
cache,
venv,
Expand Down
9 changes: 5 additions & 4 deletions crates/puffin-cli/src/commands/pip_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -92,8 +92,8 @@ pub(crate) async fn sync_requirements(
extraneous,
} = InstallPlan::from_requirements(
requirements,
editable_requirements,
reinstall,
editables,
&index_urls,
cache,
&venv,
Expand All @@ -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()
Expand Down
42 changes: 34 additions & 8 deletions crates/puffin-cli/tests/pip_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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");
Expand All @@ -2106,14 +2105,15 @@ fn install_editable() -> Result<()> {
"
})?;

// Install the editable packages.
let filter_path = requirements_txt.display().to_string();
let filters = INSTA_FILTERS
.iter()
.chain(&[(filter_path.as_str(), "requirements.txt")])
.copied()
.collect::<Vec<_>>();
insta::with_settings!({
filters => filters
filters => filters.clone()
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.arg("pip-sync")
Expand All @@ -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"
Expand All @@ -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
Expand All @@ -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
Expand All @@ -2194,7 +2220,7 @@ fn install_editable() -> Result<()> {
----- stdout -----
----- stderr -----
Audited 2 packages in [TIME]
Audited 4 packages in [TIME]
"###);
});

Expand Down
2 changes: 1 addition & 1 deletion crates/puffin-dispatch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ impl BuildContext for BuildDispatch {
editables: _,
} = InstallPlan::from_requirements(
&resolution.requirements(),
&Reinstall::None,
&[],
&Reinstall::None,
&self.index_urls,
self.cache(),
venv,
Expand Down
27 changes: 23 additions & 4 deletions crates/puffin-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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());
}
}
}

Expand Down

0 comments on commit 77c6e6f

Please sign in to comment.