From d47ded75d21b2a6c61b4e4436741de554d8c0f08 Mon Sep 17 00:00:00 2001 From: "Tomasz (Tom) Kramkowski" Date: Mon, 8 Dec 2025 17:08:01 +0000 Subject: [PATCH] Refactor dry run reporting Remove the reporter by rolling the functionality into the normal flow. This approach has the side effect of eagerly preparing packages. --- crates/uv/src/commands/mod.rs | 10 +- crates/uv/src/commands/pip/loggers.rs | 27 +-- crates/uv/src/commands/pip/operations.rs | 222 ++++++----------------- crates/uv/tests/it/lock_conflict.rs | 2 +- crates/uv/tests/it/pip_install.rs | 32 ++-- crates/uv/tests/it/pip_sync.rs | 8 +- crates/uv/tests/it/sync.rs | 16 +- 7 files changed, 94 insertions(+), 223 deletions(-) diff --git a/crates/uv/src/commands/mod.rs b/crates/uv/src/commands/mod.rs index 72e9daa3e..1c453a0b3 100644 --- a/crates/uv/src/commands/mod.rs +++ b/crates/uv/src/commands/mod.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use std::io::stdout; use std::path::{Path, PathBuf}; use std::time::Duration; -use std::{fmt::Display, fmt::Write, process::ExitCode}; +use std::{fmt::Write, process::ExitCode}; use anstream::AutoStream; use anyhow::Context; @@ -65,7 +65,6 @@ pub(crate) use uv_console::human_readable_bytes; use uv_distribution_types::InstalledMetadata; use uv_fs::{CWD, Simplified}; use uv_installer::compile_tree; -use uv_normalize::PackageName; use uv_python::PythonEnvironment; use uv_scripts::Pep723Script; pub(crate) use venv::venv; @@ -153,13 +152,6 @@ pub(super) struct ChangeEvent<'a, T: InstalledMetadata> { kind: ChangeEventKind, } -#[derive(Debug)] -pub(super) struct DryRunEvent { - name: PackageName, - version: T, - kind: ChangeEventKind, -} - /// Compile all Python source files in site-packages to bytecode, to speed up the /// initial run of any subsequent executions. /// diff --git a/crates/uv/src/commands/pip/loggers.rs b/crates/uv/src/commands/pip/loggers.rs index 777efc842..c6dcdef62 100644 --- a/crates/uv/src/commands/pip/loggers.rs +++ b/crates/uv/src/commands/pip/loggers.rs @@ -33,7 +33,6 @@ pub(crate) trait InstallLogger { suffix: Option<&str>, start: std::time::Instant, printer: Printer, - dry_run: DryRun, ) -> fmt::Result; /// Log the completion of the uninstallation phase. @@ -101,26 +100,20 @@ impl InstallLogger for DefaultInstallLogger { suffix: Option<&str>, start: std::time::Instant, printer: Printer, - dry_run: DryRun, ) -> fmt::Result { let s = if count == 1 { "" } else { "s" }; - let what = if let Some(suffix) = suffix { - format!("{count} package{s} {suffix}") - } else { - format!("{count} package{s}") - }; - let what = what.bold(); writeln!( printer.stderr(), "{}", - if dry_run.enabled() { - format!("Would download {what}") - } else { - format!( - "Prepared {what} {}", - format!("in {}", elapsed(start.elapsed())).dimmed() - ) - } + format!( + "Prepared {} {}", + if let Some(suffix) = suffix { + format!("{count} package{s} {suffix}") + } else { + format!("{count} package{s}") + }, + format!("in {}", elapsed(start.elapsed())).dimmed() + ) .dimmed() ) } @@ -265,7 +258,6 @@ impl InstallLogger for SummaryInstallLogger { _suffix: Option<&str>, _start: std::time::Instant, _printer: Printer, - _dry_run: DryRun, ) -> fmt::Result { Ok(()) } @@ -330,7 +322,6 @@ impl InstallLogger for UpgradeInstallLogger { _suffix: Option<&str>, _start: std::time::Instant, _printer: Printer, - _dry_run: DryRun, ) -> fmt::Result { Ok(()) } diff --git a/crates/uv/src/commands/pip/operations.rs b/crates/uv/src/commands/pip/operations.rs index 64e03dfe9..05ea7bf1d 100644 --- a/crates/uv/src/commands/pip/operations.rs +++ b/crates/uv/src/commands/pip/operations.rs @@ -22,7 +22,7 @@ use uv_distribution_types::{ CachedDist, Diagnostic, InstalledDist, LocalDist, NameRequirementSpecification, Requirement, ResolutionDiagnostic, UnresolvedRequirement, UnresolvedRequirementSpecification, }; -use uv_distribution_types::{DistributionMetadata, InstalledMetadata, Name, Resolution}; +use uv_distribution_types::{Name, Resolution}; use uv_fs::Simplified; use uv_install_wheel::LinkMode; use uv_installer::{InstallationStrategy, Plan, Planner, Preparer, SitePackages}; @@ -44,9 +44,9 @@ use uv_tool::InstalledTools; use uv_types::{BuildContext, HashStrategy, InFlight, InstalledPackagesProvider}; use uv_warnings::warn_user; -use crate::commands::pip::loggers::{DefaultInstallLogger, InstallLogger, ResolveLogger}; +use crate::commands::compile_bytecode; +use crate::commands::pip::loggers::{InstallLogger, ResolveLogger}; use crate::commands::reporters::{InstallReporter, PrepareReporter, ResolverReporter}; -use crate::commands::{ChangeEventKind, DryRunEvent, compile_bytecode}; use crate::printer::Printer; /// Consolidate the requirements for an installation. @@ -484,11 +484,6 @@ pub(crate) async fn install( ) .context("Failed to determine installation plan")?; - if dry_run.enabled() { - report_dry_run(dry_run, resolution, plan, modifications, start, printer)?; - return Ok(Changelog::default()); - } - let Plan { cached, remote, @@ -548,6 +543,7 @@ pub(crate) async fn install( venv, logger.as_ref(), installer_metadata, + dry_run, printer, preview, ) @@ -577,6 +573,7 @@ pub(crate) async fn install( venv, logger.as_ref(), installer_metadata, + dry_run, printer, preview, ) @@ -595,6 +592,10 @@ pub(crate) async fn install( // Notify the user of any environment modifications. logger.on_complete(&changelog, printer, dry_run)?; + if matches!(dry_run, DryRun::Check) { + return Err(Error::OutdatedEnvironment); + } + Ok(changelog) } @@ -629,6 +630,7 @@ async fn execute_plan( venv: &PythonEnvironment, logger: &dyn InstallLogger, installer_metadata: bool, + dry_run: DryRun, printer: Printer, preview: Preview, ) -> Result<(Vec, Vec), Error> { @@ -660,13 +662,7 @@ async fn execute_plan( .prepare(remote.clone(), in_flight, resolution) .await?; - logger.on_prepare( - wheels.len(), - phase.map(InstallPhase::label), - start, - printer, - DryRun::Disabled, - )?; + logger.on_prepare(wheels.len(), phase.map(InstallPhase::label), start, printer)?; wheels }; @@ -676,58 +672,62 @@ async fn execute_plan( if !uninstalls.is_empty() { let start = std::time::Instant::now(); - for dist_info in &uninstalls { - match uv_installer::uninstall(dist_info).await { - Ok(summary) => { - debug!( - "Uninstalled {} ({} file{}, {} director{})", - dist_info.name(), - summary.file_count, - if summary.file_count == 1 { "" } else { "s" }, - summary.dir_count, - if summary.dir_count == 1 { "y" } else { "ies" }, - ); + if !dry_run.enabled() { + for dist_info in &uninstalls { + match uv_installer::uninstall(dist_info).await { + Ok(summary) => { + debug!( + "Uninstalled {} ({} file{}, {} director{})", + dist_info.name(), + summary.file_count, + if summary.file_count == 1 { "" } else { "s" }, + summary.dir_count, + if summary.dir_count == 1 { "y" } else { "ies" }, + ); + } + Err(uv_installer::UninstallError::Uninstall( + uv_install_wheel::Error::MissingRecord(_), + )) => { + warn_user!( + "Failed to uninstall package at {} due to missing `RECORD` file. Installation may result in an incomplete environment.", + dist_info.install_path().user_display().cyan(), + ); + } + Err(uv_installer::UninstallError::Uninstall( + uv_install_wheel::Error::MissingTopLevel(_), + )) => { + warn_user!( + "Failed to uninstall package at {} due to missing `top_level.txt` file. Installation may result in an incomplete environment.", + dist_info.install_path().user_display().cyan(), + ); + } + Err(err) => return Err(err.into()), } - Err(uv_installer::UninstallError::Uninstall( - uv_install_wheel::Error::MissingRecord(_), - )) => { - warn_user!( - "Failed to uninstall package at {} due to missing `RECORD` file. Installation may result in an incomplete environment.", - dist_info.install_path().user_display().cyan(), - ); - } - Err(uv_installer::UninstallError::Uninstall( - uv_install_wheel::Error::MissingTopLevel(_), - )) => { - warn_user!( - "Failed to uninstall package at {} due to missing `top_level.txt` file. Installation may result in an incomplete environment.", - dist_info.install_path().user_display().cyan(), - ); - } - Err(err) => return Err(err.into()), } } - logger.on_uninstall(uninstalls.len(), start, printer, DryRun::Disabled)?; + logger.on_uninstall(uninstalls.len(), start, printer, dry_run)?; } // Install the resolved distributions. let mut installs = wheels.into_iter().chain(cached).collect::>(); if !installs.is_empty() { let start = std::time::Instant::now(); - installs = uv_installer::Installer::new(venv, preview) - .with_link_mode(link_mode) - .with_cache(cache) - .with_installer_metadata(installer_metadata) - .with_reporter(Arc::new( - InstallReporter::from(printer).with_length(installs.len() as u64), - )) - // This technically can block the runtime, but we are on the main thread and - // have no other running tasks at this point, so this lets us avoid spawning a blocking - // task. - .install_blocking(installs)?; + if !dry_run.enabled() { + installs = uv_installer::Installer::new(venv, preview) + .with_link_mode(link_mode) + .with_cache(cache) + .with_installer_metadata(installer_metadata) + .with_reporter(Arc::new( + InstallReporter::from(printer).with_length(installs.len() as u64), + )) + // This technically can block the runtime, but we are on the main thread and + // have no other running tasks at this point, so this lets us avoid spawning a blocking + // task. + .install_blocking(installs)?; + } - logger.on_install(installs.len(), start, printer, DryRun::Disabled)?; + logger.on_install(installs.len(), start, printer, dry_run)?; } Ok((installs, uninstalls)) @@ -838,116 +838,6 @@ pub(crate) fn report_target_environment( Ok(writeln!(printer.stderr(), "{}", message.dimmed())?) } -/// Report on the results of a dry-run installation. -#[allow(clippy::result_large_err)] -fn report_dry_run( - dry_run: DryRun, - resolution: &Resolution, - plan: Plan, - modifications: Modifications, - start: std::time::Instant, - printer: Printer, -) -> Result<(), Error> { - let Plan { - cached, - remote, - reinstalls, - extraneous, - } = plan; - - // If we're in `install` mode, ignore any extraneous distributions. - let extraneous = match modifications { - Modifications::Sufficient => vec![], - Modifications::Exact => extraneous, - }; - - // Nothing to do. - if remote.is_empty() && cached.is_empty() && reinstalls.is_empty() && extraneous.is_empty() { - DefaultInstallLogger.on_audit(resolution.len(), start, printer, dry_run)?; - return Ok(()); - } - - // Download, build, and unzip any missing distributions. - let wheels = if remote.is_empty() { - vec![] - } else { - DefaultInstallLogger.on_prepare(remote.len(), None, start, printer, dry_run)?; - remote.clone() - }; - - // Remove any upgraded or extraneous installations. - let uninstalls = extraneous.len() + reinstalls.len(); - - if uninstalls > 0 { - DefaultInstallLogger.on_uninstall(uninstalls, start, printer, dry_run)?; - } - - // Install the resolved distributions. - let installs = wheels.len() + cached.len(); - - if installs > 0 { - DefaultInstallLogger.on_install(installs, start, printer, dry_run)?; - } - - // TODO(charlie): DRY this up with `report_modifications`. The types don't quite line up. - for event in reinstalls - .into_iter() - .chain(extraneous.into_iter()) - .map(|distribution| DryRunEvent { - name: distribution.name().clone(), - version: distribution.installed_version().to_string(), - kind: ChangeEventKind::Removed, - }) - .chain(wheels.into_iter().map(|distribution| DryRunEvent { - name: distribution.name().clone(), - version: distribution.version_or_url().to_string(), - kind: ChangeEventKind::Added, - })) - .chain(cached.into_iter().map(|distribution| DryRunEvent { - name: distribution.name().clone(), - version: distribution.installed_version().to_string(), - kind: ChangeEventKind::Added, - })) - .sorted_unstable_by(|a, b| a.name.cmp(&b.name).then_with(|| a.kind.cmp(&b.kind))) - { - match event.kind { - ChangeEventKind::Added => { - writeln!( - printer.stderr(), - " {} {}{}", - "+".green(), - event.name.bold(), - event.version.dimmed() - )?; - } - ChangeEventKind::Removed => { - writeln!( - printer.stderr(), - " {} {}{}", - "-".red(), - event.name.bold(), - event.version.dimmed() - )?; - } - ChangeEventKind::Reinstalled => { - writeln!( - printer.stderr(), - " {} {}{}", - "~".yellow(), - event.name.bold(), - event.version.dimmed() - )?; - } - } - } - - if matches!(dry_run, DryRun::Check) { - return Err(Error::OutdatedEnvironment); - } - - Ok(()) -} - /// Report any diagnostics on resolved distributions. #[allow(clippy::result_large_err)] pub(crate) fn diagnose_resolution( diff --git a/crates/uv/tests/it/lock_conflict.rs b/crates/uv/tests/it/lock_conflict.rs index 335b0b9fe..2b406fc3c 100644 --- a/crates/uv/tests/it/lock_conflict.rs +++ b/crates/uv/tests/it/lock_conflict.rs @@ -15292,7 +15292,7 @@ fn do_not_simplify_if_not_all_conflict_extras_satisfy_the_marker_by_themselves() Would use project environment at: .venv Resolved 4 packages in [TIME] Found up-to-date lockfile at: uv.lock - Would download 2 packages + Prepared 2 packages in [TIME] Would install 2 packages + python-dateutil==2.8.0 + six==1.17.0 diff --git a/crates/uv/tests/it/pip_install.rs b/crates/uv/tests/it/pip_install.rs index b442fd111..e11716010 100644 --- a/crates/uv/tests/it/pip_install.rs +++ b/crates/uv/tests/it/pip_install.rs @@ -5322,14 +5322,14 @@ fn dry_run_install() -> std::result::Result<(), Box> { .arg("-r") .arg("requirements.txt") .arg("--dry-run") - .arg("--strict"), @r###" + .arg("--strict"), @r" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- Resolved 7 packages in [TIME] - Would download 7 packages + Prepared 7 packages in [TIME] Would install 7 packages + anyio==4.3.0 + certifi==2024.2.2 @@ -5338,7 +5338,7 @@ fn dry_run_install() -> std::result::Result<(), Box> { + httpx==0.25.1 + idna==3.6 + sniffio==1.3.1 - "### + " ); Ok(()) @@ -5354,19 +5354,19 @@ fn dry_run_install_url_dependency() -> std::result::Result<(), Box std::result::Result<(), Box std::result::Result<(), Box Result<()> { uv_snapshot!(context.filters(), windows_filters=false, context.pip_sync() .arg("requirements.txt") .arg("--dry-run") - .arg("--strict"), @r###" + .arg("--strict"), @r#" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- Resolved 1 package in [TIME] - Would download 1 package + Prepared 1 package in [TIME] Would install 1 package + colorama==0.4.2 warning: `colorama==0.4.2` is yanked (reason: "Bad build, missing files, will not install") - "### + "# ); Ok(()) @@ -5883,7 +5883,7 @@ fn pep_751_wheel_only() -> Result<()> { ----- stdout ----- ----- stderr ----- - Would download 9 packages + Prepared 9 packages in [TIME] Would install 9 packages + filelock==3.13.1 + fsspec==2024.3.1 diff --git a/crates/uv/tests/it/sync.rs b/crates/uv/tests/it/sync.rs index d73d99062..cf6d5af02 100644 --- a/crates/uv/tests/it/sync.rs +++ b/crates/uv/tests/it/sync.rs @@ -641,7 +641,7 @@ fn sync_dry_json() -> Result<()> { ----- stderr ----- Using CPython 3.12.[X] interpreter at: [PYTHON-3.12] Resolved 2 packages in [TIME] - Would download 1 package + Prepared 1 package in [TIME] Would install 1 package + iniconfig==2.0.0 "#); @@ -1041,7 +1041,7 @@ fn check() -> Result<()> { )?; // Running `uv sync --check` should fail. - uv_snapshot!(context.filters(), context.sync().arg("--check"), @r###" + uv_snapshot!(context.filters(), context.sync().arg("--check"), @r" success: false exit_code: 1 ----- stdout ----- @@ -1050,11 +1050,11 @@ fn check() -> Result<()> { Would use project environment at: .venv Resolved 2 packages in [TIME] Would create lockfile at: uv.lock - Would download 1 package + Prepared 1 package in [TIME] Would install 1 package + iniconfig==2.0.0 The environment is outdated; run `uv sync` to update the environment - "###); + "); // Sync the environment. uv_snapshot!(context.filters(), context.sync(), @r" @@ -1064,7 +1064,6 @@ fn check() -> Result<()> { ----- stderr ----- Resolved 2 packages in [TIME] - Prepared 1 package in [TIME] Installed 1 package in [TIME] + iniconfig==2.0.0 "); @@ -10588,7 +10587,7 @@ fn sync_dry_run() -> Result<()> { Would create project environment at: .venv Resolved 2 packages in [TIME] Would create lockfile at: uv.lock - Would download 1 package + Prepared 1 package in [TIME] Would install 1 package + iniconfig==2.0.0 "); @@ -10603,7 +10602,6 @@ fn sync_dry_run() -> Result<()> { Using CPython 3.12.[X] interpreter at: [PYTHON-3.12] Creating virtual environment at: .venv Resolved 2 packages in [TIME] - Prepared 1 package in [TIME] Installed 1 package in [TIME] + iniconfig==2.0.0 "); @@ -10743,7 +10741,7 @@ fn sync_dry_run_and_locked() -> Result<()> { ----- stderr ----- Would use project environment at: .venv Resolved 2 packages in [TIME] - Would download 1 package + Prepared 1 package in [TIME] Would install 1 package + iniconfig==2.0.0 The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`. @@ -10794,7 +10792,7 @@ fn sync_dry_run_and_frozen() -> Result<()> { ----- stderr ----- Would use project environment at: .venv - Would download 3 packages + Prepared 3 packages in [TIME] Would install 3 packages + anyio==3.7.0 + idna==3.6