Conversation
Reviewer's GuideThis PR introduces a new Ninja file generator module with deterministic output, exposes it through the public library API and CLI test harness, updates documentation and roadmap tasks to reflect completion, and adds comprehensive unit and behavior tests. Class diagram for the new Ninja generator moduleclassDiagram
class BuildGraph {
+HashMap<String, Action> actions
+HashMap<PathBuf, BuildEdge> targets
+Vec<PathBuf> default_targets
}
class Action {
+Recipe recipe
+Option<String> description
+Option<String> depfile
+Option<String> deps_format
+Option<String> pool
+bool restat
}
class BuildEdge {
+String action_id
+Vec<PathBuf> inputs
+Vec<PathBuf> explicit_outputs
+Vec<PathBuf> implicit_outputs
+Vec<PathBuf> order_only_deps
+bool phony
+bool always
}
class Recipe {
<<enum>>
Command
Script
Rule
}
class ninja_gen {
+generate(graph: &BuildGraph) String
}
BuildGraph "1" o-- "many" Action : actions
BuildGraph "1" o-- "many" BuildEdge : targets
Action "1" o-- "1" Recipe
BuildEdge "1" --> "1" Action : action_id
ninja_gen ..> BuildGraph : uses
ninja_gen ..> Action : uses
ninja_gen ..> BuildEdge : uses
ninja_gen ..> Recipe : uses
Class diagram for public API exposure of the Ninja generatorclassDiagram
class ninja_gen {
+generate(graph: &BuildGraph) String
}
class lib {
+pub mod ninja_gen
}
lib --> ninja_gen : exposes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughIntroduce a new Ninja build file generator module ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant World as CliWorld
participant NinjaGen as ninja_gen::generate
participant Ninja as Ninja Binary
Test->>World: Retrieve BuildGraph
Test->>NinjaGen: Call generate(BuildGraph)
NinjaGen-->>Test: Return Ninja file content
Test->>World: Store Ninja file content
Test->>Ninja: Run Ninja commands with generated file
Ninja-->>Test: Return build results
Test->>Test: Assert output and no-op behaviour
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Instead of ignoring write errors (using
let _ = writeln!(…)), consider propagating or unwrapping them to catch unexpected formatting issues during Ninja generation. - Edge sorting currently compares
Vec<PathBuf>directly; using a joined string representation of the paths as a sort key would ensure consistent ordering across platforms. - Deduplication only checks the first explicit output, which may miss duplicate edges with the same later outputs—consider deduplicating based on the full set of outputs or clarifying this behavior in the documentation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of ignoring write errors (using `let _ = writeln!(…)`), consider propagating or unwrapping them to catch unexpected formatting issues during Ninja generation.
- Edge sorting currently compares `Vec<PathBuf>` directly; using a joined string representation of the paths as a sort key would ensure consistent ordering across platforms.
- Deduplication only checks the first explicit output, which may miss duplicate edges with the same later outputs—consider deduplicating based on the full set of outputs or clarifying this behavior in the documentation.
## Individual Comments
### Comment 1
<location> `src/ninja_gen.rs:64` </location>
<code_context>
+
+fn write_edges(out: &mut String, targets: &HashMap<PathBuf, BuildEdge>) {
+ let mut edges: Vec<&BuildEdge> = targets.values().collect();
+ edges.sort_by(|a, b| a.explicit_outputs.cmp(&b.explicit_outputs));
+ let mut seen = HashSet::new();
+ for edge in edges {
</code_context>
<issue_to_address>
Sorting edges by explicit_outputs may not guarantee a stable order if explicit_outputs contains multiple paths.
If explicit_outputs contains multiple paths, the current sorting may be non-deterministic. To ensure stable output, sort explicit_outputs within each edge before sorting the edges, or implement a more robust comparison method.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
fn write_edges(out: &mut String, targets: &HashMap<PathBuf, BuildEdge>) {
let mut edges: Vec<&BuildEdge> = targets.values().collect();
edges.sort_by(|a, b| a.explicit_outputs.cmp(&b.explicit_outputs));
let mut seen = HashSet::new();
for edge in edges {
=======
fn write_edges(out: &mut String, targets: &HashMap<PathBuf, BuildEdge>) {
let mut edges: Vec<&BuildEdge> = targets.values().collect();
// Sort explicit_outputs within each edge for stable comparison
for edge in &mut edges {
// We need to sort the explicit_outputs for each edge.
// Since edges is Vec<&BuildEdge>, and BuildEdge is likely not mutable here,
// we need to collect sorted keys for comparison instead.
// So, instead, sort by a tuple of sorted explicit_outputs.
}
edges.sort_by(|a, b| {
let mut a_outputs = a.explicit_outputs.clone();
let mut b_outputs = b.explicit_outputs.clone();
a_outputs.sort();
b_outputs.sort();
a_outputs.cmp(&b_outputs)
});
let mut seen = HashSet::new();
for edge in edges {
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/ninja_gen.rs:93` </location>
<code_context>
+ let _ = write!(out, " || {}", join(&edge.order_only_deps));
+ }
+ let _ = writeln!(out);
+ if edge.always {
+ let _ = writeln!(out, " restat = 1");
+ }
+ let _ = writeln!(out);
</code_context>
<issue_to_address>
Setting 'restat = 1' for 'always' may conflict with action-level restat.
Clarify which 'restat' setting takes precedence, or ensure only one source sets it to avoid duplicate lines.
Suggested implementation:
```rust
let _ = writeln!(out);
// Only set 'restat = 1' if not already set by the action.
// Action-level 'restat' takes precedence over 'always'.
if !edge.action_restat && edge.always {
let _ = writeln!(out, " restat = 1");
}
let _ = writeln!(out);
```
If there is no `edge.action_restat` boolean or equivalent, you will need to add logic to detect if the action-level restat is set, or adjust the condition to fit your codebase's conventions for action-level restat.
</issue_to_address>
### Comment 3
<location> `src/ninja_gen.rs:16` </location>
<code_context>
+
+/// Generate a Ninja build file as a string.
+#[must_use]
+pub fn generate(graph: &BuildGraph) -> String {
+ let mut out = String::new();
+ write_rules(&mut out, &graph.actions);
</code_context>
<issue_to_address>
Consider replacing the multiple formatting helper functions with `Display` trait implementations for actions and edges to simplify and clarify the code.
Here is one way to collapse all of the little `write_*` helpers and `join` into two `Display` impls. Once you have these, your `generate` function becomes just “sort + write!(&edge)” or “sort + write!(&NamedAction)”, which is both shorter and more intention-revealing.
```rust
use std::fmt::{self, Display, Formatter};
use itertools::Itertools; // add `itertools = "0.10"` to Cargo.toml
/// Wrapper so we can include the rule‐name (the HashMap key) in the Display impl.
struct NamedAction<'a> {
id: &'a str,
action: &'a crate::ir::Action,
}
impl Display for NamedAction<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
writeln!(f, "rule {}", self.id)?;
match &self.action.recipe {
Recipe::Command { command } => writeln!(f, " command = {}", command)?,
Recipe::Script { script } => {
writeln!(f, " command = /bin/sh -e -c \"")?;
for line in script.lines() {
writeln!(f, " {}", line)?;
}
writeln!(f, " \"")?;
}
_ => unreachable!(),
}
if let Some(desc) = &self.action.description {
writeln!(f, " description = {}", desc)?;
}
if let Some(depfile) = &self.action.depfile {
writeln!(f, " depfile = {}", depfile)?;
}
if let Some(deps) = &self.action.deps_format {
writeln!(f, " deps = {}", deps)?;
}
if let Some(pool) = &self.action.pool {
writeln!(f, " pool = {}", pool)?;
}
if self.action.restat {
writeln!(f, " restat = 1")?;
}
writeln!(f) // blank line
}
}
impl Display for crate::ir::BuildEdge {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
let out = |p: &[std::path::PathBuf]| p.iter().map(|p| p.display()).join(" ");
write!(f, "build {}", out(&self.explicit_outputs))?;
if !self.implicit_outputs.is_empty() {
write!(f, " | {}", out(&self.implicit_outputs))?;
}
let rule = if self.phony { "phony" } else { &self.action_id };
write!(f, ": {}", rule)?;
if !self.inputs.is_empty() {
write!(f, " {}", out(&self.inputs))?;
}
if !self.order_only_deps.is_empty() {
write!(f, " || {}", out(&self.order_only_deps))?;
}
writeln!(f)?;
if self.always {
writeln!(f, " restat = 1")?;
}
writeln!(f)
}
}
```
And now your `generate` can be shrunk to:
```rust
pub fn generate(graph: &BuildGraph) -> String {
let mut out = String::new();
// rules
let mut acts: Vec<_> = graph.actions.iter().collect();
acts.sort_by_key(|(id, _)| *id);
for (id, action) in acts {
write!(out, "{}", NamedAction { id, action }).unwrap();
}
// edges
let mut edges: Vec<_> = graph.targets.values().collect();
edges.sort_by(|a, b| a.explicit_outputs.cmp(&b.explicit_outputs));
let mut seen = std::collections::HashSet::new();
for e in edges {
if e.explicit_outputs.first().map_or(false, |p| !seen.insert(p.clone())) {
continue;
}
write!(out, "{}", e).unwrap();
}
// defaults
if !graph.default_targets.is_empty() {
let defs = graph
.default_targets
.iter()
.sorted()
.map(|p| p.display())
.join(" ");
writeln!(out, "default {}", defs).unwrap();
}
out
}
```
This removes all of the `write_rules`, `write_edge(s)`, `join`, `write_defaults` plumbing and puts the formatting where it belongs—in small, focused `Display` impls.
</issue_to_address>
### Comment 4
<location> `tests/features/ninja.feature:1` </location>
<code_context>
+Feature: Ninja file generation
+
+ Scenario: Generate build statements
</code_context>
<issue_to_address>
Consider replacing the Cucumber feature with simple pytest functions to test Ninja file generation.
The Cucumber feature here is adding a whole new DSL layer just to check that your generated Ninja text contains a couple of strings. You can get the same coverage with two tiny pytest functions—no feature files or step-defs required.
For example, under `tests/` create something like:
```python
# tests/test_ninja_generation.py
import pytest
from mypkg.compiler import compile_manifest
from mypkg.ninja import generate_ninja
@pytest.mark.parametrize("yml, substrings", [
("tests/data/rules.yml", ["rule", "build hello.o:"]),
("tests/data/phony.yml", ["build clean: phony"]),
])
def test_generate_ninja_contains_expected(yml, substrings):
ir = compile_manifest(yml)
ninja_text = generate_ninja(ir)
for sub in substrings:
assert sub in ninja_text
```
If you prefer “golden files” for full-output comparison, you can do:
```python
def test_ninja_matches_golden(tmp_path):
ir = compile_manifest("tests/data/rules.yml")
ninja_text = generate_ninja(ir)
golden = (tmp_path / "rules.ninja")
if not golden.exists():
golden.write_text(ninja_text) # first run: creates golden
pytest.skip("Golden file created; re-run to verify")
assert ninja_text == golden.read_text()
```
This keeps all current assertions, removes the extra Gherkin layer, and is just as readable and maintainable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/ninja_gen.rs (3)
62-76: Address sorting stability concern raised in past review.The sorting by
explicit_outputsmay be non-deterministic when outputs contain multiple paths, as flagged in the previous review. The deduplication logic using the first output also compounds this issue.Apply this fix to ensure stable sorting:
fn write_edges(out: &mut String, targets: &HashMap<PathBuf, BuildEdge>) { let mut edges: Vec<&BuildEdge> = targets.values().collect(); - edges.sort_by(|a, b| a.explicit_outputs.cmp(&b.explicit_outputs)); + edges.sort_by(|a, b| { + let mut a_outputs = a.explicit_outputs.clone(); + let mut b_outputs = b.explicit_outputs.clone(); + a_outputs.sort(); + b_outputs.sort(); + a_outputs.cmp(&b_outputs) + }); let mut seen = HashSet::new();
93-95: Clarify restat precedence to avoid conflicts.The edge-level
restat = 1whenalwaysis true could conflict with action-level restat settings, as noted in the previous review.Add logic to prevent duplicate restat declarations:
let _ = writeln!(out); - if edge.always { + // Only set restat if not already set by the action + if edge.always && !action_has_restat { let _ = writeln!(out, " restat = 1"); }
16-22: Consider Display trait implementation for cleaner code.The previous comprehensive review suggested replacing helper functions with
Displayimplementations for actions and edges. This would significantly simplify the code and improve maintainability.The suggested
Displayimplementation would reduce thegeneratefunction to just sorting and writing, making the code more intention-revealing and easier to maintain.tests/features/ninja.feature (1)
1-13: Consider the complexity trade-off of Cucumber for simple assertions.The Gherkin scenarios are well-structured and readable, but essentially test string containment. The past review comment correctly identifies that simple unit tests would achieve the same coverage with less complexity overhead.
However, if this aligns with an established BDD testing strategy across the project for consistency, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktests/snapshots/ninja/ninja_snapshot_tests__touch_manifest_ninja.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.github/workflows/ci.yml(1 hunks)Cargo.toml(1 hunks)docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/lib.rs(1 hunks)src/ninja_gen.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/ninja.feature(1 hunks)tests/ninja_gen_tests.rs(1 hunks)tests/ninja_snapshot_tests.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/ninja_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/cucumber.rstests/steps/mod.rssrc/lib.rstests/ninja_gen_tests.rstests/ninja_snapshot_tests.rssrc/ninja_gen.rstests/steps/ninja_steps.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/cucumber.rstests/steps/mod.rssrc/lib.rstests/ninja_gen_tests.rstests/ninja_snapshot_tests.rssrc/ninja_gen.rstests/steps/ninja_steps.rs
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/roadmap.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/netsuke-design.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-27T17:59:36.506Z
Learning: Applies to docs/src/ninja_gen.rs : Implement the Ninja file synthesizer in src/ninja_gen.rs to traverse the BuildGraph IR.
📚 Learning: applies to docs/src/ninja_gen.rs : implement the ninja file synthesizer in src/ninja_gen.rs to trave...
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-27T17:59:36.506Z
Learning: Applies to docs/src/ninja_gen.rs : Implement the Ninja file synthesizer in src/ninja_gen.rs to traverse the BuildGraph IR.
Applied to files:
docs/roadmap.md
📚 Learning: applies to docs/src/ninja_gen.rs : write logic to generate ninja rule statements from ir::action str...
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-27T17:59:36.506Z
Learning: Applies to docs/src/ninja_gen.rs : Write logic to generate Ninja rule statements from ir::Action structs and build statements from ir::BuildEdge structs.
Applied to files:
docs/roadmap.md
🧬 Code Graph Analysis (3)
tests/ninja_gen_tests.rs (1)
src/ninja_gen.rs (1)
generate(16-22)
tests/ninja_snapshot_tests.rs (3)
src/manifest.rs (1)
from_str(33-35)src/ir.rs (1)
from_manifest(108-119)src/ninja_gen.rs (1)
generate(16-22)
tests/steps/ninja_steps.rs (1)
src/ninja_gen.rs (1)
generate(16-22)
🔇 Additional comments (16)
Cargo.toml (1)
60-61: LGTM! Dependencies follow guidelines correctly.Both dependencies use proper SemVer-compatible caret requirements and serve legitimate testing purposes for the new Ninja functionality.
src/ninja_gen.rs (2)
1-7: LGTM! Module documentation follows guidelines.The module-level documentation clearly explains the purpose and notes the deterministic output behaviour.
129-163: LGTM! Comprehensive unit test.The test covers the main generation path with a realistic example and verifies the expected output format.
tests/cucumber.rs (1)
10-10: LGTM! Clean addition to test infrastructure.The optional
ninjafield follows the existing pattern and appropriately supports the new Ninja generation testing..github/workflows/ci.yml (1)
21-22: LGTM! Sensible CI enhancement.Adding the Ninja version check ensures tool availability and aids debugging for the new Ninja-related functionality.
tests/steps/mod.rs (1)
4-4: LGTM! Proper module integration.The
ninja_stepsmodule declaration follows the established pattern and correctly integrates the new Ninja testing capabilities.src/lib.rs (1)
11-11: LGTM! Clean module export addition.The new
ninja_genmodule export follows the established pattern and maintains alphabetical ordering with other public modules.docs/roadmap.md (1)
58-62: LGTM! Roadmap updates accurately reflect completed work.The checkboxes and "(done)" annotations properly mark the Ninja synthesizer implementation and rule generation logic as completed, maintaining consistency with other roadmap entries.
docs/netsuke-design.md (1)
1074-1078: LGTM! Valuable design documentation enhancements.These additions clearly describe the deterministic behaviour and comprehensive testing approach for Ninja file generation, providing important implementation details in the appropriate section.
tests/ninja_gen_tests.rs (2)
1-8: LGTM! Clean test file structure.The module documentation, imports, and use of rstest follow established patterns and testing best practices.
9-41: LGTM! Comprehensive unit test for Ninja generation.The test properly constructs IR structures and validates the complete Ninja output format. The use of
concat!for the expected string maintains readability whilst following the coding guidelines.tests/features/ninja.feature (1)
4-10: Confirm test data files exist and contain correct manifests.
tests/data/rules.ymldefines a compile rule and thehello.otarget, satisfying therule compileandbuild hello.o:assertions.tests/data/phony.ymldefines a phonycleantarget.Proceed with tests as-is.
tests/steps/ninja_steps.rs (1)
1-5: LGTM!The module documentation clearly explains the purpose and the imports are appropriate for the Cucumber step definitions.
tests/ninja_snapshot_tests.rs (3)
1-11: LGTM!The module documentation is comprehensive and clearly explains the testing approach. The imports are well-organised and appropriate.
13-21: LGTM!The helper function properly handles command execution with appropriate error messages and follows the coding guideline to use
.expect()over.unwrap().
23-42: LGTM!The test manifest is well-designed with a simple, deterministic touch rule. The error handling follows guidelines with descriptive
.expect()calls.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/ninja_gen.rs (1)
66-72: Use itertools for cleaner path joining.Extract path joining logic to avoid duplication and use a more idiomatic approach.
Use
itertools::Itertools::joinfor cleaner code:fn join(paths: &[PathBuf]) -> String { - paths - .iter() - .map(|p| p.display().to_string()) - .collect::<Vec<_>>() - .join(" ") + use itertools::Itertools; + paths.iter().map(|p| p.display()).join(" ") }tests/ninja_snapshot_tests.rs (1)
23-76: Comprehensive end-to-end validation with ninja executable dependency.The test provides excellent coverage of the entire pipeline from manifest to execution. The Python-based touch command ensures cross-platform compatibility, and the ninja validation is thorough.
Note: The test assumes
ninjaexecutable is available in PATH. This dependency is documented in the CI workflow update that runsninja --version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/snapshots/ninja/ninja_snapshot_tests__touch_manifest_ninja.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
docs/netsuke-design.md(1 hunks)src/ninja_gen.rs(1 hunks)tests/ninja_gen_tests.rs(1 hunks)tests/ninja_snapshot_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/ninja_gen_tests.rstests/ninja_snapshot_tests.rssrc/ninja_gen.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/ninja_gen_tests.rstests/ninja_snapshot_tests.rssrc/ninja_gen.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-27T17:59:36.506Z
Learning: Applies to docs/src/ninja_gen.rs : Implement the Ninja file synthesizer in src/ninja_gen.rs to traverse the BuildGraph IR.
📚 Learning: applies to docs/src/ninja_gen.rs : implement the ninja file synthesizer in src/ninja_gen.rs to trave...
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-27T17:59:36.506Z
Learning: Applies to docs/src/ninja_gen.rs : Implement the Ninja file synthesizer in src/ninja_gen.rs to traverse the BuildGraph IR.
Applied to files:
src/ninja_gen.rs
📚 Learning: applies to docs/src/ninja_gen.rs : write logic to generate ninja rule statements from ir::action str...
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-27T17:59:36.506Z
Learning: Applies to docs/src/ninja_gen.rs : Write logic to generate Ninja rule statements from ir::Action structs and build statements from ir::BuildEdge structs.
Applied to files:
src/ninja_gen.rs
🧬 Code Graph Analysis (2)
tests/ninja_gen_tests.rs (1)
src/ninja_gen.rs (1)
generate(19-63)
tests/ninja_snapshot_tests.rs (3)
src/manifest.rs (1)
from_str(33-35)src/ir.rs (1)
from_manifest(108-119)src/ninja_gen.rs (1)
generate(19-63)
🔇 Additional comments (11)
docs/netsuke-design.md (1)
1074-1083: Excellent documentation of deterministic Ninja generation.The documentation accurately describes the sorting and deduplication strategies, script formatting, and conditional field inclusion. This provides clear understanding of how the generator ensures deterministic, portable output.
src/ninja_gen.rs (5)
74-79: LGTM! Good key generation strategy.The use of null separator and sorted paths ensures collision-free, deterministic keys for deduplication.
87-118: Excellent implementation of Ninja rule formatting.The Display implementation correctly handles both command and script recipes, with proper shell wrapping for scripts and conditional inclusion of optional fields. The formatting matches Ninja syntax requirements.
126-150: Proper Ninja build statement formatting.The implementation correctly formats all Ninja build statement components. The conditional restat logic (lines 145-147) appropriately avoids conflicts between action-level and edge-level restat settings.
158-193: Basic test coverage is adequate given comprehensive testing elsewhere.The simple test validates core functionality. More comprehensive scenarios are covered in
tests/ninja_gen_tests.rsandtests/ninja_snapshot_tests.rs.
28-46: Confirm sorting stability and deduplication logic
Ensure fn path_key (src/ninja_gen.rs:75–78) sorts and joins the explicit outputs, producing a stable key. Sorting edges by this key at line 29 yields a deterministic order. HashSet deduplication at lines 32–35 removes duplicates correctly. No action required.tests/ninja_gen_tests.rs (3)
9-41: Good test coverage for phony build rules.The test correctly validates that phony edges generate the appropriate Ninja build statement with the "phony" rule.
43-85: Excellent comprehensive test coverage.This test validates script recipe formatting, all optional action fields, complex build edge configurations with implicit outputs and order-only dependencies, and default target generation. The coverage is thorough.
87-122: Excellent parameterised test for restat logic.The rstest parameterisation effectively validates the conditional restat behaviour across different combinations of action and edge flags. The assertion logic correctly verifies when restat should be included.
tests/ninja_snapshot_tests.rs (2)
13-21: Useful helper function for command execution.The helper provides consistent command execution with proper error handling and descriptive failure messages.
78-145: Excellent validation of deterministic output and deduplication.The test effectively validates that ninja generation is deterministic by comparing multiple generations, checks deduplication by counting build statements, and verifies proper ordering of rules and default targets.
|
@coderabbitai resolve |
|
@sourcery-ai resolve |
✅ Actions performedComments resolved and changes approved. |
|
Superseded by #38 |
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688a921dd3f08322b003b278f13cdce5
Summary by Sourcery
Implement a deterministic Ninja build file generator from the IR, expose it in the public API, update documentation to mark completion and describe deterministic output, and add comprehensive unit and behavior tests.
New Features:
Documentation:
Tests: