From ade6f97241bdcb0903699d984d897c6990c003e5 Mon Sep 17 00:00:00 2001 From: zackees Date: Sun, 5 Jul 2026 11:34:20 -0700 Subject: [PATCH] fix(build): don't treat the depfile's own mtime as a stale prerequisite (#957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dependency_is_newer_than_object compared the object against its OWN depfile mtime. The .d is an *output* of the same gcc `-c -MMD` invocation that wrote the .o (gcc finalizes the .d right after the .o), so on a cold build the depfile is always slightly newer than its object. That tripped the staleness check on the first rebuild after a cold build, recompiling every TU exactly once (FastLED/fbuild#957) — it settled only because the recompile bumped the object's mtime past the stale depfile. Remove the depfile-own-mtime comparison; staleness is determined solely by the real prerequisites (source + headers) the depfile lists, which were already checked (with #951's workspace-relative base resolution). Adds two regression tests: depfile-newer-than-object is NOT stale; a prerequisite-newer-than-object IS stale (genuine staleness preserved). Part of #957 / #942. (Separate latent build_unflags signature asymmetry tracked in #970.) Co-Authored-By: Claude Fable 5 --- crates/fbuild-build/src/compiler.rs | 13 +++-- crates/fbuild-build/src/compiler_tests.rs | 65 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/crates/fbuild-build/src/compiler.rs b/crates/fbuild-build/src/compiler.rs index 151bfde4..6eaecff7 100644 --- a/crates/fbuild-build/src/compiler.rs +++ b/crates/fbuild-build/src/compiler.rs @@ -511,11 +511,14 @@ fn dependency_is_newer_than_object( object_time: SystemTime, base: Option<&Path>, ) -> std::io::Result { - let depfile_time = depfile.metadata()?.modified()?; - if depfile_time > object_time { - return Ok(true); - } - + // The depfile's OWN mtime is deliberately NOT compared against the object. + // The `.d` is an *output* of the same gcc `-c -MMD` invocation that wrote + // the `.o` — gcc finalizes the `.d` right after the `.o`, so on a cold build + // the depfile is always slightly newer than its object. Treating that as + // "stale" made every TU recompile exactly once on the first rebuild after a + // cold build (FastLED/fbuild#957), settling only because the recompile bumps + // the object's mtime past the stale depfile. Staleness is determined solely + // by the real prerequisites (source + headers) the depfile *lists*, below. for dependency in parse_depfile_paths(depfile)? { let resolved = match base { Some(base) if dependency.is_relative() => base.join(&dependency), diff --git a/crates/fbuild-build/src/compiler_tests.rs b/crates/fbuild-build/src/compiler_tests.rs index 52c32950..211c232f 100644 --- a/crates/fbuild-build/src/compiler_tests.rs +++ b/crates/fbuild-build/src/compiler_tests.rs @@ -472,3 +472,68 @@ fn test_build_rebuild_signature_changes_when_non_path_flag_changes() { assert_ne!(sig_a, sig_b); } + +#[test] +fn test_depfile_own_mtime_does_not_force_rebuild() { + // Regression for FastLED/fbuild#957: gcc writes the `.d` AFTER the `.o`, so + // on a cold build the depfile is always slightly newer than its object. + // That must NOT be treated as stale — only the real prerequisites the + // depfile lists (source + headers) determine staleness. Before the fix, + // this returned `true` and forced every TU to recompile once on the first + // rebuild after a cold build. + use filetime::{set_file_mtime, FileTime}; + + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + let src = dir.join("src.cpp"); + let hdr = dir.join("hdr.h"); + let obj = dir.join("obj.o"); + let dep = dir.join("obj.d"); + std::fs::write(&src, "int main(){}").unwrap(); + std::fs::write(&hdr, "#define X 1").unwrap(); + std::fs::write(&obj, b"obj").unwrap(); + // Relative prereqs (resolved against `base`) — avoids Windows drive-colon + // ambiguity in the depfile parser. + std::fs::write(&dep, "obj.o: src.cpp hdr.h\n").unwrap(); + + // Prerequisites OLDEST, object newer, depfile NEWEST (exactly as gcc emits). + let base = FileTime::from_unix_time(1_000_000, 0); + set_file_mtime(&src, base).unwrap(); + set_file_mtime(&hdr, base).unwrap(); + set_file_mtime(&obj, FileTime::from_unix_time(1_000_100, 0)).unwrap(); + set_file_mtime(&dep, FileTime::from_unix_time(1_000_200, 0)).unwrap(); + + let object_time = std::fs::metadata(&obj).unwrap().modified().unwrap(); + let stale = dependency_is_newer_than_object(&dep, object_time, Some(dir)).unwrap(); + assert!( + !stale, + "depfile newer than object must not force a rebuild (#957)" + ); +} + +#[test] +fn test_depfile_newer_prerequisite_still_forces_rebuild() { + // Complement: a real header edit (a prerequisite newer than the object) IS + // stale — the fix must not weaken genuine staleness detection. + use filetime::{set_file_mtime, FileTime}; + + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + let hdr = dir.join("hdr.h"); + let obj = dir.join("obj.o"); + let dep = dir.join("obj.d"); + std::fs::write(&hdr, "#define X 2").unwrap(); + std::fs::write(&obj, b"obj").unwrap(); + std::fs::write(&dep, "obj.o: hdr.h\n").unwrap(); + + set_file_mtime(&obj, FileTime::from_unix_time(1_000_100, 0)).unwrap(); + // Header edited AFTER the object was built. + set_file_mtime(&hdr, FileTime::from_unix_time(1_000_300, 0)).unwrap(); + + let object_time = std::fs::metadata(&obj).unwrap().modified().unwrap(); + let stale = dependency_is_newer_than_object(&dep, object_time, Some(dir)).unwrap(); + assert!( + stale, + "a prerequisite newer than the object must force a rebuild" + ); +}