Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 42 additions & 8 deletions docs/netsuke-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ top-level keys.
the sources it depends on, and the rule used to produce it. This corresponds
to a Ninja `build` statement.3

- `actions`: A secondary list of build targets. Any target placed here is
treated as `{ phony: true, always: false }` by default.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
- `defaults`: An optional list of target names to be built when Netsuke is
invoked without any specific targets on the command line. This maps directly
to Ninja's `default` target statement.3
Expand All @@ -178,8 +181,8 @@ Each entry in the `rules` list is a mapping that defines a command template.

### 2.4 Defining `targets`

Each entry in the `targets` list is a mapping that defines a build edge in the
dependency graph.
Each entry in `targets` defines a build edge; placing a target in the optional
`actions` list instead marks it as `phony: true` with `always` left `false`.

- `name`: The primary output file or files for this build step. This can be a
single string or a list of strings.
Expand Down Expand Up @@ -210,6 +213,12 @@ dependency graph.
YAML `|` block style. Netsuke registers these macros in the template
environment before rendering other sections.

- `phony`: When set to `true`, the target runs when explicitly requested even if
a file with the same name exists. The default value is `false`.

- `always`: When set to `true`, the target runs on every invocation regardless
of timestamps or dependencies. The default value is `false`.

### 2.5 Generated Targets with `foreach`

Large sets of similar outputs can clutter a manifest when written individually.
Expand Down Expand Up @@ -302,6 +311,9 @@ pub struct NetsukeManifest {
#[serde(default)]
pub rules: Vec<Rule>,

#[serde(default)]
pub actions: Vec<Target>,

pub targets: Vec<Target>,
Comment on lines +315 to 317
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Document the default semantics for actions directly in the struct doc-comment.

Readers jumping to the code block should not have to infer behaviour from
earlier prose. Add a short comment beside the new actions field.

-    #[serde(default)]
-    pub actions: Vec<Target>,
+    /// A second target list; every entry is implicitly `{ phony: true }`.
+    #[serde(default)]
+    pub actions: Vec<Target>,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub actions: Vec<Target>,
pub targets: Vec<Target>,
/// A second target list; every entry is implicitly `{ phony: true }`.
#[serde(default)]
pub actions: Vec<Target>,
pub targets: Vec<Target>,
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 317 to 319, the new `actions` field lacks
an inline comment explaining its default semantics. Add a brief comment directly
beside the `actions` field in the struct to clearly document its default
behavior, so readers can understand it immediately without referring back to
earlier text.


#[serde(default)]
Expand Down Expand Up @@ -336,6 +348,14 @@ pub struct Target {

#[serde(default)]
pub vars: HashMap<String, String>,

/// Run this target when requested even if a file with the same name exists.
#[serde(default)]
pub phony: bool,

/// Run this target on every invocation regardless of timestamps.
#[serde(default)]
pub always: bool,
}

/// An enum to handle fields that can be either a single string or a list of strings.
Expand Down Expand Up @@ -615,7 +635,9 @@ pub struct Action {
}

/// Represents a single build statement, analogous to a Ninja 'build' edge.
/// It connects a set of inputs to a set of outputs via an Action.
/// It connects a set of inputs to a set of outputs via an Action. The `phony`
/// and `always` flags control execution when outputs already exist or when
/// timestamps would normally skip the step.
pub struct BuildEdge {
/// The unique identifier of the Action used for this edge.
pub action_id: String,
Expand All @@ -632,6 +654,12 @@ pub struct BuildEdge {
/// Dependencies that must be built first but do not trigger a rebuild on change.
/// Maps to Ninja's '||' syntax.
pub order_only_deps: Vec<PathBuf>,

/// Run this edge when requested even if the output file already exists.
pub phony: bool,

/// Run this edge on every invocation regardless of timestamps.
pub always: bool,
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
```

Expand All @@ -646,13 +674,15 @@ This transformation involves several steps:
stored in the `BuildGraph`'s `actions` map, keyed by a hash of their contents
to automatically deduplicate identical rules.

2. **Target Expansion:** Iterate through the `manifest.targets`. For each target
in the AST, resolve all strings into `PathBuf`s and resolve all dependency
names against other targets.
2. **Target Expansion:** Iterate through the `manifest.targets` and the optional
`manifest.actions`. Entries in `actions` are treated identically to targets
but with `phony` defaulting to `true`. For each item, resolve all strings
into `PathBuf`s and resolve all dependency names against other targets.

3. **Edge Creation:** For each AST target, create an `ir::BuildEdge` object.
This involves linking it to the appropriate `ir::Action` (by its ID), and
populating its input and output vectors.
This involves linking it to the appropriate `ir::Action` (by its ID),
transferring the `phony` and `always` flags, and populating its input and
output vectors.

4. **Graph Validation:** As the graph is constructed, perform validation checks.
This includes ensuring that every rule referenced by a target exists in the
Expand Down Expand Up @@ -693,6 +723,10 @@ structures to the Ninja file syntax.
`ir::BuildEdge`, write a corresponding Ninja `build` statement. This involves
formatting the lists of explicit outputs, implicit outputs, inputs, and
order-only dependencies using the correct Ninja syntax (`:`, `|`, and `||`).7
Use Ninja's built-in `phony` rule when `phony` is `true`. For an `always`
edge, either generate a `phony` build with no outputs or emit a dummy output
marked `restat = 1` and depend on a permanently dirty target so the command
runs on each invocation.

Code snippet

Expand Down
3 changes: 3 additions & 0 deletions docs/roadmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ compilation pipeline from parsing to execution.
- [ ] Annotate the AST structs with `#[derive(Deserialize)]` and
`#[serde(deny_unknown_fields)]` to enable parsing.

- [ ] Support `phony` and `always` boolean flags on targets and parse an
optional `actions` list that treats each entry as a `phony` target.

Comment on lines +22 to +24
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Wrap the new bullet to ≤ 80 columns and add the serial comma.

The added bullet exceeds the 80-column soft limit and omits the Oxford comma mandated by the style guide. Re-wrap and punctuate correctly.

-  - [ ] Support `phony` and `always` boolean flags on targets and parse an
-    optional `actions` list that treats each entry as a `phony` target.
+  - [ ] Support `phony` and `always` boolean flags on targets, and parse an
+    optional `actions` list that treats each entry as a `phony` target.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- [ ] Support `phony` and `always` boolean flags on targets and parse an
optional `actions` list that treats each entry as a `phony` target.
- [ ] Support `phony` and `always` boolean flags on targets, and parse an
optional `actions` list that treats each entry as a `phony` target.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~22-~22: Possible missing comma found.
Context: ...t phony and always boolean flags on targets and parse an optional actions lis...

(AI_HYDRA_LEO_MISSING_COMMA)

🤖 Prompt for AI Agents
In docs/roadmap.md around lines 22 to 24, the new bullet point exceeds 80
columns and lacks the serial (Oxford) comma. Reformat the bullet text to wrap
lines at or below 80 characters and add the serial comma before the last item in
the list to comply with the style guide.

- [ ] Implement the YAML parsing logic using `serde_yaml` to deserialize a
static `Netsuke.yml` file into the `NetsukeManifest` AST.

Expand Down