From d3a3c329fd2868001df2a9f0e62596f0a729ea5f Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 28 Jan 2024 13:36:45 -0500 Subject: [PATCH 1/5] refactor: call `log_compare` in fingerprint compare fn --- src/cargo/core/compiler/fingerprint/mod.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index e1737a8b657..94a464042e6 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -415,8 +415,7 @@ pub fn prepare_target(cx: &mut Context<'_, '_>, unit: &Unit, force: bool) -> Car // information about failed comparisons to aid in debugging. let fingerprint = calculate(cx, unit)?; let mtime_on_use = cx.bcx.config.cli_unstable().mtime_on_use; - let compare = compare_old_fingerprint(&loc, &*fingerprint, mtime_on_use); - log_compare(unit, &compare); + let compare = compare_old_fingerprint(unit, &loc, &*fingerprint, mtime_on_use); // If our comparison failed or reported dirty (e.g., we're going to trigger // a rebuild of this crate), then we also ensure the source of the crate @@ -1752,6 +1751,17 @@ fn target_root(cx: &Context<'_, '_>) -> PathBuf { /// If dirty, it then restores the detailed information /// from the fingerprint JSON file, and provides an rich dirty reason. fn compare_old_fingerprint( + unit: &Unit, + old_hash_path: &Path, + new_fingerprint: &Fingerprint, + mtime_on_use: bool, +) -> CargoResult> { + let compare = _compare_old_fingerprint(old_hash_path, new_fingerprint, mtime_on_use); + log_compare(unit, &compare); + compare +} + +fn _compare_old_fingerprint( old_hash_path: &Path, new_fingerprint: &Fingerprint, mtime_on_use: bool, From 95303550048bd6cfd9e5039388690eed25ec4d07 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 28 Jan 2024 13:58:51 -0500 Subject: [PATCH 2/5] refactor: `DirtyReason::FreshBuild` to represent build from scratch the only beahvior change in this commit is that source verification is also performed for `DirtyReason::Forced`. --- .../core/compiler/fingerprint/dirty_reason.rs | 8 +++ src/cargo/core/compiler/fingerprint/mod.rs | 52 ++++++++----------- src/cargo/core/compiler/job_queue/mod.rs | 8 +-- 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint/dirty_reason.rs b/src/cargo/core/compiler/fingerprint/dirty_reason.rs index bdd2052c04e..418422c14c5 100644 --- a/src/cargo/core/compiler/fingerprint/dirty_reason.rs +++ b/src/cargo/core/compiler/fingerprint/dirty_reason.rs @@ -77,6 +77,8 @@ pub enum DirtyReason { FsStatusOutdated(FsStatus), NothingObvious, Forced, + /// First time to build something. + FreshBuild, } trait ShellExt { @@ -131,6 +133,11 @@ impl fmt::Display for After { } impl DirtyReason { + /// Whether a build is dirty because it is a fresh build being kicked off. + pub fn is_fresh_build(&self) -> bool { + matches!(self, DirtyReason::FreshBuild) + } + fn after(old_time: FileTime, new_time: FileTime, what: &'static str) -> After { After { old_time, @@ -257,6 +264,7 @@ impl DirtyReason { s.dirty_because(unit, "the fingerprint comparison turned up nothing obvious") } DirtyReason::Forced => s.dirty_because(unit, "forced"), + DirtyReason::FreshBuild => s.dirty_because(unit, "fresh build"), } } } diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index 94a464042e6..ea768e78cbc 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -415,11 +415,14 @@ pub fn prepare_target(cx: &mut Context<'_, '_>, unit: &Unit, force: bool) -> Car // information about failed comparisons to aid in debugging. let fingerprint = calculate(cx, unit)?; let mtime_on_use = cx.bcx.config.cli_unstable().mtime_on_use; - let compare = compare_old_fingerprint(unit, &loc, &*fingerprint, mtime_on_use); + let dirty_reason = compare_old_fingerprint(unit, &loc, &*fingerprint, mtime_on_use, force); - // If our comparison failed or reported dirty (e.g., we're going to trigger - // a rebuild of this crate), then we also ensure the source of the crate - // passes all verification checks before we build it. + let Some(dirty_reason) = dirty_reason else { + return Ok(Job::new_fresh()); + }; + + // We're going to rebuild, so ensure the source of the crate passes all + // verification checks before we build it. // // The `Source::verify` method is intended to allow sources to execute // pre-build checks to ensure that the relevant source code is all @@ -427,30 +430,12 @@ pub fn prepare_target(cx: &mut Context<'_, '_>, unit: &Unit, force: bool) -> Car // directory sources which will use this hook to perform an integrity check // on all files in the source to ensure they haven't changed. If they have // changed then an error is issued. - if compare - .as_ref() - .map(|dirty| dirty.is_some()) - .unwrap_or(true) - { - let source_id = unit.pkg.package_id().source_id(); - let sources = bcx.packages.sources(); - let source = sources - .get(source_id) - .ok_or_else(|| internal("missing package source"))?; - source.verify(unit.pkg.package_id())?; - } - - let dirty_reason = match compare { - Ok(None) => { - if force { - Some(DirtyReason::Forced) - } else { - return Ok(Job::new_fresh()); - } - } - Ok(reason) => reason, - Err(_) => None, - }; + let source_id = unit.pkg.package_id().source_id(); + let sources = bcx.packages.sources(); + let source = sources + .get(source_id) + .ok_or_else(|| internal("missing package source"))?; + source.verify(unit.pkg.package_id())?; // Clear out the old fingerprint file if it exists. This protects when // compilation is interrupted leaving a corrupt file. For example, a @@ -521,7 +506,7 @@ pub fn prepare_target(cx: &mut Context<'_, '_>, unit: &Unit, force: bool) -> Car Work::new(move |_| write_fingerprint(&loc, &fingerprint)) }; - Ok(Job::new_dirty(write_fingerprint, dirty_reason)) + Ok(Job::new_dirty(write_fingerprint, Some(dirty_reason))) } /// Dependency edge information for fingerprints. This is generated for each @@ -1755,10 +1740,15 @@ fn compare_old_fingerprint( old_hash_path: &Path, new_fingerprint: &Fingerprint, mtime_on_use: bool, -) -> CargoResult> { + forced: bool, +) -> Option { let compare = _compare_old_fingerprint(old_hash_path, new_fingerprint, mtime_on_use); log_compare(unit, &compare); - compare + match compare { + Ok(None) if forced => Some(DirtyReason::Forced), + Ok(reason) => reason, + Err(_) => Some(DirtyReason::FreshBuild), + } } fn _compare_old_fingerprint( diff --git a/src/cargo/core/compiler/job_queue/mod.rs b/src/cargo/core/compiler/job_queue/mod.rs index 7c4c89e4f37..aa2ced1c269 100644 --- a/src/cargo/core/compiler/job_queue/mod.rs +++ b/src/cargo/core/compiler/job_queue/mod.rs @@ -1111,9 +1111,11 @@ impl<'cfg> DrainState<'cfg> { // being a compiled package. Dirty(dirty_reason) => { if let Some(reason) = dirty_reason { - config - .shell() - .verbose(|shell| reason.present_to(shell, unit, ws_root))?; + if !reason.is_fresh_build() { + config + .shell() + .verbose(|shell| reason.present_to(shell, unit, ws_root))?; + } } if unit.mode.is_doc() { From 21f4de7bcf3f8a754d05f3250f3659d47a52c6b3 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Sun, 28 Jan 2024 14:06:20 -0500 Subject: [PATCH 3/5] refactor: remove unnecessary Option in `Freshness::Dirty` --- src/cargo/core/compiler/custom_build.rs | 3 ++- src/cargo/core/compiler/fingerprint/mod.rs | 2 +- src/cargo/core/compiler/job_queue/job.rs | 4 ++-- src/cargo/core/compiler/job_queue/mod.rs | 10 ++++------ src/cargo/core/compiler/mod.rs | 2 +- src/cargo/ops/common_for_install_and_uninstall.rs | 6 +++--- 6 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 99311a5ffd1..f66e448cbf3 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -34,6 +34,7 @@ use super::{fingerprint, Context, Job, Unit, Work}; use crate::core::compiler::artifact; use crate::core::compiler::context::Metadata; +use crate::core::compiler::fingerprint::DirtyReason; use crate::core::compiler::job_queue::JobState; use crate::core::{profiles::ProfileRoot, PackageId, Target}; use crate::util::errors::CargoResult; @@ -608,7 +609,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { }); let mut job = if cx.bcx.build_config.build_plan { - Job::new_dirty(Work::noop(), None) + Job::new_dirty(Work::noop(), DirtyReason::FreshBuild) } else { fingerprint::prepare_target(cx, unit, false)? }; diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index ea768e78cbc..925123a8a10 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -506,7 +506,7 @@ pub fn prepare_target(cx: &mut Context<'_, '_>, unit: &Unit, force: bool) -> Car Work::new(move |_| write_fingerprint(&loc, &fingerprint)) }; - Ok(Job::new_dirty(write_fingerprint, Some(dirty_reason))) + Ok(Job::new_dirty(write_fingerprint, dirty_reason)) } /// Dependency edge information for fingerprints. This is generated for each diff --git a/src/cargo/core/compiler/job_queue/job.rs b/src/cargo/core/compiler/job_queue/job.rs index ae802d6a03c..71a9fc4184c 100644 --- a/src/cargo/core/compiler/job_queue/job.rs +++ b/src/cargo/core/compiler/job_queue/job.rs @@ -60,7 +60,7 @@ impl Job { } /// Creates a new job representing a unit of work. - pub fn new_dirty(work: Work, dirty_reason: Option) -> Job { + pub fn new_dirty(work: Work, dirty_reason: DirtyReason) -> Job { Job { work, fresh: Freshness::Dirty(dirty_reason), @@ -100,7 +100,7 @@ impl fmt::Debug for Job { #[derive(Debug, Clone)] pub enum Freshness { Fresh, - Dirty(Option), + Dirty(DirtyReason), } impl Freshness { diff --git a/src/cargo/core/compiler/job_queue/mod.rs b/src/cargo/core/compiler/job_queue/mod.rs index aa2ced1c269..79ea60dfd49 100644 --- a/src/cargo/core/compiler/job_queue/mod.rs +++ b/src/cargo/core/compiler/job_queue/mod.rs @@ -1110,12 +1110,10 @@ impl<'cfg> DrainState<'cfg> { // Any dirty stage which runs at least one command gets printed as // being a compiled package. Dirty(dirty_reason) => { - if let Some(reason) = dirty_reason { - if !reason.is_fresh_build() { - config - .shell() - .verbose(|shell| reason.present_to(shell, unit, ws_root))?; - } + if !dirty_reason.is_fresh_build() { + config + .shell() + .verbose(|shell| dirty_reason.present_to(shell, unit, ws_root))?; } if unit.mode.is_doc() { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 14e4bde4b11..d6411d88851 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -183,7 +183,7 @@ fn compile<'cfg>( // We run these targets later, so this is just a no-op for now. Job::new_fresh() } else if build_plan { - Job::new_dirty(rustc(cx, unit, &exec.clone())?, None) + Job::new_dirty(rustc(cx, unit, &exec.clone())?, DirtyReason::FreshBuild) } else { let force = exec.force_rebuild(unit) || force_rebuild; let mut job = fingerprint::prepare_target(cx, unit, force)?; diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs index e678a64dfc3..c1d8039edc6 100644 --- a/src/cargo/ops/common_for_install_and_uninstall.rs +++ b/src/cargo/ops/common_for_install_and_uninstall.rs @@ -175,7 +175,7 @@ impl InstallTracker { // Check if any tracked exe's are already installed. let duplicates = self.find_duplicates(dst, &exes); if force || duplicates.is_empty() { - return Ok((Freshness::Dirty(Some(DirtyReason::Forced)), duplicates)); + return Ok((Freshness::Dirty(DirtyReason::Forced), duplicates)); } // Check if all duplicates come from packages of the same name. If // there are duplicates from other packages, then --force will be @@ -205,7 +205,7 @@ impl InstallTracker { let source_id = pkg.package_id().source_id(); if source_id.is_path() { // `cargo install --path ...` is always rebuilt. - return Ok((Freshness::Dirty(Some(DirtyReason::Forced)), duplicates)); + return Ok((Freshness::Dirty(DirtyReason::Forced), duplicates)); } let is_up_to_date = |dupe_pkg_id| { let info = self @@ -229,7 +229,7 @@ impl InstallTracker { if matching_duplicates.iter().all(is_up_to_date) { Ok((Freshness::Fresh, duplicates)) } else { - Ok((Freshness::Dirty(Some(DirtyReason::Forced)), duplicates)) + Ok((Freshness::Dirty(DirtyReason::Forced), duplicates)) } } else { // Format the error message. From bf0b156885d266c65d2307831f8ec47448c710e6 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 29 Jan 2024 13:20:09 -0500 Subject: [PATCH 4/5] refactor: move mtime_on_use check up to caller --- src/cargo/core/compiler/fingerprint/mod.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index 925123a8a10..5bbe804e913 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -1742,7 +1742,14 @@ fn compare_old_fingerprint( mtime_on_use: bool, forced: bool, ) -> Option { - let compare = _compare_old_fingerprint(old_hash_path, new_fingerprint, mtime_on_use); + if mtime_on_use { + // update the mtime so other cleaners know we used it + let t = FileTime::from_system_time(SystemTime::now()); + debug!("mtime-on-use forcing {:?} to {}", old_hash_path, t); + paths::set_file_time_no_err(old_hash_path, t); + } + + let compare = _compare_old_fingerprint(old_hash_path, new_fingerprint); log_compare(unit, &compare); match compare { Ok(None) if forced => Some(DirtyReason::Forced), @@ -1754,17 +1761,9 @@ fn compare_old_fingerprint( fn _compare_old_fingerprint( old_hash_path: &Path, new_fingerprint: &Fingerprint, - mtime_on_use: bool, ) -> CargoResult> { let old_fingerprint_short = paths::read(old_hash_path)?; - if mtime_on_use { - // update the mtime so other cleaners know we used it - let t = FileTime::from_system_time(SystemTime::now()); - debug!("mtime-on-use forcing {:?} to {}", old_hash_path, t); - paths::set_file_time_no_err(old_hash_path, t); - } - let new_hash = new_fingerprint.hash_u64(); if util::to_hex(new_hash) == old_fingerprint_short && new_fingerprint.fs_status.up_to_date() { From a04f2b2a7df7e8351569d06cffc696bb35759b6d Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 29 Jan 2024 13:22:57 -0500 Subject: [PATCH 5/5] refactor: flatten `log_compare` into caller --- src/cargo/core/compiler/fingerprint/mod.rs | 43 ++++++++++------------ 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index 5bbe804e913..316e143e71d 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -1750,7 +1750,25 @@ fn compare_old_fingerprint( } let compare = _compare_old_fingerprint(old_hash_path, new_fingerprint); - log_compare(unit, &compare); + + match compare.as_ref() { + Ok(None) => {} + Ok(Some(reason)) => { + info!( + "fingerprint dirty for {}/{:?}/{:?}", + unit.pkg, unit.mode, unit.target, + ); + info!(" dirty: {reason:?}"); + } + Err(e) => { + info!( + "fingerprint error for {}/{:?}/{:?}", + unit.pkg, unit.mode, unit.target, + ); + info!(" err: {e:?}"); + } + } + match compare { Ok(None) if forced => Some(DirtyReason::Forced), Ok(reason) => reason, @@ -1784,29 +1802,6 @@ fn _compare_old_fingerprint( Ok(Some(new_fingerprint.compare(&old_fingerprint))) } -/// Logs the result of fingerprint comparison. -/// -/// TODO: Obsolete and mostly superseded by [`DirtyReason`]. Could be removed. -fn log_compare(unit: &Unit, compare: &CargoResult>) { - match compare { - Ok(None) => {} - Ok(Some(reason)) => { - info!( - "fingerprint dirty for {}/{:?}/{:?}", - unit.pkg, unit.mode, unit.target, - ); - info!(" dirty: {reason:?}"); - } - Err(e) => { - info!( - "fingerprint error for {}/{:?}/{:?}", - unit.pkg, unit.mode, unit.target, - ); - info!(" err: {e:?}"); - } - } -} - /// Parses Cargo's internal [`EncodedDepInfo`] structure that was previously /// serialized to disk. ///