Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove unnecessary Option in Freshness::Dirty #13361

Merged
merged 5 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -608,7 +609,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
});

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)?
};
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/core/compiler/fingerprint/dirty_reason.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ pub enum DirtyReason {
FsStatusOutdated(FsStatus),
NothingObvious,
Forced,
/// First time to build something.
FreshBuild,
}

trait ShellExt {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"),
}
}
}
104 changes: 49 additions & 55 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,43 +415,27 @@ 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 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
// up-to-date and as expected. This is currently used primarily for
// 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
Expand Down Expand Up @@ -1752,19 +1736,52 @@ 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<Option<DirtyReason>> {
let old_fingerprint_short = paths::read(old_hash_path)?;

forced: bool,
) -> Option<DirtyReason> {
if mtime_on_use {
epage marked this conversation as resolved.
Show resolved Hide resolved
// 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);

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,
Err(_) => Some(DirtyReason::FreshBuild),
epage marked this conversation as resolved.
Show resolved Hide resolved
}
}

fn _compare_old_fingerprint(
old_hash_path: &Path,
new_fingerprint: &Fingerprint,
) -> CargoResult<Option<DirtyReason>> {
let old_fingerprint_short = paths::read(old_hash_path)?;

let new_hash = new_fingerprint.hash_u64();

if util::to_hex(new_hash) == old_fingerprint_short && new_fingerprint.fs_status.up_to_date() {
Expand All @@ -1785,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<Option<DirtyReason>>) {
epage marked this conversation as resolved.
Show resolved Hide resolved
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.
///
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/job_queue/job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl Job {
}

/// Creates a new job representing a unit of work.
pub fn new_dirty(work: Work, dirty_reason: Option<DirtyReason>) -> Job {
pub fn new_dirty(work: Work, dirty_reason: DirtyReason) -> Job {
Job {
work,
fresh: Freshness::Dirty(dirty_reason),
Expand Down Expand Up @@ -100,7 +100,7 @@ impl fmt::Debug for Job {
#[derive(Debug, Clone)]
pub enum Freshness {
Fresh,
Dirty(Option<DirtyReason>),
Dirty(DirtyReason),
}

impl Freshness {
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/job_queue/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1110,10 +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 !dirty_reason.is_fresh_build() {
config
.shell()
.verbose(|shell| reason.present_to(shell, unit, ws_root))?;
.verbose(|shell| dirty_reason.present_to(shell, unit, ws_root))?;
}

if unit.mode.is_doc() {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/ops/common_for_install_and_uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down