Lower target and action deps (3.14.3)#315
Conversation
Draft an execution plan for lowering target and action `deps` into a new implicit-dependency class on the IR `BuildEdge` and emitting them as Ninja `|` implicit dependencies between explicit inputs and `||` order-only deps. The plan records the cycle-participation decision (explicit inputs and implicit deps participate; order-only deps do not), defers rule-level `Rule.deps` rejection to roadmap item 3.14.6, and keeps action hashing unchanged because `implicit_deps` lives on `BuildEdge` rather than `Action`. Validation runs through `make check-fmt`, `make lint`, `make test`, `make markdownlint`, `make nixie`, and `coderabbit review --agent`. The plan is DRAFT and must be approved before implementation begins. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Introduce the `BuildEdge.implicit_deps` field and initialise it as empty at every existing construction site. Keep behaviour unchanged for this step so the later manifest lowering, cycle detection, and Ninja rendering changes can be reviewed separately.
Populate `BuildEdge.implicit_deps` from manifest `deps` while keeping
`sources` as the only explicit recipe input class.
Preserve documented `{{ ins }}` and `{{ outs }}` recipe placeholders
through manifest rendering so IR interpolation can substitute them from
explicit inputs and outputs before action hashing.
Traverse `BuildEdge.implicit_deps` alongside explicit inputs during IR cycle analysis while continuing to ignore order-only dependencies. Add regression coverage for implicit-only cycles, mixed cycles, missing implicit dependencies, and bounded small cycles without adding a new test dependency.
Emit `BuildEdge.implicit_deps` with Ninja's single-pipe separator between explicit inputs and order-only dependencies. Cover explicit, implicit-only, order-only, and phony action edge shapes so the separator ordering remains stable.
Add an end-to-end manifest fixture that lowers target and action `deps`
into IR implicit deps and renders them as Ninja single-pipe dependencies.
Assert that recipe interpolation still receives only explicit `sources`,
including the documented `{{ ins }}` placeholder path.
Record that manifest `sources` become explicit recipe inputs, `deps` become implicit dependencies, and order-only dependencies remain outside cycle detection. Align the user, developer, design, and formal-verification notes with the implemented IR and Ninja dependency classes.
Record the final deterministic validation state before CodeRabbit review and note the existing Markdown formatting exception that prevents using `make fmt` as an all-or-nothing gate for this branch.
Mark the implicit dependency lowering roadmap item complete after the implementation, deterministic gates, and CodeRabbit review all passed. Update the ExecPlan with the final review and validation state.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Mark the ExecPlan complete after pushing the branch and refreshing the draft pull request with the final validation and reviewer context.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| cycle.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Code Duplicationsrc/ir/cycle.rs: What lead to degradation?The module contains 2 functions with similar structure: tests.cycle_detector_records_missing_dependencies,tests.cycle_detector_records_missing_implicit_dependencies Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
1 participant
Summary
This branch implements roadmap task (3.14.3) by lowering target and action
depsinto a dedicated implicit-dependency class in the IR and generated Ninja output. It keepssourcesas the explicit recipe-input class used by$inand{{ ins }}, whiledepsnow affect dependency ordering, rebuild decisions, and cycle detection without appearing in recipe arguments.Roadmap task: (3.14.3)
Execplan: docs/execplans/3-14-3-lower-target-and-action-deps.md
Review walkthrough
BuildEdge.implicit_depsfield and manifest lowering fromtarget.deps.| <implicit_deps>rendering.{{ ins }}/{{ outs }}placeholder preservation needed by the documented recipe contract.Validation
cargo test --workspace: passed after the IR field addition.cargo test --test ir_from_manifest_tests: passed with 15 tests after manifest lowering.cargo test -p netsuke ir::cycle: passed after implicit-dependency cycle traversal.cargo test --test ninja_gen_tests: passed after Ninja emission updates.cargo test --test bdd_tests implicit: passed after behavioural coverage was added.make check-fmt: passed in the final gate.make lint: passed in the final gate.make test: passed in the final gate.make markdownlint: passed in the final gate.make nixie: passed in the final gate.coderabbit review --agent: completed with zero findings.Notes
make fmtstill triggers a pre-existing repository-wide Markdown formatting backlog and rewrites unrelated documentation before failing. This branch restored that unrelated churn and used the non-mutating gates above as the applicable completion checks.No new external crate dependencies were added. The planned
proptestcoverage was replaced with deterministic bounded cycle coverage becauseproptestis not currently in the dependency graph and the execplan requires explicit approval before adding dependencies.