fix: require Dylint and repair core cache hydration#996
Merged
Conversation
📝 WalkthroughWalkthroughAdds a project-independent ChangesArtifact cache signature and core cache hydrate refresh
Dylint workflow comments
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant SequentialPipeline
participant Esp32Orchestrator
participant FrameworkCoreCache
participant Compiler
SequentialPipeline->>FrameworkCoreCache: hydrate(core_build_dir, compiler, core_and_variant sources, user_overlay)
Esp32Orchestrator->>FrameworkCoreCache: hydrate(core_build_dir, compiler, all_core_sources, user_overlay)
FrameworkCoreCache->>FrameworkCoreCache: copy_artifacts(include_cmdhash=false)
FrameworkCoreCache->>Compiler: artifact_cache_signature(project_dir, source, extra_flags)
Compiler-->>FrameworkCoreCache: signature
FrameworkCoreCache->>FrameworkCoreCache: refresh_command_hashes(copied_objects)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/fbuild-build/src/compiler.rs (1)
527-536: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between the two normalizer functions.
normalize_signature_value_for_projectrepeats the empty/absolute-path checks fromnormalize_signature_valuebefore diverging only in the "under project_dir" branch. Could be collapsed into one function parameterized by an optionalproject_dir, but not urgent.Also applies to: 538-557
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-build/src/compiler.rs` around lines 527 - 536, The two signature normalizer paths duplicate the same empty-string and absolute-path checks, so refactor the logic in normalize_signature_value and normalize_signature_value_for_project to share a single common flow. Keep the existing behavior, but extract the shared pre-checks into one helper or parameterize normalize_signature_value with an optional project_dir so the project-specific branch only handles the under-project-dir case.crates/fbuild-build/src/ch32v/ch32v_compiler.rs (1)
228-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract shared flag-selection/suppression logic to avoid 3-way drift.
artifact_cache_signatureduplicates the extension dispatch and framework-suppression-flag augmentation already inrebuild_signature(Lines 194-226), which itself must already mirrorcompile_one's suppression logic per the comment above it. That's now 3 call sites (compile_one, rebuild_signature, artifact_cache_signature) that must be kept manually in sync per chip file, and the pattern is repeated identically across ch32v/esp32/renesas compilers — 6 places total.Consider extracting a helper like
fn signature_inputs(&self, source, extra_flags) -> (&Path, Vec<String>, Vec<String>)returning(compiler_path, flags, effective_extra), then have bothrebuild_signatureandartifact_cache_signaturecall it and just pick the builder function.♻️ Sketch of a shared helper
+ fn signature_inputs(&self, source: &Path, extra_flags: &[String]) -> (&Path, Vec<String>, Vec<String>) { + let ext = source + .extension() + .unwrap_or_default() + .to_string_lossy() + .to_lowercase(); + let (compiler_path, flags) = match ext.as_str() { + "c" | "s" => (self.gcc_path(), self.c_flags()), + _ => (self.gxx_path(), self.cpp_flags()), + }; + let extra_owned: Vec<String> = if self.is_framework_source(source) { + extra_flags + .iter() + .cloned() + .chain(framework_suppression_flags().iter().map(|s| (*s).to_string())) + .collect() + } else { + extra_flags.to_vec() + }; + (compiler_path, flags, extra_owned) + } + fn rebuild_signature(&self, source: &Path, extra_flags: &[String]) -> String { - // ...duplicated logic... + let (compiler_path, flags, extra_owned) = self.signature_inputs(source, extra_flags); crate::compiler::build_rebuild_signature(compiler_path, &flags, &[], &extra_owned, self.build_unflags()) } fn artifact_cache_signature(&self, project_dir: &Path, source: &Path, extra_flags: &[String]) -> String { - // ...duplicated logic... + let (compiler_path, flags, extra_owned) = self.signature_inputs(source, extra_flags); crate::compiler::build_rebuild_signature_for_project(project_dir, compiler_path, &flags, &[], &extra_owned, self.build_unflags()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-build/src/ch32v/ch32v_compiler.rs` around lines 228 - 264, `artifact_cache_signature` repeats the compiler/flag selection and framework-suppression augmentation already handled in `rebuild_signature`, which can drift from `compile_one`. Extract the shared logic into a helper on `Ch32vCompiler` (for example, a signature-input builder) that returns the compiler path, base flags, and effective extra flags for a source, then have both `rebuild_signature` and `artifact_cache_signature` call it so the extension dispatch and framework suppression stay consistent across these paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/fbuild-build/src/ch32v/ch32v_compiler.rs`:
- Around line 228-264: `artifact_cache_signature` repeats the compiler/flag
selection and framework-suppression augmentation already handled in
`rebuild_signature`, which can drift from `compile_one`. Extract the shared
logic into a helper on `Ch32vCompiler` (for example, a signature-input builder)
that returns the compiler path, base flags, and effective extra flags for a
source, then have both `rebuild_signature` and `artifact_cache_signature` call
it so the extension dispatch and framework suppression stay consistent across
these paths.
In `@crates/fbuild-build/src/compiler.rs`:
- Around line 527-536: The two signature normalizer paths duplicate the same
empty-string and absolute-path checks, so refactor the logic in
normalize_signature_value and normalize_signature_value_for_project to share a
single common flow. Keep the existing behavior, but extract the shared
pre-checks into one helper or parameterize normalize_signature_value with an
optional project_dir so the project-specific branch only handles the
under-project-dir case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef97e0b7-7d4b-4a1e-9b01-1d38c1b72993
📒 Files selected for processing (9)
.github/workflows/dylint.ymlcrates/fbuild-build/src/ch32v/ch32v_compiler.rscrates/fbuild-build/src/compiler.rscrates/fbuild-build/src/compiler_tests.rscrates/fbuild-build/src/esp32/esp32_compiler.rscrates/fbuild-build/src/esp32/orchestrator/build.rscrates/fbuild-build/src/framework_core_cache.rscrates/fbuild-build/src/pipeline/sequential.rscrates/fbuild-build/src/renesas/renesas_compiler.rs
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dylintas a strict required status check formainvia branch protection, without adding Dylint to./lint..github/workflows/dylint.ymlso local lint stays rustfmt + clippy only..o/.dpayloads while regenerating destination.cmdhashfiles from the hydrating project's compiler signature.Validation
required_status_checks.strict=true,contexts=[Dylint],checks=[{context:Dylint, app_id:15368}]; repository rulesets remain empty.git diff --checkuv run --no-project python ci/render_workflows.py --check~/dev/soldrfast-rebuild pattern withsoldr 0.8.0, Rust/Cargo1.94.1, persistent/target, Cargo, Rustup, and soldr cache volumes.docker exec -w /src clud-docker-build-soldr-ee1b6073b8ae soldr --version->soldr 0.8.0docker exec -w /src clud-docker-build-soldr-ee1b6073b8ae sh -lc 'SOLDR_COMPILE_REPLY_TIMEOUT_SECS=180 soldr cargo test -p fbuild-build framework_core_cache -- --nocapture'docker exec -w /src clud-docker-build-soldr-ee1b6073b8ae sh -lc 'SOLDR_COMPILE_REPLY_TIMEOUT_SECS=180 soldr cargo test -p fbuild-build test_artifact_cache_signature_ignores_project_directory_name -- --nocapture'docker exec -w /src clud-docker-build-soldr-ee1b6073b8ae sh -lc 'SOLDR_COMPILE_REPLY_TIMEOUT_SECS=180 soldr cargo test -p fbuild-build rebuild_signature_matches_write_path_with_unflags -- --nocapture'docker exec -w /src clud-docker-build-soldr-ee1b6073b8ae sh -lc 'soldr cargo fmt --all -- --check'docker exec -w /src clud-docker-build-soldr-ee1b6073b8ae sh -lc 'SOLDR_COMPILE_REPLY_TIMEOUT_SECS=180 soldr cargo clippy -p fbuild-build --all-targets -- -D warnings'Notes
esp32_build::build_esp32dev_blinkrun was attempted for perf-log evidence, but that standalone test currently bypasses the daemon startup path and fails the existingcompile_backend not installedguard from perf(zccache) Phase 4 stage 2: embedded mandatory — delete FBUILD_ZCCACHE_EMBEDDED + wrapper fallback #800, so it is not used as this PR's validation signal..clud/docker-build/soldrhelper files were used only for validation and intentionally left out of this fbuild PR. The durable upstream clud follow-up is filed as fix: bake soldr into docker-build soldr helper image zackees/clud#502.Closes #994.
Closes #995.
Summary by CodeRabbit
New Features
Bug Fixes