diff --git a/crates/distribution-types/src/resolution.rs b/crates/distribution-types/src/resolution.rs index 65e69f5df..6557e5d5e 100644 --- a/crates/distribution-types/src/resolution.rs +++ b/crates/distribution-types/src/resolution.rs @@ -2,7 +2,6 @@ use rustc_hash::FxHashMap; use pep508_rs::Requirement; use puffin_normalize::PackageName; -use requirements_txt::EditableRequirement; use crate::{BuiltDist, Dist, PathSourceDist, SourceDist}; @@ -60,31 +59,6 @@ impl Resolution { requirements.sort_unstable_by(|a, b| a.name.cmp(&b.name)); requirements } - - /// Return the set of [`EditableRequirement`]s that this resolution represents. - pub fn editable_requirements(&self) -> Vec { - let mut requirements = self - .0 - .values() - .filter_map(|dist| { - let Dist::Source(SourceDist::Path(PathSourceDist { - url, - path, - editable: true, - .. - })) = dist - else { - return None; - }; - Some(EditableRequirement::Path { - path: path.clone(), - url: url.clone(), - }) - }) - .collect::>(); - requirements.sort_unstable_by(|a, b| a.url().cmp(b.url())); - requirements - } } impl From for Requirement { diff --git a/crates/puffin-cli/src/commands/pip_compile.rs b/crates/puffin-cli/src/commands/pip_compile.rs index 7d3125fe2..b2cc78b08 100644 --- a/crates/puffin-cli/src/commands/pip_compile.rs +++ b/crates/puffin-cli/src/commands/pip_compile.rs @@ -114,9 +114,6 @@ pub(crate) async fn pip_compile( .map(|spec| spec.requirements) .unwrap_or_default(); - // Create a manifest of the requirements. - let options = ResolutionOptions::new(resolution_mode, prerelease_mode, exclude_newer); - // Detect the current Python interpreter. let platform = Platform::current()?; let venv = Virtualenv::from_env(platform, &cache)?; @@ -127,13 +124,9 @@ pub(crate) async fn pip_compile( venv.python_executable().display() ); - // Determine the compatible platform tags. - let tags = Tags::from_interpreter(venv.interpreter())?; - - // Determine the interpreter to use for resolution. + // Determine the tags, markers, and interpreter to use for resolution. let interpreter = venv.interpreter().clone(); - - // Determine the markers to use for resolution. + let tags = Tags::from_interpreter(venv.interpreter())?; let markers = python_version.map_or_else( || Cow::Borrowed(venv.interpreter().markers()), |python_version| Cow::Owned(python_version.markers(venv.interpreter().markers())), @@ -144,6 +137,7 @@ pub(crate) async fn pip_compile( .index_urls(index_urls.clone()) .build(); + let options = ResolutionOptions::new(resolution_mode, prerelease_mode, exclude_newer); let build_dispatch = BuildDispatch::new( client.clone(), cache.clone(), @@ -203,6 +197,7 @@ pub(crate) async fn pip_compile( editable_metadata }; + // Create a manifest of the requirements. let manifest = Manifest::new( requirements, constraints, diff --git a/crates/puffin-cli/src/commands/pip_install.rs b/crates/puffin-cli/src/commands/pip_install.rs index 84849b684..a722152a9 100644 --- a/crates/puffin-cli/src/commands/pip_install.rs +++ b/crates/puffin-cli/src/commands/pip_install.rs @@ -18,9 +18,10 @@ use puffin_cache::Cache; use puffin_client::{RegistryClient, RegistryClientBuilder}; use puffin_dispatch::BuildDispatch; use puffin_installer::{ - BuiltEditable, Downloader, EditableMode, InstallPlan, Reinstall, SitePackages, + BuiltEditable, Downloader, InstallPlan, Reinstall, ResolvedEditable, SitePackages, }; use puffin_interpreter::Virtualenv; +use puffin_normalize::PackageName; use puffin_resolver::{ Manifest, PreReleaseMode, ResolutionGraph, ResolutionMode, ResolutionOptions, Resolver, }; @@ -62,8 +63,32 @@ pub(crate) async fn pip_install( let start = std::time::Instant::now(); - // Determine the requirements. - let spec = specification(requirements, constraints, overrides, extras)?; + // Read all requirements from the provided sources. + let RequirementsSpecification { + project, + requirements, + constraints, + overrides, + editables, + extras: used_extras, + } = specification(requirements, constraints, overrides, extras)?; + + // Check that all provided extras are used + if let ExtrasSpecification::Some(extras) = extras { + let mut unused_extras = extras + .iter() + .filter(|extra| !used_extras.contains(extra)) + .collect::>(); + if !unused_extras.is_empty() { + unused_extras.sort_unstable(); + unused_extras.dedup(); + let s = if unused_extras.len() == 1 { "" } else { "s" }; + return Err(anyhow!( + "Requested extra{s} not found: {}", + unused_extras.iter().join(", ") + )); + } + } // Detect the current Python interpreter. let platform = Platform::current()?; @@ -73,11 +98,15 @@ pub(crate) async fn pip_install( venv.python_executable().display() ); + // Determine the set of installed packages. + let site_packages = + SitePackages::from_executable(&venv).context("Failed to list installed packages")?; + // If the requirements are already satisfied, we're done. Ideally, the resolver would be fast // enough to let us remove this check. But right now, for large environments, it's an order of // magnitude faster to validate the environment than to resolve the requirements. - if reinstall.is_none() && satisfied(&spec, &venv)? { - let num_requirements = spec.requirements.len() + spec.editables.len(); + if reinstall.is_none() && site_packages.satisfies(&requirements, &editables, &constraints)? { + let num_requirements = requirements.len() + editables.len(); let s = if num_requirements == 1 { "" } else { "s" }; writeln!( printer, @@ -92,13 +121,9 @@ pub(crate) async fn pip_install( return Ok(ExitStatus::Success); } - // Determine the compatible platform tags. - let tags = Tags::from_interpreter(venv.interpreter())?; - - // Determine the interpreter to use for resolution. + // Determine the tags, markers, and interpreter to use for resolution. let interpreter = venv.interpreter().clone(); - - // Determine the markers to use for resolution. + let tags = Tags::from_interpreter(venv.interpreter())?; let markers = venv.interpreter().markers(); // Instantiate a client. @@ -107,6 +132,7 @@ pub(crate) async fn pip_install( .build(); let options = ResolutionOptions::new(resolution_mode, prerelease_mode, exclude_newer); + let build_dispatch = BuildDispatch::new( client.clone(), cache.clone(), @@ -121,12 +147,12 @@ pub(crate) async fn pip_install( // installation, and should live for the duration of the command. If an editable is already // installed in the environment, we'll still re-build it here. let editable_wheel_dir; - let editables = if spec.editables.is_empty() { + let editables = if editables.is_empty() { vec![] } else { editable_wheel_dir = tempdir_in(venv.root())?; build_editables( - &spec.editables, + &editables, editable_wheel_dir.path(), &cache, &tags, @@ -139,15 +165,18 @@ pub(crate) async fn pip_install( // Resolve the requirements. let resolution = match resolve( - spec, + requirements, + constraints, + overrides, + project, &editables, + &site_packages, reinstall, &tags, markers, &client, &build_dispatch, options, - &venv, printer, ) .await @@ -168,7 +197,8 @@ pub(crate) async fn pip_install( // Sync the environment. install( &resolution, - &editables, + editables, + site_packages, reinstall, link_mode, index_urls, @@ -228,15 +258,6 @@ fn specification( Ok(spec) } -/// Returns `true` if the requirements are already satisfied. -fn satisfied(spec: &RequirementsSpecification, venv: &Virtualenv) -> Result { - Ok(SitePackages::from_executable(venv)?.satisfies( - &spec.requirements, - &spec.editables, - &spec.constraints, - )?) -} - /// Build a set of editable distributions. async fn build_editables( editables: &[EditableRequirement], @@ -290,36 +311,27 @@ async fn build_editables( /// Resolve a set of requirements, similar to running `pip-compile`. #[allow(clippy::too_many_arguments)] async fn resolve( - spec: RequirementsSpecification, + requirements: Vec, + constraints: Vec, + overrides: Vec, + project: Option, editables: &[BuiltEditable], + site_packages: &SitePackages<'_>, reinstall: &Reinstall, tags: &Tags, markers: &MarkerEnvironment, client: &RegistryClient, build_dispatch: &BuildDispatch, options: ResolutionOptions, - venv: &Virtualenv, mut printer: Printer, ) -> Result { let start = std::time::Instant::now(); - // Create a manifest of the requirements. - let RequirementsSpecification { - project, - requirements, - constraints, - overrides, - editables: _, - extras: _, - } = spec; - // Respect preferences from the existing environments. let preferences: Vec = match reinstall { Reinstall::All => vec![], - Reinstall::None => SitePackages::from_executable(venv)? - .requirements() - .collect(), - Reinstall::Packages(packages) => SitePackages::from_executable(venv)? + Reinstall::None => site_packages.requirements().collect(), + Reinstall::Packages(packages) => site_packages .requirements() .filter(|requirement| !packages.contains(&requirement.name)) .collect(), @@ -336,6 +348,7 @@ async fn resolve( }) .collect(); + // Create a manifest of the requirements. let manifest = Manifest::new( requirements, constraints, @@ -369,7 +382,8 @@ async fn resolve( #[allow(clippy::too_many_arguments)] async fn install( resolution: &Resolution, - built_editables: &[BuiltEditable], + built_editables: Vec, + site_packages: SitePackages<'_>, reinstall: &Reinstall, link_mode: LinkMode, index_urls: IndexUrls, @@ -384,26 +398,30 @@ async fn install( // Partition into those that should be linked from the cache (`local`), those that need to be // downloaded (`remote`), and those that should be removed (`extraneous`). + let requirements = resolution.requirements(); + let editables = built_editables + .into_iter() + .map(ResolvedEditable::Built) + .collect::>(); let InstallPlan { local, remote, reinstalls, - editables, extraneous: _, } = InstallPlan::from_requirements( - &resolution.requirements(), - &resolution.editable_requirements(), + &requirements, + editables, + site_packages, reinstall, &index_urls, cache, venv, tags, - EditableMode::Mutable, ) .context("Failed to determine installation plan")?; // Nothing to do. - if remote.is_empty() && local.is_empty() && reinstalls.is_empty() && editables.is_empty() { + if remote.is_empty() && local.is_empty() && reinstalls.is_empty() { let s = if resolution.len() == 1 { "" } else { "s" }; writeln!( printer, @@ -430,18 +448,6 @@ async fn install( }) .collect::>(); - // Map any local editable requirements back to those that were built ahead of time. - let built_editables = editables - .iter() - .map(|editable| { - let built_editable = built_editables - .iter() - .find(|built_editable| built_editable.editable.requirement == *editable) - .expect("Editable should be built"); - built_editable.wheel.clone() - }) - .collect::>(); - // Download, build, and unzip any missing distributions. let wheels = if remote.is_empty() { vec![] @@ -487,11 +493,7 @@ async fn install( } // Install the resolved distributions. - let wheels = wheels - .into_iter() - .chain(local) - .chain(built_editables) - .collect::>(); + let wheels = wheels.into_iter().chain(local).collect::>(); if !wheels.is_empty() { let start = std::time::Instant::now(); puffin_installer::Installer::new(venv) diff --git a/crates/puffin-cli/src/commands/pip_sync.rs b/crates/puffin-cli/src/commands/pip_sync.rs index ce39fda49..49bf63b2c 100644 --- a/crates/puffin-cli/src/commands/pip_sync.rs +++ b/crates/puffin-cli/src/commands/pip_sync.rs @@ -4,18 +4,16 @@ use anyhow::{bail, Context, Result}; use colored::Colorize; use fs_err as fs; use itertools::Itertools; -use tempfile::tempdir_in; use tracing::debug; -use distribution_types::{AnyDist, CachedDist, LocalEditable, Metadata}; +use distribution_types::{AnyDist, LocalEditable, Metadata}; use install_wheel_rs::linker::LinkMode; -use pep508_rs::Requirement; use platform_host::Platform; use platform_tags::Tags; use puffin_cache::Cache; -use puffin_client::RegistryClientBuilder; +use puffin_client::{RegistryClient, RegistryClientBuilder}; use puffin_dispatch::BuildDispatch; -use puffin_installer::{Downloader, EditableMode, InstallPlan, Reinstall, SitePackages}; +use puffin_installer::{Downloader, InstallPlan, Reinstall, ResolvedEditable, SitePackages}; use puffin_interpreter::Virtualenv; use puffin_traits::OnceMap; use pypi_types::{IndexUrls, Yanked}; @@ -27,6 +25,7 @@ use crate::printer::Printer; use crate::requirements::{RequirementsSource, RequirementsSpecification}; /// Install a set of locked requirements into the current Python environment. +#[allow(clippy::too_many_arguments)] pub(crate) async fn pip_sync( sources: &[RequirementsSource], reinstall: &Reinstall, @@ -36,44 +35,19 @@ pub(crate) async fn pip_sync( cache: Cache, mut printer: Printer, ) -> Result { + let start = std::time::Instant::now(); + // Read all requirements from the provided sources. let (requirements, editables) = RequirementsSpecification::requirements_and_editables(sources)?; - - if requirements.is_empty() && editables.is_empty() { + let num_requirements = requirements.len() + editables.len(); + if num_requirements == 0 { writeln!(printer, "No requirements found")?; return Ok(ExitStatus::Success); } - sync_requirements( - &requirements, - reinstall, - &editables, - link_mode, - index_urls, - no_build, - &cache, - printer, - ) - .await -} - -/// Install a set of locked requirements into the current Python environment. -#[allow(clippy::too_many_arguments)] -pub(crate) async fn sync_requirements( - requirements: &[Requirement], - reinstall: &Reinstall, - editable_requirements: &[EditableRequirement], - link_mode: LinkMode, - index_urls: IndexUrls, - no_build: bool, - cache: &Cache, - mut printer: Printer, -) -> Result { - let start = std::time::Instant::now(); - // Detect the current Python interpreter. let platform = Platform::current()?; - let venv = Virtualenv::from_env(platform, cache)?; + let venv = Virtualenv::from_env(platform, &cache)?; debug!( "Using Python interpreter: {}", venv.python_executable().display() @@ -82,34 +56,60 @@ pub(crate) async fn sync_requirements( // Determine the current environment markers. let tags = Tags::from_interpreter(venv.interpreter())?; + // Prep the registry client. + let client = RegistryClientBuilder::new(cache.clone()) + .index_urls(index_urls.clone()) + .build(); + + // Prep the build context. + let build_dispatch = BuildDispatch::new( + client.clone(), + cache.clone(), + venv.interpreter().clone(), + fs::canonicalize(venv.python_executable())?, + no_build, + index_urls.clone(), + ); + + // Determine the set of installed packages. + let site_packages = + SitePackages::from_executable(&venv).context("Failed to list installed packages")?; + + // Resolve any editables. + let resolved_editables = resolve_editables( + editables, + &site_packages, + reinstall, + &venv, + &tags, + &cache, + &client, + &build_dispatch, + printer, + ) + .await?; + // Partition into those that should be linked from the cache (`local`), those that need to be // downloaded (`remote`), and those that should be removed (`extraneous`). let InstallPlan { local, - editables, remote, reinstalls, extraneous, } = InstallPlan::from_requirements( - requirements, - editable_requirements, + &requirements, + resolved_editables.editables, + site_packages, reinstall, &index_urls, - cache, + &cache, &venv, &tags, - EditableMode::Immutable, ) .context("Failed to determine installation plan")?; // Nothing to do. - if remote.is_empty() - && local.is_empty() - && reinstalls.is_empty() - && extraneous.is_empty() - && editables.is_empty() - { - let num_requirements = requirements.len() + editable_requirements.len(); + if remote.is_empty() && local.is_empty() && reinstalls.is_empty() && extraneous.is_empty() { let s = if num_requirements == 1 { "" } else { "s" }; writeln!( printer, @@ -182,66 +182,15 @@ pub(crate) async fn sync_requirements( } } - let build_dispatch = BuildDispatch::new( - client.clone(), - cache.clone(), - venv.interpreter().clone(), - fs::canonicalize(venv.python_executable())?, - no_build, - index_urls.clone(), - ); - let downloader = Downloader::new(cache, &tags, &client, &build_dispatch).with_reporter( - DownloadReporter::from(printer).with_length((editables.len() + remote.len()) as u64), - ); - - // Build any editable requirements. - let editable_wheel_dir = tempdir_in(venv.root())?; - let built_editables = if editables.is_empty() { - Vec::new() - } else { - let start = std::time::Instant::now(); - - let editables: Vec = editables - .into_iter() - .map(|editable| match editable.clone() { - EditableRequirement::Path { path, .. } => Ok(LocalEditable { - requirement: editable, - path, - }), - EditableRequirement::Url(_) => { - bail!("url editables are not supported yet"); - } - }) - .collect::>()?; - - let built_editables: Vec = downloader - .build_editables(editables, editable_wheel_dir.path()) - .await - .context("Failed to build editables")? - .into_iter() - .map(|built_editable| built_editable.wheel) - .collect(); - let s = if built_editables.len() == 1 { "" } else { "s" }; - writeln!( - printer, - "{}", - format!( - "Built {} in {}", - format!("{} editable{}", built_editables.len(), s).bold(), - elapsed(start.elapsed()) - ) - .dimmed() - )?; - - built_editables - }; - // Download, build, and unzip any missing distributions. let wheels = if remote.is_empty() { Vec::new() } else { let start = std::time::Instant::now(); + let downloader = Downloader::new(&cache, &tags, &client, &build_dispatch) + .with_reporter(DownloadReporter::from(printer).with_length(remote.len() as u64)); + let wheels = downloader .download(remote, &OnceMap::default()) .await @@ -296,11 +245,7 @@ pub(crate) async fn sync_requirements( } // Install the resolved distributions. - let wheels = wheels - .into_iter() - .chain(local) - .chain(built_editables) - .collect::>(); + let wheels = wheels.into_iter().chain(local).collect::>(); if !wheels.is_empty() { let start = std::time::Instant::now(); puffin_installer::Installer::new(&venv) @@ -376,3 +321,111 @@ pub(crate) async fn sync_requirements( Ok(ExitStatus::Success) } + +#[derive(Debug)] +struct ResolvedEditables { + /// The set of resolved editables, including both those that were already installed and those + /// that were built. + editables: Vec, + /// The temporary directory in which the built editables were stored. + #[allow(dead_code)] + temp_dir: Option, +} + +/// Resolve the set of editables that need to be installed. +#[allow(clippy::too_many_arguments)] +async fn resolve_editables( + editables: Vec, + site_packages: &SitePackages<'_>, + reinstall: &Reinstall, + venv: &Virtualenv, + tags: &Tags, + cache: &Cache, + client: &RegistryClient, + build_dispatch: &BuildDispatch, + mut printer: Printer, +) -> Result { + // Partition the editables into those that are already installed, and those that must be built. + let mut installed = Vec::with_capacity(editables.len()); + let mut uninstalled = Vec::with_capacity(editables.len()); + for editable in editables { + match reinstall { + Reinstall::None => { + if let Some(dist) = site_packages.get_editable(editable.raw()) { + installed.push(dist.clone()); + } else { + uninstalled.push(editable); + } + } + Reinstall::All => { + uninstalled.push(editable); + } + Reinstall::Packages(packages) => { + if let Some(dist) = site_packages.get_editable(editable.raw()) { + if packages.contains(dist.name()) { + uninstalled.push(editable); + } else { + installed.push(dist.clone()); + } + } else { + uninstalled.push(editable); + } + } + } + } + + // Build any editable installs. + let (built_editables, temp_dir) = if uninstalled.is_empty() { + (Vec::new(), None) + } else { + let start = std::time::Instant::now(); + + let temp_dir = tempfile::tempdir_in(venv.root())?; + + let downloader = Downloader::new(cache, tags, client, build_dispatch) + .with_reporter(DownloadReporter::from(printer).with_length(uninstalled.len() as u64)); + + let local_editables: Vec = uninstalled + .iter() + .map(|editable| match editable { + EditableRequirement::Path { path, .. } => Ok(LocalEditable { + requirement: editable.clone(), + path: path.clone(), + }), + EditableRequirement::Url(_) => { + bail!("Editable installs for URLs are not yet supported"); + } + }) + .collect::>()?; + + let built_editables: Vec<_> = downloader + .build_editables(local_editables, temp_dir.path()) + .await + .context("Failed to build editables")? + .into_iter() + .collect(); + + let s = if built_editables.len() == 1 { "" } else { "s" }; + writeln!( + printer, + "{}", + format!( + "Built {} in {}", + format!("{} editable{}", built_editables.len(), s).bold(), + elapsed(start.elapsed()) + ) + .dimmed() + )?; + + (built_editables, Some(temp_dir)) + }; + + Ok(ResolvedEditables { + editables: installed + .into_iter() + .map(ResolvedEditable::Installed) + .chain(built_editables.into_iter().map(ResolvedEditable::Built)) + .collect::>(), + temp_dir, + }) +} diff --git a/crates/puffin-cli/tests/pip_install.rs b/crates/puffin-cli/tests/pip_install.rs index 1b1b7722d..583fb8a93 100644 --- a/crates/puffin-cli/tests/pip_install.rs +++ b/crates/puffin-cli/tests/pip_install.rs @@ -550,3 +550,117 @@ fn install_editable() -> Result<()> { Ok(()) } + +#[test] +fn install_editable_and_registry() -> Result<()> { + let temp_dir = assert_fs::TempDir::new()?; + let cache_dir = assert_fs::TempDir::new()?; + let venv = create_venv_py312(&temp_dir, &cache_dir); + + // Install the registry-based version of Black. + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-install") + .arg("black") + .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 ----- + Resolved 13 packages in [TIME] + Downloaded 13 packages in [TIME] + Installed 13 packages in [TIME] + + aiohttp==3.9.1 + + aiosignal==1.3.1 + + attrs==23.1.0 + + black==23.12.0 + + click==8.1.7 + + frozenlist==1.4.1 + + idna==3.6 + + multidict==6.0.4 + + mypy-extensions==1.0.0 + + packaging==23.2 + + pathspec==0.12.1 + + platformdirs==4.1.0 + + yarl==1.9.4 + "###); + }); + + // Install the editable version of Black. This should remove the registry-based version. + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-install") + .arg("-e") + .arg("../../scripts/editable-installs/black_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] + Resolved 1 package in [TIME] + Installed 1 package in [TIME] + - black==23.12.0 + + black @ ../../scripts/editable-installs/black_editable + "###); + }); + + // Re-install the registry-based version of Black. This should be a no-op, since we have a + // version of Black installed (the editable version) that satisfies the requirements. + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-install") + .arg("black") + .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 ----- + Audited 1 package in [TIME] + "###); + }); + + // Re-install Black at a specific version. This should replace the editable version. + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-install") + .arg("black==23.10.0") + .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 ----- + Resolved 6 packages in [TIME] + Downloaded 1 package in [TIME] + Installed 1 package in [TIME] + - black==0.1.0 + + black==23.10.0 + "###); + }); + + Ok(()) +} diff --git a/crates/puffin-cli/tests/pip_sync.rs b/crates/puffin-cli/tests/pip_sync.rs index 5c2f2d5fe..003c979d5 100644 --- a/crates/puffin-cli/tests/pip_sync.rs +++ b/crates/puffin-cli/tests/pip_sync.rs @@ -1903,9 +1903,7 @@ fn duplicate_package_overlap() -> Result<()> { ----- stderr ----- error: Failed to determine installation plan - Caused by: Detected duplicate package in requirements: - markupsafe ==2.1.2 - markupsafe ==2.1.3 + Caused by: Detected duplicate package in requirements: markupsafe "###); }); @@ -2127,8 +2125,8 @@ fn sync_editable() -> Result<()> { ----- stdout ----- ----- stderr ----- - Resolved 2 packages in [TIME] Built 2 editables in [TIME] + Resolved 2 packages in [TIME] Downloaded 2 packages in [TIME] Installed 4 packages in [TIME] + boltons==23.1.1 @@ -2226,3 +2224,164 @@ fn sync_editable() -> Result<()> { Ok(()) } + +#[test] +fn sync_editable_and_registry() -> Result<()> { + let temp_dir = assert_fs::TempDir::new()?; + let cache_dir = assert_fs::TempDir::new()?; + let venv = create_venv_py312(&temp_dir, &cache_dir); + + // Install the registry-based version of Black. + let requirements_txt = temp_dir.child("requirements.txt"); + requirements_txt.write_str(indoc! {r" + black + " + })?; + + let filter_path = requirements_txt.display().to_string(); + let filters = INSTA_FILTERS + .iter() + .chain(&[(filter_path.as_str(), "requirements.txt")]) + .copied() + .collect::>(); + insta::with_settings!({ + filters => filters.clone() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-sync") + .arg(requirements_txt.path()) + .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 ----- + Resolved 1 package in [TIME] + Downloaded 1 package in [TIME] + Installed 1 package in [TIME] + + black==24.1a1 + warning: The package `black` requires `click >=8.0.0`, but it's not installed. + warning: The package `black` requires `mypy-extensions >=0.4.3`, but it's not installed. + warning: The package `black` requires `packaging >=22.0`, but it's not installed. + warning: The package `black` requires `pathspec >=0.9.0`, but it's not installed. + warning: The package `black` requires `platformdirs >=2`, but it's not installed. + warning: The package `black` requires `aiohttp >=3.7.4 ; sys_platform != 'win32' or (implementation_name != 'pypy' and extra == 'd')`, but it's not installed. + "###); + }); + + // Install the editable version of Black. This should remove the registry-based version. + let requirements_txt = temp_dir.child("requirements.txt"); + requirements_txt.write_str(indoc! {r" + -e ../../scripts/editable-installs/black_editable + " + })?; + + let filter_path = requirements_txt.display().to_string(); + let filters = INSTA_FILTERS + .iter() + .chain(&[(filter_path.as_str(), "requirements.txt")]) + .copied() + .collect::>(); + insta::with_settings!({ + filters => filters.clone() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-sync") + .arg(requirements_txt.path()) + .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] + - black==24.1a1 + + black @ ../../scripts/editable-installs/black_editable + "###); + }); + + // Re-install the registry-based version of Black. This should be a no-op, since we have a + // version of Black installed (the editable version) that satisfies the requirements. + let requirements_txt = temp_dir.child("requirements.txt"); + requirements_txt.write_str(indoc! {r" + black + " + })?; + + let filter_path = requirements_txt.display().to_string(); + let filters = INSTA_FILTERS + .iter() + .chain(&[(filter_path.as_str(), "requirements.txt")]) + .copied() + .collect::>(); + insta::with_settings!({ + filters => filters.clone() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-sync") + .arg(requirements_txt.path()) + .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 ----- + Audited 1 package in [TIME] + "###); + }); + + // Re-install Black at a specific version. This should replace the editable version. + let requirements_txt = temp_dir.child("requirements.txt"); + requirements_txt.write_str(indoc! {r" + black==23.10.0 + " + })?; + + let filter_path = requirements_txt.display().to_string(); + let filters = INSTA_FILTERS + .iter() + .chain(&[(filter_path.as_str(), "requirements.txt")]) + .copied() + .collect::>(); + insta::with_settings!({ + filters => filters.clone() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-sync") + .arg(requirements_txt.path()) + .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 ----- + Resolved 1 package in [TIME] + Downloaded 1 package in [TIME] + Uninstalled 1 package in [TIME] + Installed 1 package in [TIME] + - black==0.1.0 + + black==23.10.0 + warning: The package `black` requires `click >=8.0.0`, but it's not installed. + warning: The package `black` requires `mypy-extensions >=0.4.3`, but it's not installed. + warning: The package `black` requires `packaging >=22.0`, but it's not installed. + warning: The package `black` requires `pathspec >=0.9.0`, but it's not installed. + warning: The package `black` requires `platformdirs >=2`, but it's not installed. + "###); + }); + + Ok(()) +} diff --git a/crates/puffin-client/src/registry_client.rs b/crates/puffin-client/src/registry_client.rs index 2a7d8473b..00f34258e 100644 --- a/crates/puffin-client/src/registry_client.rs +++ b/crates/puffin-client/src/registry_client.rs @@ -100,7 +100,7 @@ impl RegistryClientBuilder { } /// A client for fetching packages from a `PyPI`-compatible index. -// TODO(konstin): Clean up the clients once we moved everything to common caching +// TODO(konstin): Clean up the clients once we moved everything to common caching. #[derive(Debug, Clone)] pub struct RegistryClient { pub(crate) index_urls: IndexUrls, diff --git a/crates/puffin-dispatch/src/lib.rs b/crates/puffin-dispatch/src/lib.rs index 07f340145..407793534 100644 --- a/crates/puffin-dispatch/src/lib.rs +++ b/crates/puffin-dispatch/src/lib.rs @@ -16,7 +16,7 @@ use platform_tags::Tags; use puffin_build::{SourceBuild, SourceBuildContext}; use puffin_cache::Cache; use puffin_client::RegistryClient; -use puffin_installer::{Downloader, EditableMode, InstallPlan, Installer, Reinstall}; +use puffin_installer::{Downloader, InstallPlan, Installer, Reinstall, SitePackages}; use puffin_interpreter::{Interpreter, Virtualenv}; use puffin_resolver::{Manifest, ResolutionOptions, Resolver}; use puffin_traits::{BuildContext, BuildKind, OnceMap}; @@ -131,23 +131,27 @@ impl BuildContext for BuildDispatch { venv.root().display(), ); + // Determine the current environment markers. let tags = Tags::from_interpreter(&self.interpreter)?; + // Determine the set of installed packages. + let site_packages = + SitePackages::from_executable(venv).context("Failed to list installed packages")?; + let InstallPlan { local, remote, reinstalls, extraneous, - editables: _, } = InstallPlan::from_requirements( &resolution.requirements(), - &[], + Vec::new(), + site_packages, &Reinstall::None, &self.index_urls, self.cache(), venv, &tags, - EditableMode::default(), )?; // Resolve any registry-based requirements. diff --git a/crates/puffin-installer/src/editable.rs b/crates/puffin-installer/src/editable.rs index 194611d31..17b74734b 100644 --- a/crates/puffin-installer/src/editable.rs +++ b/crates/puffin-installer/src/editable.rs @@ -1,9 +1,59 @@ -use distribution_types::{CachedDist, LocalEditable}; +use distribution_types::{CachedDist, InstalledDist, LocalEditable, Metadata, VersionOrUrl}; +use puffin_normalize::PackageName; use pypi_types::Metadata21; +/// An editable distribution that has been built. #[derive(Debug, Clone)] pub struct BuiltEditable { pub editable: LocalEditable, pub wheel: CachedDist, pub metadata: Metadata21, } + +/// An editable distribution that has been resolved to a concrete distribution. +#[derive(Debug, Clone)] +#[allow(clippy::large_enum_variant)] +pub enum ResolvedEditable { + /// The editable is already installed in the environment. + Installed(InstalledDist), + /// The editable has been built and is ready to be installed. + Built(BuiltEditable), +} + +impl Metadata for BuiltEditable { + fn name(&self) -> &PackageName { + &self.metadata.name + } + + fn version_or_url(&self) -> VersionOrUrl { + VersionOrUrl::Version(&self.metadata.version) + } +} + +impl Metadata for ResolvedEditable { + fn name(&self) -> &PackageName { + match self { + Self::Installed(dist) => dist.name(), + Self::Built(dist) => dist.name(), + } + } + + fn version_or_url(&self) -> VersionOrUrl { + match self { + Self::Installed(dist) => dist.version_or_url(), + Self::Built(dist) => dist.version_or_url(), + } + } +} + +impl std::fmt::Display for BuiltEditable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}{}", self.name(), self.version_or_url()) + } +} + +impl std::fmt::Display for ResolvedEditable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}{}", self.name(), self.version_or_url()) + } +} diff --git a/crates/puffin-installer/src/lib.rs b/crates/puffin-installer/src/lib.rs index 54a75da20..1fc7462ed 100644 --- a/crates/puffin-installer/src/lib.rs +++ b/crates/puffin-installer/src/lib.rs @@ -1,7 +1,7 @@ pub use downloader::{Downloader, Reporter as DownloadReporter}; -pub use editable::BuiltEditable; +pub use editable::{BuiltEditable, ResolvedEditable}; pub use installer::{Installer, Reporter as InstallReporter}; -pub use plan::{EditableMode, InstallPlan, Reinstall}; +pub use plan::{InstallPlan, Reinstall}; pub use site_packages::SitePackages; pub use uninstall::uninstall; diff --git a/crates/puffin-installer/src/plan.rs b/crates/puffin-installer/src/plan.rs index cf4304b89..2f65eea07 100644 --- a/crates/puffin-installer/src/plan.rs +++ b/crates/puffin-installer/src/plan.rs @@ -1,8 +1,8 @@ use std::hash::BuildHasherDefault; -use anyhow::{bail, Context, Result}; -use rustc_hash::FxHashMap; -use tracing::debug; +use anyhow::{bail, Result}; +use rustc_hash::FxHashSet; +use tracing::{debug, warn}; use distribution_types::direct_url::{git_reference, DirectUrl}; use distribution_types::{BuiltDist, Dist, SourceDist}; @@ -14,9 +14,8 @@ use puffin_distribution::{BuiltWheelIndex, RegistryWheelIndex}; use puffin_interpreter::Virtualenv; use puffin_normalize::PackageName; use pypi_types::IndexUrls; -use requirements_txt::EditableRequirement; -use crate::SitePackages; +use crate::{ResolvedEditable, SitePackages}; #[derive(Debug, Default)] pub struct InstallPlan { @@ -35,12 +34,6 @@ pub struct InstallPlan { /// Any distributions that are already installed in the current environment, and are /// _not_ necessary to satisfy the requirements. pub extraneous: Vec, - - /// Editable installs that are missing in the current environment. - /// - /// Since editable installs happen from a path through a non-cacheable wheel, we don't have to - /// divide those into cached and non-cached. - pub editables: Vec, } impl InstallPlan { @@ -49,18 +42,14 @@ impl InstallPlan { #[allow(clippy::too_many_arguments)] pub fn from_requirements( requirements: &[Requirement], - editable_requirements: &[EditableRequirement], + editable_requirements: Vec, + mut site_packages: SitePackages, reinstall: &Reinstall, index_urls: &IndexUrls, cache: &Cache, venv: &Virtualenv, tags: &Tags, - editable_mode: EditableMode, ) -> Result { - // Index all the already-installed packages in site-packages. - let mut site_packages = - SitePackages::from_executable(venv).context("Failed to list installed packages")?; - // Index all the already-downloaded wheels in the cache. let mut registry_index = RegistryWheelIndex::new(cache, tags, index_urls); @@ -68,43 +57,44 @@ impl InstallPlan { let mut remote = vec![]; let mut reinstalls = vec![]; let mut extraneous = vec![]; - let mut editables = vec![]; let mut seen = - FxHashMap::with_capacity_and_hasher(requirements.len(), BuildHasherDefault::default()); + FxHashSet::with_capacity_and_hasher(requirements.len(), BuildHasherDefault::default()); // Remove any editable requirements. - for editable in editable_requirements { - // 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())), - }; + for requirement in editable_requirements { + // If we see the same requirement twice, then we have a conflict. + if !seen.insert(requirement.name().clone()) { + bail!( + "Detected duplicate package in requirements: {}", + requirement.name() + ); + } - if reinstall { - if let Some(distribution) = site_packages.remove_editable(editable.raw()) { - reinstalls.push(distribution); - } - editables.push(editable.clone()); - } else { - if let Some(dist) = site_packages.remove_editable(editable.raw()) { - match editable_mode { - EditableMode::Immutable => { - debug!("Treating editable requirement as immutable: {editable}"); - } - EditableMode::Mutable => { - debug!("Treating editable requirement as mutable: {editable}"); - reinstalls.push(dist); - editables.push(editable.clone()); - } + match requirement { + ResolvedEditable::Installed(installed) => { + debug!("Treating editable requirement as immutable: {installed}"); + + // Remove from the site-packages index, to avoid marking as extraneous. + let Some(editable) = installed.editable() else { + warn!("Editable requirement is not editable: {installed}"); + continue; + }; + if site_packages.remove_editable(editable).is_none() { + warn!("Editable requirement is not installed: {installed}"); + continue; } - } else { - editables.push(editable.clone()); + } + ResolvedEditable::Built(built) => { + debug!("Treating editable requirement as mutable: {built}"); + + if let Some(dist) = site_packages.remove_editable(built.editable.raw()) { + // Remove any editable installs. + reinstalls.push(dist); + } else if let Some(dist) = site_packages.remove(built.name()) { + // Remove any non-editable installs of the same package. + reinstalls.push(dist); + } + local.push(built.wheel); } } } @@ -116,8 +106,11 @@ impl InstallPlan { } // If we see the same requirement twice, then we have a conflict. - if let Some(existing) = seen.insert(requirement.name.clone(), requirement) { - bail!("Detected duplicate package in requirements:\n {requirement}\n {existing}"); + if !seen.insert(requirement.name.clone()) { + bail!( + "Detected duplicate package in requirements: {}", + requirement.name + ); } // Check if the package should be reinstalled. A reinstall involves (1) purging any @@ -328,7 +321,6 @@ impl InstallPlan { remote, reinstalls, extraneous, - editables, }) } } @@ -362,13 +354,3 @@ impl Reinstall { matches!(self, Self::None) } } - -#[derive(Debug, Default, Copy, Clone)] -pub enum EditableMode { - /// Assume that editables are immutable, such that they're left untouched if already present - /// in the environment. - #[default] - Immutable, - /// Assume that editables are mutable, such that they're always reinstalled. - Mutable, -} diff --git a/scripts/editable-installs/black_editable/black/__init__.py b/scripts/editable-installs/black_editable/black/__init__.py new file mode 100644 index 000000000..b2dde251b --- /dev/null +++ b/scripts/editable-installs/black_editable/black/__init__.py @@ -0,0 +1,2 @@ +def a(): + pass diff --git a/scripts/editable-installs/black_editable/pyproject.toml b/scripts/editable-installs/black_editable/pyproject.toml new file mode 100644 index 000000000..ff10712d5 --- /dev/null +++ b/scripts/editable-installs/black_editable/pyproject.toml @@ -0,0 +1,12 @@ +[tool.poetry] +name = "black" +version = "0.1.0" +description = "" +authors = ["konstin "] + +[tool.poetry.dependencies] +python = "^3.10" + +[build-system] +requires = ["poetry-core"] +build-backend = "poetry.core.masonry.api"