Uh oh!
There was an error while loading. Please reload this page.
ci: switch compile to fbuild compile-many - #2476
Conversation
📝 WalkthroughWalkthroughAdds batched fbuild support: uncaps cpu_count on Actions, implements a compile-many/ci batch runner that parses per-sketch results, stages isolated per-example PlatformIO projects, routes PioCompiler to invoke a single batched run, and adds tests covering parsing, staging, and artifact generation. Changesfbuild compile-many integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@ci/tests/test_fbuild_compile_many.py`:
- Around line 134-141: The constructor call to
fbuild_runner.FbuildCompileManySketchResult has inconsistent indentation: align
the stage="stage1" argument with the other keyword arguments
(sketch_dir=root_project, success=True, build_time_secs=1.0, log_path=root_log,
message="ok") so all parameters have the same indentation level inside the
fbuild_runner.FbuildCompileManySketchResult(...) invocation.
In `@ci/util/fbuild_runner.py`:
- Around line 51-68: The two dataclasses FbuildCompileManySketchResult and
FbuildCompileManyResult must be decorated with `@typechecked` for runtime type
validation; add "from typeguard import typechecked" if missing and place
`@typechecked` immediately above each `@dataclass` declaration (i.e., above
FbuildCompileManySketchResult and above FbuildCompileManyResult) so their fields
are validated at runtime (FbuildCompileManyResult still inherits from
FbuildCommandResult).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b52caa49-26bc-4b92-93ba-4edbf9aa9efc
📒 Files selected for processing (4)
ci/compiler/pio.pyci/tests/test_fbuild_compile_many.pyci/util/cpu_count.pyci/util/fbuild_runner.py
💤 Files with no reviewable changes (1)
- ci/util/cpu_count.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@ci/util/fbuild_runner.py`:
- Around line 73-79: The parser currently requires exact two-space separators in
_COMPILE_MANY_RESULT_RE and _parse_compile_many_results silently drops
non-matching lines; update run_fbuild_compile_many so that after calling
_parse_compile_many_results you check the raw fbuild stdout/stderr and if they
are non-empty but the returned sketch_results list is empty, emit a warning (via
logger) and mark success=False (or return an explicit error) so callers don't
treat an empty parse as success; reference _COMPILE_MANY_RESULT_RE,
_parse_compile_many_results and run_fbuild_compile_many when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0f77402-4ac8-4836-994a-a5dbe0440aeb
📒 Files selected for processing (2)
ci/tests/test_fbuild_compile_many.pyci/util/fbuild_runner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/tests/test_fbuild_compile_many.py
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@ci/util/fbuild_runner.py`:
- Around line 295-301: The public function run_fbuild_compile_many currently
defines defaults for operational flags (verbose: bool = False, timeout: float =
1800, quiet: bool = False, log_file: IO[str] | None = None) which violates the
project's rule that public Python function parameters must not have default
values; change the signature of run_fbuild_compile_many to remove the default
assignments so each parameter (verbose, timeout, quiet, log_file) is required
(keep existing type annotations), and update all call sites to pass these
arguments explicitly (or propagate callers similarly) to preserve behavior.
- Around line 154-163: The feature-probe subprocess call that invokes
subprocess.run([fbuild_exe, "help", "compile-many"], ...) must handle
KeyboardInterrupt before the broader exception clause; add an explicit "except
KeyboardInterrupt:" block immediately after the try to re-raise the interrupt
(or propagate it) so cancellation is handled identically to other fbuild
runners, then keep the existing "except (FileNotFoundError,
subprocess.SubprocessError, OSError): return False" branch; locate the try
around subprocess.run and insert the KeyboardInterrupt handler there.
- Around line 354-366: The current success branch only fails when zero parse
results are returned; update the post-parse validation in the block that handles
returncode==0 to also compare the number of parsed results against the expected
count (len(sketch_project_dirs)) and fail if they differ. Specifically, after
calling _parse_compile_many_results(output) and computing success from
returncode, detect if success and output.strip() but (not sketch_results or
len(sketch_results) != len(sketch_project_dirs)), then print a descriptive
message and return an FbuildCompileManyResult with success=False, the raw output
and returncode so callers fail fast on partial parse drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7420ef22-2b34-4234-b893-b64e69eafb41
📒 Files selected for processing (2)
ci/tests/test_fbuild_compile_many.pyci/util/fbuild_runner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/tests/test_fbuild_compile_many.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
966e21c to
89e4674Compare89e4674 to
4d1efb0CompareThere was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/compiler/pio.py (1)
57-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new
build_dirparameter broke the existing positional call path.Adding
build_dirbefore theadditional_*parameters shifts the positional call in_internal_init_build_no_lock():self.additional_definesnow binds tobuild_dir,self.additional_include_dirsbinds toadditional_defines, andself.additional_libsbinds toadditional_include_dirs. That means any non-Noneadditional_defineswill blow up onbuild_dir.mkdir(...), and even theNonecase silently misroutes include/lib overrides.Suggested fix
def _init_platformio_build( board: Board, verbose: bool, example: str, paths: FastLEDPaths, + *, build_dir: Optional[Path] = None, additional_defines: Optional[list[str]] = None, additional_include_dirs: Optional[list[str]] = None, additional_libs: Optional[list[str]] = None, use_fbuild: bool = False, ) -> InitResult:result = _init_platformio_build( self.board, self.verbose, example, self.paths, - self.additional_defines,- self.additional_include_dirs,- self.additional_libs,+ additional_defines=self.additional_defines,+ additional_include_dirs=self.additional_include_dirs,+ additional_libs=self.additional_libs, use_fbuild=self.use_fbuild, )🤖 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 `@ci/compiler/pio.py` around lines 57 - 67, The new positional parameter build_dir in _init_platformio_build shifts downstream positional arguments and breaks calls (e.g., _internal_init_build_no_lock), causing self.additional_defines/include_dirs/libs to bind incorrectly; fix by making build_dir a keyword-only parameter (move it after a * or place it after additional_* so callers aren’t affected) or update all call sites (notably _internal_init_build_no_lock) to pass build_dir by name; ensure the signature change is applied consistently and tests/callers use keywords to prevent positional misbinding of additional_defines/additional_include_dirs/additional_libs.
🤖 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.
Outside diff comments:
In `@ci/compiler/pio.py`:
- Around line 57-67: The new positional parameter build_dir in
_init_platformio_build shifts downstream positional arguments and breaks calls
(e.g., _internal_init_build_no_lock), causing
self.additional_defines/include_dirs/libs to bind incorrectly; fix by making
build_dir a keyword-only parameter (move it after a * or place it after
additional_* so callers aren’t affected) or update all call sites (notably
_internal_init_build_no_lock) to pass build_dir by name; ensure the signature
change is applied consistently and tests/callers use keywords to prevent
positional misbinding of
additional_defines/additional_include_dirs/additional_libs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b103a12-6a6f-45fd-9a22-782aeac473b9
📒 Files selected for processing (4)
ci/compiler/pio.pyci/tests/test_fbuild_compile_many.pyci/util/cpu_count.pyci/util/fbuild_runner.py
💤 Files with no reviewable changes (1)
- ci/util/cpu_count.py
Uh oh!
There was an error while loading. Please reload this page.
Summary
Testing
Closes#2470.
Summary by CodeRabbit
New Features
Improvements
Tests