From 51aef1bdf6edbf06eaecd6a9bd42111a8930ca59 Mon Sep 17 00:00:00 2001 From: zackees Date: Thu, 2 Jul 2026 17:18:50 -0700 Subject: [PATCH] fix(build): stop no-change rebuilds from recompiling every TU MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two staleness-check defects made every rebuild a full recompile (FastLED/fbuild#951, ~108s per no-op rebuild of NightDriverStrip): 1. Compiles run with cwd = the project workspace (zccache compile-cwd normalization), so gcc -MMD depfiles list workspace-relative prerequisites — but dependency_is_newer_than_object stat'ed them against the daemon's process cwd, failed, and .unwrap_or(true) marked every TU stale. Resolve relative prerequisites against zccache::compile_cwd_from_output(object). 2. compile_c/compile_cpp apply build_unflags before hashing the written .cmdhash, but both rebuild_signature impls (trait default + Esp32Compiler) hashed the raw flags, so any project with build_unflags (NightDriverStrip sets -std=gnu++11) mismatched on every file. Apply the same filter on the check side. Validated in the #942 Docker harness: steady-state no-change rebuild drops 108s -> 2.1s (compile-sketch 92s -> 0.48s stat walk, compile-core-variant 10.7s -> 0.2s). The first rebuild after a cold build still recompiles once (signature drift between mid-cold and steady-state flag assembly) — tracked separately. Closes #951 Part of #942 Co-Authored-By: Claude Fable 5 --- crates/fbuild-build/src/compiler.rs | 26 +++++++++-- crates/fbuild-build/src/compiler_tests.rs | 43 +++++++++++++++++++ .../fbuild-build/src/esp32/esp32_compiler.rs | 43 ++++++++++++++++++- 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/crates/fbuild-build/src/compiler.rs b/crates/fbuild-build/src/compiler.rs index e98231c8..151bfde4 100644 --- a/crates/fbuild-build/src/compiler.rs +++ b/crates/fbuild-build/src/compiler.rs @@ -168,7 +168,11 @@ pub trait Compiler: Send + Sync { "c" | "s" => (self.gcc_path(), self.c_flags()), _ => (self.gxx_path(), self.cpp_flags()), }; - build_rebuild_signature(compiler_path, &flags, &[], extra_flags) + // Mirror compile_c/compile_cpp: build_unflags are applied before + // the compile, so the checked signature must hash the same + // filtered flag set as the written one (FastLED/fbuild#951). + let (flags, extra_flags) = apply_compile_unflags(flags, extra_flags, self.build_unflags()); + build_rebuild_signature(compiler_path, &flags, &[], &extra_flags) } } @@ -242,7 +246,16 @@ impl CompilerBase { let depfile = depfile_path(object); if depfile.exists() { - if dependency_is_newer_than_object(&depfile, obj_time).unwrap_or(true) { + // Compiles run with cwd = the project workspace (see + // zccache::compile_cwd_from_output), so -MMD depfiles list + // workspace-relative prerequisites. Resolve them against that + // same workspace — NOT the process cwd, which in the daemon is + // unrelated and made every stat fail → every TU "stale" + // (FastLED/fbuild#951). + let dep_base = crate::zccache::compile_cwd_from_output(object); + if dependency_is_newer_than_object(&depfile, obj_time, dep_base.as_deref()) + .unwrap_or(true) + { return true; } return false; @@ -285,7 +298,7 @@ impl CompilerBase { /// Returns the filtered pair ready to pass to `compile_one`. Short-circuits /// when `unflags` is empty so platforms that don't opt in pay no overhead. /// See FastLED/fbuild#37. -fn apply_compile_unflags( +pub(crate) fn apply_compile_unflags( flags: Vec, extra_flags: &[String], unflags: &[String], @@ -496,6 +509,7 @@ fn compiler_version(path: &Path) -> String { fn dependency_is_newer_than_object( depfile: &Path, object_time: SystemTime, + base: Option<&Path>, ) -> std::io::Result { let depfile_time = depfile.metadata()?.modified()?; if depfile_time > object_time { @@ -503,7 +517,11 @@ fn dependency_is_newer_than_object( } for dependency in parse_depfile_paths(depfile)? { - let dep_time = std::fs::metadata(&dependency)?.modified()?; + let resolved = match base { + Some(base) if dependency.is_relative() => base.join(&dependency), + _ => dependency, + }; + let dep_time = std::fs::metadata(&resolved)?.modified()?; if dep_time > object_time { return Ok(true); } diff --git a/crates/fbuild-build/src/compiler_tests.rs b/crates/fbuild-build/src/compiler_tests.rs index 4a5c5813..52c32950 100644 --- a/crates/fbuild-build/src/compiler_tests.rs +++ b/crates/fbuild-build/src/compiler_tests.rs @@ -309,6 +309,49 @@ fn test_needs_rebuild_uses_depfile_when_dependencies_are_current() { assert!(!CompilerBase::needs_rebuild(&src, &obj)); } +/// FastLED/fbuild#951: compiles run with cwd = the project workspace +/// (see `zccache::compile_cwd_from_output`), so gcc's `-MMD` depfiles +/// list *relative* prerequisites. The staleness walk runs inside the +/// long-lived daemon whose process cwd is unrelated — resolving those +/// prerequisites against the process cwd made `metadata()` fail and +/// `.unwrap_or(true)` marked every TU stale, recompiling the whole +/// sketch + core variant on no-change rebuilds (~103 s per build). +#[test] +fn test_needs_rebuild_resolves_relative_depfile_deps_against_workspace() { + let tmp = tempfile::TempDir::new().unwrap(); + let ws = tmp.path(); + let src_dir = ws.join("src"); + let build_dir = ws.join(".fbuild/build/demo/release/src"); + std::fs::create_dir_all(&src_dir).unwrap(); + std::fs::create_dir_all(&build_dir).unwrap(); + + let src = src_dir.join("main.cpp"); + let header = src_dir.join("config.h"); + let obj = build_dir.join("main.cpp.o"); + let dep = build_dir.join("main.cpp.d"); + + std::fs::write(&src, "#include \"config.h\"\n").unwrap(); + std::fs::write(&header, "#define X 1\n").unwrap(); + // Relative prerequisites, exactly as gcc -MMD emits them when the + // compile cwd is the workspace root. + std::fs::write( + &dep, + ".fbuild/build/demo/release/src/main.cpp.o: src/main.cpp src/config.h\n", + ) + .unwrap(); + std::thread::sleep(std::time::Duration::from_millis(20)); + std::fs::write(&obj, "obj").unwrap(); + + // Everything is current — must NOT rebuild even though the deps are + // relative and the test process cwd is nowhere near the workspace. + assert!(!CompilerBase::needs_rebuild(&src, &obj)); + + // And a genuinely newer relative dep must still trigger a rebuild. + std::thread::sleep(std::time::Duration::from_millis(20)); + std::fs::write(&header, "#define X 2\n").unwrap(); + assert!(CompilerBase::needs_rebuild(&src, &obj)); +} + #[test] fn test_needs_rebuild_when_command_hash_changes() { let tmp = tempfile::TempDir::new().unwrap(); diff --git a/crates/fbuild-build/src/esp32/esp32_compiler.rs b/crates/fbuild-build/src/esp32/esp32_compiler.rs index 70f1e039..4864b6ba 100644 --- a/crates/fbuild-build/src/esp32/esp32_compiler.rs +++ b/crates/fbuild-build/src/esp32/esp32_compiler.rs @@ -211,11 +211,19 @@ impl Compiler for Esp32Compiler { "c" | "s" => self.gcc_path(), _ => self.gxx_path(), }; + // Mirror compile_c/compile_cpp: build_unflags are applied before + // the compile, so the checked signature must hash the same + // filtered flag set as the written one (FastLED/fbuild#951). + let (base_flags, extra_flags) = crate::compiler::apply_compile_unflags( + base_flags, + extra_flags, + Compiler::build_unflags(self), + ); crate::compiler::build_rebuild_signature( compiler_path, &base_flags, &include_flags, - extra_flags, + &extra_flags, ) } } @@ -373,6 +381,39 @@ mod tests { ); } + /// FastLED/fbuild#951: the staleness check's signature must equal the + /// one written after a compile. `compile_c`/`compile_cpp` apply + /// `build_unflags` before hashing, but `rebuild_signature` hashed the + /// raw flags — so any project with `build_unflags` (NightDriverStrip + /// sets `-std=gnu++11`) mismatched on every file and recompiled the + /// entire sketch + core on no-change rebuilds. + #[test] + fn rebuild_signature_matches_write_path_with_unflags() { + let compiler = + test_compiler("esp32c6").with_build_unflags(vec!["-std=gnu++2b".to_string()]); + let source = Path::new("src/main.cpp"); + let extra = vec!["-DX=1".to_string()]; + + let check = compiler.rebuild_signature(source, &extra); + + // Mirror the write path: compile_cpp applies unflags, then + // compile_source hashes (flags, include_flags, extra). + let (applied_flags, applied_extra) = crate::compiler::apply_compile_unflags( + compiler.cpp_flags(), + &extra, + Compiler::build_unflags(&compiler), + ); + let include_flags = compiler.base.build_include_flags(); + let written = crate::compiler::build_rebuild_signature( + compiler.gxx_path(), + &applied_flags, + &include_flags, + &applied_extra, + ); + + assert_eq!(check, written); + } + /// FastLED/fbuild#243: by default the compiler preserves eh_frame; the /// STRIP_FLAGS must not leak into the effective compile line. #[test]