Generalize Boost/C++ patch pipeline behind config and plugin points (#69)#73
Conversation
📝 WalkthroughWalkthroughThis PR generalizes the previously Boost.Capy-specific patch pipeline. ChangesConfigurable Patch Pipeline and Project Settings
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cli/localci/core/patch_registry.py (1)
27-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo error handling around
ep.load().If a registered plugin's entry point fails to import (bad package, missing dependency),
get_patch_step_registry()raises unhandled, breakingPatchesConfigvalidation (and thus the whole CLI) even for unrelated config changes. Consider catching and logging load failures per entry point so one broken plugin doesn't take down config parsing.🤖 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 `@cli/localci/core/patch_registry.py` around lines 27 - 32, The get_patch_step_registry() entry point loading path currently calls ep.load() without protection, so one broken plugin can crash PatchesConfig validation and the CLI. Update the registry-building loop in get_patch_step_registry() to catch import/load failures for each entry point, log the failure with the entry point name and error details, and continue processing the remaining plugins so unrelated config parsing still succeeds.cli/localci/core/config.py (1)
290-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefault literals duplicated across three files.
ProjectConfig.repo_full_name/native_image_prefixdefaults ("cppalliance/capy"/"capy-") are re-declared as fallback literals inorchestrator.py'sOrchestratorConfig.from_config(getattr(project, "repo_full_name", "cppalliance/capy")) and incommand_builder.py'sActCommandBuilder.__init__(self.repo_full_name = repo_full_name or "cppalliance/capy"). Three independent copies of the same magic strings risk drifting if the default ever changes.♻️ Suggested consolidation
+DEFAULT_REPO_FULL_NAME = "cppalliance/capy" +DEFAULT_NATIVE_IMAGE_PREFIX = "capy-" + class ProjectConfig(BaseModel): """Project identity and image conventions for act command building.""" - repo_full_name: str = "cppalliance/capy" - native_image_prefix: str = "capy-" + repo_full_name: str = DEFAULT_REPO_FULL_NAME + native_image_prefix: str = DEFAULT_NATIVE_IMAGE_PREFIXThen import
DEFAULT_REPO_FULL_NAME/DEFAULT_NATIVE_IMAGE_PREFIXinorchestrator.pyandcommand_builder.pyinstead of re-typing the literals.🤖 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 `@cli/localci/core/config.py` around lines 290 - 295, The default repo/image strings are duplicated as magic literals across ProjectConfig, OrchestratorConfig.from_config, and ActCommandBuilder.__init__, so consolidate them into shared defaults and reuse those symbols instead of retyping "cppalliance/capy" and "capy-". Update the fallback logic in orchestrator.py and command_builder.py to reference the existing ProjectConfig defaults or imported DEFAULT_REPO_FULL_NAME/DEFAULT_NATIVE_IMAGE_PREFIX so all three places stay in sync if the defaults change.cli/localci/core/patch_steps.py (1)
171-195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefix-only match for workspace copy line is looser than necessary.
dest_prefix = dest.split("/")[0]only checks the first path segment (e.g."libs"/"vendor") is present in the line, rather than the full configureddeststring (e.g."libs/$module"). Since the fixture/tests showdestappears verbatim in the workflow source, matching the full string would be equally simple and more precise, avoiding accidental matches on unrelated lines that happen to contain the prefix substring.♻️ Suggested tightening
- dest_prefix = dest.split("/")[0] for i, line in enumerate(ctx.lines): - if marker in line and dest_prefix in line: + if marker in line and dest in line:🤖 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 `@cli/localci/core/patch_steps.py` around lines 171 - 195, The workspace copy replacement in apply() is using a prefix-only check via dest_prefix, which is too broad and can match unrelated lines. Update the matching logic in PatchSteps.apply to look for the full configured proj.workspace_libs_copy_dest string together with the marker instead of splitting dest, so the correct workspace copy line is targeted precisely. Keep the replacement behavior unchanged; only tighten the condition used to locate the line.
🤖 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 `@cli/localci/core/config.py`:
- Around line 290-295: The default repo/image strings are duplicated as magic
literals across ProjectConfig, OrchestratorConfig.from_config, and
ActCommandBuilder.__init__, so consolidate them into shared defaults and reuse
those symbols instead of retyping "cppalliance/capy" and "capy-". Update the
fallback logic in orchestrator.py and command_builder.py to reference the
existing ProjectConfig defaults or imported
DEFAULT_REPO_FULL_NAME/DEFAULT_NATIVE_IMAGE_PREFIX so all three places stay in
sync if the defaults change.
In `@cli/localci/core/patch_registry.py`:
- Around line 27-32: The get_patch_step_registry() entry point loading path
currently calls ep.load() without protection, so one broken plugin can crash
PatchesConfig validation and the CLI. Update the registry-building loop in
get_patch_step_registry() to catch import/load failures for each entry point,
log the failure with the entry point name and error details, and continue
processing the remaining plugins so unrelated config parsing still succeeds.
In `@cli/localci/core/patch_steps.py`:
- Around line 171-195: The workspace copy replacement in apply() is using a
prefix-only check via dest_prefix, which is too broad and can match unrelated
lines. Update the matching logic in PatchSteps.apply to look for the full
configured proj.workspace_libs_copy_dest string together with the marker instead
of splitting dest, so the correct workspace copy line is targeted precisely.
Keep the replacement behavior unchanged; only tighten the condition used to
locate the line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b33c2e6-abe7-4ef9-b7c1-2f3943ea1864
📒 Files selected for processing (11)
.pre-commit-config.yamlcli/localci/core/command_builder.pycli/localci/core/config.pycli/localci/core/orchestrator.pycli/localci/core/patch_pipeline.pycli/localci/core/patch_registry.pycli/localci/core/patch_steps.pycli/tests/fixtures/patcher/generic_cpp.ymlcli/tests/test_executor.pycli/tests/test_patch_pipeline.pycli/tests/test_patcher_patches.py
bradjin8
force-pushed
the
feat/generalize-patch-pipeline
branch
from
c3a5c06 to
bf1b8b2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/localci/core/patch_steps.py (2)
184-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winC++ source extensions are hardcoded in the file-stat scan.
The
findfilter is fixed to*.cpp/*.hpp/*.h/*.ipp. For projects using other extensions (e.g..cc,.cxx,.hh,.tcc), incremental-build stat records will be incomplete, silently defeating the timestamp-restore optimization. Given the generalization objective, consider making the extension set project-configurable.🤖 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 `@cli/localci/core/patch_steps.py` at line 184, The file-stat scan in patch_steps.py hardcodes only a small set of C++ extensions, so incremental build stat records will miss files like .cc, .cxx, .hh, and .tcc. Update the logic that generates the find command in the patch-building flow to use a configurable extension list instead of the fixed filter, and thread that configuration through the existing patch steps/helpers so projects can define their own source/header extensions.
205-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGeneralization is incomplete: step title and
b2binary name remain hardcoded.Unlike
RestoreCapyTimestampsStep, which now sources its title fromproj.restore_timestamps_step_title, this step still hardcodes"Skip b2 bootstrap"(the idempotency marker at Line 216 and the inserted title at Line 226) and the cached-binary nameb2at Line 228. That keeps Boost/Capy-specific literals baked into the step despite the PR goal of lifting them into project config. Consider parameterizing these viaPatchProjectConfigfor consistency with the other steps.🤖 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 `@cli/localci/core/patch_steps.py` around lines 205 - 236, The new step in patch_steps.py still hardcodes Boost-specific literals instead of using project configuration. Update the logic in Skip b2 bootstrap insertion to source the step title/idempotency marker and the cached binary name from PatchProjectConfig via _project_settings(ctx), similar to RestoreCapyTimestampsStep using proj.restore_timestamps_step_title. Replace the fixed “Skip b2 bootstrap” and “b2” references in the step construction and already_patched check with config-backed values so the behavior stays project-agnostic and consistent across workflows.
🤖 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 `@cli/localci/core/patch_steps.py`:
- Line 184: The file-stat scan in patch_steps.py hardcodes only a small set of
C++ extensions, so incremental build stat records will miss files like .cc,
.cxx, .hh, and .tcc. Update the logic that generates the find command in the
patch-building flow to use a configurable extension list instead of the fixed
filter, and thread that configuration through the existing patch steps/helpers
so projects can define their own source/header extensions.
- Around line 205-236: The new step in patch_steps.py still hardcodes
Boost-specific literals instead of using project configuration. Update the logic
in Skip b2 bootstrap insertion to source the step title/idempotency marker and
the cached binary name from PatchProjectConfig via _project_settings(ctx),
similar to RestoreCapyTimestampsStep using proj.restore_timestamps_step_title.
Replace the fixed “Skip b2 bootstrap” and “b2” references in the step
construction and already_patched check with config-backed values so the behavior
stays project-agnostic and consistent across workflows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d5b3402-7eb9-40ed-a488-dc3918014b8b
📒 Files selected for processing (12)
README.mdcli/Usage Guide.mdcli/localci/core/command_builder.pycli/localci/core/config.pycli/localci/core/orchestrator.pycli/localci/core/patch_pipeline.pycli/localci/core/patch_registry.pycli/localci/core/patch_steps.pycli/tests/fixtures/patcher/generic_cpp.ymlcli/tests/test_executor.pycli/tests/test_patch_pipeline.pycli/tests/test_patcher_patches.py
🚧 Files skipped from review as they are similar to previous changes (11)
- README.md
- cli/localci/core/patch_registry.py
- cli/tests/fixtures/patcher/generic_cpp.yml
- cli/localci/core/patch_pipeline.py
- cli/tests/test_executor.py
- cli/Usage Guide.md
- cli/localci/core/command_builder.py
- cli/tests/test_patcher_patches.py
- cli/localci/core/orchestrator.py
- cli/tests/test_patch_pipeline.py
- cli/localci/core/config.py
3 participants
Summary
boost-source/boost-root,Patch Boost,capy-root,b2-workflow,libs/$module, etc.) intopatches.project(PatchProjectConfig) so other C++/GitHub Actions projects can retarget the existing steps without code changes.patch_registry.pyandpatches.extra_stepsso custom patch steps can register via thelocalci.patch_stepsentry-point group instead of editingPATCH_STEP_REGISTRY.repo_full_nameandnative_image_prefixconfigurable viaProjectConfig, wired through the orchestrator intoActCommandBuilder(replaces hardcodedcppalliance/capyandcapy-defaults)..localci.ymlbehaviour is unchanged and existing patcher fixtures still pass.Closes #69
Test plan
pytest— 593 tests passpre-commit run -a— ruff, format, mypy, yaml hooks passcli/tests/fixtures/patcher/) patch identically with default configgeneric_cpp.ymlfixture patches correctly with custompatches.projectliteralsextra_steps+ registry monkeypatch testActCommandBuildertests for configurablerepo_full_nameandnative_image_prefix(x86 /linux/386gating)Summary by CodeRabbit
New Features
Bug Fixes