From 364318c7f37ba46f663e3582d9e32e0b80fd2a61 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 21:10:28 +0100 Subject: [PATCH 1/6] Add IR generation from manifests --- Cargo.lock | 67 ++++++++++++++ Cargo.toml | 2 + docs/netsuke-design.md | 6 ++ docs/roadmap.md | 11 ++- src/ir.rs | 156 +++++++++++++++++++++++++++++++- tests/data/duplicate_rules.yml | 21 +++++ tests/data/missing_rule.yml | 7 ++ tests/features/ir.feature | 14 +++ tests/ir_from_manifest_tests.rs | 30 ++++++ tests/steps/ir_steps.rs | 20 ++++ 10 files changed, 328 insertions(+), 6 deletions(-) create mode 100644 tests/data/duplicate_rules.yml create mode 100644 tests/data/missing_rule.yml create mode 100644 tests/ir_from_manifest_tests.rs diff --git a/Cargo.lock b/Cargo.lock index d54581da..25029ed8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -120,6 +120,15 @@ version = "2.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1b8e56985ec62d17e9c1001dc89c88ecd7dc08e47eba5ec7c29c7b5eeecde967" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bstr" version = "1.12.0" @@ -202,6 +211,15 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + [[package]] name = "crossbeam-deque" version = "0.8.6" @@ -227,6 +245,16 @@ version = "0.8.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" +[[package]] +name = "crypto-common" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "cucumber" version = "0.20.2" @@ -298,6 +326,16 @@ dependencies = [ "syn", ] +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "drain_filter_polyfill" version = "0.1.3" @@ -427,6 +465,16 @@ dependencies = [ "slab", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "gherkin" version = "0.14.0" @@ -677,6 +725,8 @@ dependencies = [ "semver", "serde", "serde_yml", + "sha2", + "thiserror", "tokio", ] @@ -986,6 +1036,17 @@ dependencies = [ "version_check", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "slab" version = "0.4.10" @@ -1146,6 +1207,12 @@ dependencies = [ "syn", ] +[[package]] +name = "typenum" +version = "1.18.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1dccffe3ce07af9386bfd29e80c0ab1a8205a2fc34e4bcd40364df902cfa8f3f" + [[package]] name = "unicode-ident" version = "1.0.18" diff --git a/Cargo.toml b/Cargo.toml index 4844fe69..a103fc4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,8 @@ serde = { version = "1", features = ["derive"] } serde_yml = "0.0.12" semver = { version = "1", features = ["serde"] } anyhow = "1" +thiserror = "1" +sha2 = "0.10" [lints.clippy] pedantic = { level = "warn", priority = -1 } diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index d283de23..169a0c84 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1056,6 +1056,12 @@ action identifier and carries the `phony` and `always` flags verbatim from the manifest. No Ninja specific placeholders are stored in the IR to keep the representation portable. +- Actions are deduplicated using a SHA-256 hash of their recipe and metadata. + Identical commands therefore share the same identifier which keeps the IR + deterministic for snapshot tests. +- Multiple rule references in a single target are not yet supported. The IR + generator reports `IrGenError::MultipleRules` when encountered. + ## Section 6: Process Management and Secure Execution The final stage of a Netsuke build involves executing commands. While Netsuke diff --git a/docs/roadmap.md b/docs/roadmap.md index fbda68f7..46bfcbb5 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -40,14 +40,15 @@ compilation pipeline from parsing to execution. - [x] Define the IR data structures (BuildGraph, Action, BuildEdge) in `src/ir.rs`, keeping it backend-agnostic as per the design. *(done)* - - [ ] Implement the ir::from_manifest transformation logic to convert the - AST into the BuildGraph IR. + - [x] Implement the ir::from_manifest transformation logic to convert the + AST into the BuildGraph IR. *(done)* - - [ ] During transformation, consolidate and deduplicate rules into ir::Action - structs based on a hash of their properties. + - [x] During transformation, consolidate and deduplicate rules into ir::Action + structs based on a hash of their properties. *(done)* - - [ ] Implement validation to ensure that every rule, command, or script + - [x] Implement validation to ensure that every rule, command, or script referenced by a target is valid and that they are mutually exclusive. + *(done)* - [ ] Implement a cycle detection algorithm (e.g., depth-first search) to fail compilation if a circular dependency is found in the target graph. diff --git a/src/ir.rs b/src/ir.rs index 9680fbd9..2491946c 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -24,7 +24,6 @@ //! graph.default_targets.push(PathBuf::from("hello")); //! ``` // -use crate::ast::Recipe; use std::collections::HashMap; use std::path::PathBuf; @@ -68,3 +67,158 @@ pub struct BuildEdge { /// Run the command on every invocation regardless of timestamps. pub always: bool, } + +use crate::ast::{NetsukeManifest, Recipe, StringOrList}; +use sha2::{Digest, Sha256}; +use thiserror::Error; + +/// Errors produced during IR generation. +#[derive(Debug, Error)] +pub enum IrGenError { + #[error("rule '{rule_name}' referenced by target '{target_name}' was not found")] + RuleNotFound { + target_name: String, + rule_name: String, + }, + + #[error("multiple rules per target are not supported for '{target_name}'")] + MultipleRules { target_name: String }, +} + +impl BuildGraph { + /// Transform a manifest into a [`BuildGraph`]. + /// + /// # Errors + /// + /// Returns [`IrGenError`] when a referenced rule is missing or multiple + /// rules are specified for a single target. + pub fn from_manifest(manifest: &NetsukeManifest) -> Result { + let mut graph = Self::default(); + let mut rule_map = HashMap::new(); + + for rule in &manifest.rules { + let action = Action { + recipe: rule.recipe.clone(), + description: rule.description.clone(), + depfile: None, + deps_format: None, + pool: None, + restat: false, + }; + let hash = hash_action(&action); + graph.actions.entry(hash.clone()).or_insert(action); + rule_map.insert(rule.name.clone(), hash); + } + + for target in manifest.actions.iter().chain(&manifest.targets) { + let outputs = to_paths(&target.name); + let action_id = match &target.recipe { + Recipe::Rule { rule } => { + let name = extract_single(rule).ok_or_else(|| IrGenError::MultipleRules { + target_name: outputs + .first() + .cloned() + .unwrap_or_default() + .display() + .to_string(), + })?; + rule_map + .get(name) + .cloned() + .ok_or_else(|| IrGenError::RuleNotFound { + target_name: outputs + .first() + .cloned() + .unwrap_or_default() + .display() + .to_string(), + rule_name: name.to_string(), + })? + } + Recipe::Command { .. } | Recipe::Script { .. } => { + let action = Action { + recipe: target.recipe.clone(), + description: None, + depfile: None, + deps_format: None, + pool: None, + restat: false, + }; + let hash = hash_action(&action); + graph.actions.entry(hash.clone()).or_insert(action); + hash + } + }; + + let edge = BuildEdge { + action_id: action_id.clone(), + inputs: to_paths(&target.sources), + explicit_outputs: outputs.clone(), + implicit_outputs: Vec::new(), + order_only_deps: to_paths(&target.order_only_deps), + phony: target.phony, + always: target.always, + }; + + for out in outputs { + graph.targets.insert(out, edge.clone()); + } + } + + for name in &manifest.defaults { + graph.default_targets.push(PathBuf::from(name)); + } + + Ok(graph) + } +} + +fn hash_action(action: &Action) -> String { + let mut hasher = Sha256::new(); + match &action.recipe { + Recipe::Command { command } => hasher.update(format!("cmd:{command}")), + Recipe::Script { script } => hasher.update(format!("scr:{script}")), + Recipe::Rule { rule } => { + hasher.update("rule:"); + match rule { + StringOrList::String(r) => hasher.update(r.as_bytes()), + StringOrList::List(list) => { + for r in list { + hasher.update(r.as_bytes()); + } + } + StringOrList::Empty => {} + } + } + } + if let Some(d) = &action.description { + hasher.update(d.as_bytes()); + } + if let Some(d) = &action.depfile { + hasher.update(d.as_bytes()); + } + if let Some(d) = &action.deps_format { + hasher.update(d.as_bytes()); + } + if let Some(p) = &action.pool { + hasher.update(p.as_bytes()); + } + hasher.update(if action.restat { b"1" } else { b"0" }); + format!("{:x}", hasher.finalize()) +} + +fn to_paths(sol: &StringOrList) -> Vec { + match sol { + StringOrList::Empty => Vec::new(), + StringOrList::String(s) => vec![PathBuf::from(s)], + StringOrList::List(v) => v.iter().map(PathBuf::from).collect(), + } +} + +fn extract_single(sol: &StringOrList) -> Option<&str> { + match sol { + StringOrList::String(s) => Some(s), + StringOrList::List(v) if v.len() == 1 => v.first().map(String::as_str), + _ => None, + } +} diff --git a/tests/data/duplicate_rules.yml b/tests/data/duplicate_rules.yml new file mode 100644 index 00000000..1bd3a642 --- /dev/null +++ b/tests/data/duplicate_rules.yml @@ -0,0 +1,21 @@ +netsuke_version: "1.0.0" +rules: + - name: compile1 + recipe: + kind: command + command: "cc -c $in -o $out" + - name: compile2 + recipe: + kind: command + command: "cc -c $in -o $out" +targets: + - name: hello.o + sources: hello.c + recipe: + kind: rule + rule: compile1 + - name: world.o + sources: world.c + recipe: + kind: rule + rule: compile2 diff --git a/tests/data/missing_rule.yml b/tests/data/missing_rule.yml new file mode 100644 index 00000000..11733130 --- /dev/null +++ b/tests/data/missing_rule.yml @@ -0,0 +1,7 @@ +netsuke_version: "1.0.0" +targets: + - name: hello.o + sources: hello.c + recipe: + kind: rule + rule: missing diff --git a/tests/features/ir.feature b/tests/features/ir.feature index a0295d02..9a5e453b 100644 --- a/tests/features/ir.feature +++ b/tests/features/ir.feature @@ -6,3 +6,17 @@ Feature: BuildGraph And the graph has 0 targets And the graph has 0 default targets + + Scenario: BuildGraph from manifest + When the manifest file "tests/data/rules.yml" is compiled to IR + Then the graph has 1 actions + And the graph has 1 targets + + Scenario: Duplicate rules are deduplicated + When the manifest file "tests/data/duplicate_rules.yml" is compiled to IR + Then the graph has 1 actions + And the graph has 2 targets + + Scenario: Rule not found during IR generation + When the manifest file "tests/data/missing_rule.yml" is compiled to IR + Then parsing the manifest fails diff --git a/tests/ir_from_manifest_tests.rs b/tests/ir_from_manifest_tests.rs new file mode 100644 index 00000000..e5f6ff62 --- /dev/null +++ b/tests/ir_from_manifest_tests.rs @@ -0,0 +1,30 @@ +//! Tests for generating `BuildGraph` from a manifest. + +use netsuke::{ + ir::{BuildGraph, IrGenError}, + manifest, +}; +use rstest::rstest; + +#[rstest] +fn minimal_manifest_to_ir() { + let manifest = manifest::from_path("tests/data/minimal.yml").expect("load"); + let graph = BuildGraph::from_manifest(&manifest).expect("ir"); + assert_eq!(graph.actions.len(), 1); + assert_eq!(graph.targets.len(), 1); +} + +#[rstest] +fn duplicate_rules_are_deduped() { + let manifest = manifest::from_path("tests/data/duplicate_rules.yml").expect("load"); + let graph = BuildGraph::from_manifest(&manifest).expect("ir"); + assert_eq!(graph.actions.len(), 1); + assert_eq!(graph.targets.len(), 2); +} + +#[test] +fn missing_rule_fails() { + let manifest = manifest::from_path("tests/data/missing_rule.yml").expect("load"); + let err = BuildGraph::from_manifest(&manifest).expect_err("error"); + matches!(err, IrGenError::RuleNotFound { .. }); +} diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index ce309b5d..bbb1d958 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -26,3 +26,23 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { let g = world.build_graph.as_ref().expect("graph"); assert_eq!(g.default_targets.len(), count); } + +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber requires owned String arguments" +)] +#[when(expr = "the manifest file {string} is compiled to IR")] +fn compile_manifest(world: &mut CliWorld, path: String) { + match netsuke::manifest::from_path(&path) + .and_then(|m| BuildGraph::from_manifest(&m).map_err(anyhow::Error::from)) + { + Ok(graph) => { + world.build_graph = Some(graph); + world.manifest_error = None; + } + Err(e) => { + world.build_graph = None; + world.manifest_error = Some(e.to_string()); + } + } +} From 6900414f6b6d9bd715b297bb12fffbc9767da79d Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 23:13:42 +0100 Subject: [PATCH 2/6] Refactor IR generation helpers --- src/ir.rs | 119 +++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 91 insertions(+), 28 deletions(-) diff --git a/src/ir.rs b/src/ir.rs index 2491946c..77e4b865 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -96,6 +96,18 @@ impl BuildGraph { let mut graph = Self::default(); let mut rule_map = HashMap::new(); + Self::process_rules(manifest, &mut graph.actions, &mut rule_map); + Self::process_targets(manifest, &mut graph.actions, &mut graph.targets, &rule_map)?; + Self::process_defaults(manifest, &mut graph.default_targets); + + Ok(graph) + } + + fn process_rules( + manifest: &NetsukeManifest, + actions: &mut HashMap, + rule_map: &mut HashMap, + ) { for rule in &manifest.rules { let action = Action { recipe: rule.recipe.clone(), @@ -106,10 +118,17 @@ impl BuildGraph { restat: false, }; let hash = hash_action(&action); - graph.actions.entry(hash.clone()).or_insert(action); + actions.entry(hash.clone()).or_insert(action); rule_map.insert(rule.name.clone(), hash); } + } + fn process_targets( + manifest: &NetsukeManifest, + actions: &mut HashMap, + targets: &mut HashMap, + rule_map: &HashMap, + ) -> Result<(), IrGenError> { for target in manifest.actions.iter().chain(&manifest.targets) { let outputs = to_paths(&target.name); let action_id = match &target.recipe { @@ -145,7 +164,7 @@ impl BuildGraph { restat: false, }; let hash = hash_action(&action); - graph.actions.entry(hash.clone()).or_insert(action); + actions.entry(hash.clone()).or_insert(action); hash } }; @@ -161,50 +180,61 @@ impl BuildGraph { }; for out in outputs { - graph.targets.insert(out, edge.clone()); + targets.insert(out, edge.clone()); } } + Ok(()) + } + fn process_defaults(manifest: &NetsukeManifest, defaults: &mut Vec) { for name in &manifest.defaults { - graph.default_targets.push(PathBuf::from(name)); + defaults.push(PathBuf::from(name)); } - - Ok(graph) } } fn hash_action(action: &Action) -> String { let mut hasher = Sha256::new(); - match &action.recipe { + hash_recipe(&mut hasher, &action.recipe); + hash_optional_fields(&mut hasher, action); + format!("{:x}", hasher.finalize()) +} + +fn hash_recipe(hasher: &mut Sha256, recipe: &Recipe) { + match recipe { Recipe::Command { command } => hasher.update(format!("cmd:{command}")), Recipe::Script { script } => hasher.update(format!("scr:{script}")), Recipe::Rule { rule } => { hasher.update("rule:"); - match rule { - StringOrList::String(r) => hasher.update(r.as_bytes()), - StringOrList::List(list) => { - for r in list { - hasher.update(r.as_bytes()); - } - } - StringOrList::Empty => {} - } + hash_rule_reference(hasher, rule); } } - if let Some(d) = &action.description { - hasher.update(d.as_bytes()); - } - if let Some(d) = &action.depfile { - hasher.update(d.as_bytes()); - } - if let Some(d) = &action.deps_format { - hasher.update(d.as_bytes()); +} + +fn hash_optional_fields(hasher: &mut Sha256, action: &Action) { + hash_optional_string(hasher, action.description.as_ref()); + hash_optional_string(hasher, action.depfile.as_ref()); + hash_optional_string(hasher, action.deps_format.as_ref()); + hash_optional_string(hasher, action.pool.as_ref()); + hasher.update(if action.restat { b"1" } else { b"0" }); +} + +fn hash_rule_reference(hasher: &mut Sha256, rule: &StringOrList) { + match rule { + StringOrList::String(r) => hasher.update(r.as_bytes()), + StringOrList::List(list) => { + for r in list { + hasher.update(r.as_bytes()); + } + } + StringOrList::Empty => {} } - if let Some(p) = &action.pool { - hasher.update(p.as_bytes()); +} + +fn hash_optional_string(hasher: &mut Sha256, value: Option<&String>) { + if let Some(v) = value { + hasher.update(v.as_bytes()); } - hasher.update(if action.restat { b"1" } else { b"0" }); - format!("{:x}", hasher.finalize()) } fn to_paths(sol: &StringOrList) -> Vec { @@ -222,3 +252,36 @@ fn extract_single(sol: &StringOrList) -> Option<&str> { _ => None, } } + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + #[rstest] + #[case( + Action { + recipe: Recipe::Command { command: "echo".into() }, + description: Some("desc".into()), + depfile: Some("$out.d".into()), + deps_format: Some("gcc".into()), + pool: None, + restat: false, + }, + "90e084dfc7f4df17f9a174ffbd97b0ab57c372aa25f0c42ec83ac573a6b77afc" + )] + #[case( + Action { + recipe: Recipe::Rule { rule: StringOrList::List(vec!["a".into(), "b".into()]) }, + description: None, + depfile: None, + deps_format: None, + pool: None, + restat: true, + }, + "c4d115e22054a982b41240841360b1563f035a6ad043b3ae0f0c3a5ed146333b" + )] + fn hash_action_is_stable(#[case] action: Action, #[case] expected: &str) { + assert_eq!(hash_action(&action), expected); + } +} From 14336ddd5df7b966fe1cb19541b6342aa074f2b0 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 23:13:48 +0100 Subject: [PATCH 3/6] Fix CodeScene upload step --- .github/workflows/ci.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b2520bf..92122762 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,10 +29,12 @@ jobs: - name: Run coverage run: cargo tarpaulin --out lcov - name: Upload coverage data to CodeScene - if: ${{ secrets.CS_ACCESS_TOKEN }} + env: + CS_ACCESS_TOKEN: ${{ secrets.CS_ACCESS_TOKEN }} + if: ${{ env.CS_ACCESS_TOKEN != '' }} uses: leynos/shared-actions/.github/actions/upload-codescene-coverage@v1.1.0 with: format: lcov - access-token: ${{ secrets.CS_ACCESS_TOKEN }} + access-token: ${{ env.CS_ACCESS_TOKEN }} installer-checksum: ${{ vars.CODESCENE_CLI_SHA256 }} From ee71298b4d84b6f36212976e669e2a8a73222334 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 23:23:25 +0100 Subject: [PATCH 4/6] Bump CodeScene upload action --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 92122762..77966b3d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: env: CS_ACCESS_TOKEN: ${{ secrets.CS_ACCESS_TOKEN }} if: ${{ env.CS_ACCESS_TOKEN != '' }} - uses: leynos/shared-actions/.github/actions/upload-codescene-coverage@v1.1.0 + uses: leynos/shared-actions/.github/actions/upload-codescene-coverage@v1.2.1 with: format: lcov access-token: ${{ env.CS_ACCESS_TOKEN }} From 889f0220db0384b047aafe30b6f39b6969ad7737 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 27 Jul 2025 01:18:21 +0100 Subject: [PATCH 5/6] Detect duplicate outputs and improve hashing --- docs/netsuke-design.md | 2 + src/ir.rs | 83 ++++++++++++++++++++------------ tests/data/duplicate_outputs.yml | 12 +++++ tests/features/ir.feature | 4 ++ tests/ir_from_manifest_tests.rs | 7 +++ 5 files changed, 78 insertions(+), 30 deletions(-) create mode 100644 tests/data/duplicate_outputs.yml diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 169a0c84..ea15e8a9 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1061,6 +1061,8 @@ representation portable. deterministic for snapshot tests. - Multiple rule references in a single target are not yet supported. The IR generator reports `IrGenError::MultipleRules` when encountered. +- Duplicate output files are rejected. Attempting to define the same output + path twice results in `IrGenError::DuplicateOutput`. ## Section 6: Process Management and Secure Execution diff --git a/src/ir.rs b/src/ir.rs index 77e4b865..3a1b4c27 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -83,6 +83,9 @@ pub enum IrGenError { #[error("multiple rules per target are not supported for '{target_name}'")] MultipleRules { target_name: String }, + + #[error("target output '{output}' defined multiple times")] + DuplicateOutput { output: String }, } impl BuildGraph { @@ -109,16 +112,7 @@ impl BuildGraph { rule_map: &mut HashMap, ) { for rule in &manifest.rules { - let action = Action { - recipe: rule.recipe.clone(), - description: rule.description.clone(), - depfile: None, - deps_format: None, - pool: None, - restat: false, - }; - let hash = hash_action(&action); - actions.entry(hash.clone()).or_insert(action); + let hash = register_action(actions, rule.recipe.clone(), rule.description.clone()); rule_map.insert(rule.name.clone(), hash); } } @@ -155,17 +149,7 @@ impl BuildGraph { })? } Recipe::Command { .. } | Recipe::Script { .. } => { - let action = Action { - recipe: target.recipe.clone(), - description: None, - depfile: None, - deps_format: None, - pool: None, - restat: false, - }; - let hash = hash_action(&action); - actions.entry(hash.clone()).or_insert(action); - hash + register_action(actions, target.recipe.clone(), None) } }; @@ -180,6 +164,11 @@ impl BuildGraph { }; for out in outputs { + if targets.contains_key(&out) { + return Err(IrGenError::DuplicateOutput { + output: out.display().to_string(), + }); + } targets.insert(out, edge.clone()); } } @@ -193,6 +182,24 @@ impl BuildGraph { } } +fn register_action( + actions: &mut HashMap, + recipe: Recipe, + description: Option, +) -> String { + let action = Action { + recipe, + description, + depfile: None, + deps_format: None, + pool: None, + restat: false, + }; + let hash = hash_action(&action); + actions.entry(hash.clone()).or_insert(action); + hash +} + fn hash_action(action: &Action) -> String { let mut hasher = Sha256::new(); hash_recipe(&mut hasher, &action.recipe); @@ -202,10 +209,16 @@ fn hash_action(action: &Action) -> String { fn hash_recipe(hasher: &mut Sha256, recipe: &Recipe) { match recipe { - Recipe::Command { command } => hasher.update(format!("cmd:{command}")), - Recipe::Script { script } => hasher.update(format!("scr:{script}")), + Recipe::Command { command } => { + hasher.update(b"cmd"); + update_with_len(hasher, command.as_bytes()); + } + Recipe::Script { script } => { + hasher.update(b"scr"); + update_with_len(hasher, script.as_bytes()); + } Recipe::Rule { rule } => { - hasher.update("rule:"); + hasher.update(b"rule"); hash_rule_reference(hasher, rule); } } @@ -221,10 +234,10 @@ fn hash_optional_fields(hasher: &mut Sha256, action: &Action) { fn hash_rule_reference(hasher: &mut Sha256, rule: &StringOrList) { match rule { - StringOrList::String(r) => hasher.update(r.as_bytes()), + StringOrList::String(r) => update_with_len(hasher, r.as_bytes()), StringOrList::List(list) => { for r in list { - hasher.update(r.as_bytes()); + update_with_len(hasher, r.as_bytes()); } } StringOrList::Empty => {} @@ -232,11 +245,21 @@ fn hash_rule_reference(hasher: &mut Sha256, rule: &StringOrList) { } fn hash_optional_string(hasher: &mut Sha256, value: Option<&String>) { - if let Some(v) = value { - hasher.update(v.as_bytes()); + match value { + Some(v) => { + hasher.update(b"1"); + update_with_len(hasher, v.as_bytes()); + } + None => hasher.update(b"0"), } } +fn update_with_len(hasher: &mut Sha256, bytes: &[u8]) { + let len = bytes.len(); + hasher.update(format!("{len}:").as_bytes()); + hasher.update(bytes); +} + fn to_paths(sol: &StringOrList) -> Vec { match sol { StringOrList::Empty => Vec::new(), @@ -268,7 +291,7 @@ mod tests { pool: None, restat: false, }, - "90e084dfc7f4df17f9a174ffbd97b0ab57c372aa25f0c42ec83ac573a6b77afc" + "a0f6e2cd3b9b3cee0bf94a7d53bce56cf4178dfe907bb1cb7c832f47846baf38" )] #[case( Action { @@ -279,7 +302,7 @@ mod tests { pool: None, restat: true, }, - "c4d115e22054a982b41240841360b1563f035a6ad043b3ae0f0c3a5ed146333b" + "cf8e97357820acf6f66037dcf977ee36c88c2811d60342db30c99507d24a0d60" )] fn hash_action_is_stable(#[case] action: Action, #[case] expected: &str) { assert_eq!(hash_action(&action), expected); diff --git a/tests/data/duplicate_outputs.yml b/tests/data/duplicate_outputs.yml new file mode 100644 index 00000000..02bbfc16 --- /dev/null +++ b/tests/data/duplicate_outputs.yml @@ -0,0 +1,12 @@ +netsuke_version: "1.0.0" +targets: + - name: hello.o + sources: hello.c + recipe: + kind: command + command: "cc -c $in -o $out" + - name: hello.o + sources: world.c + recipe: + kind: command + command: "cc -c $in -o $out" diff --git a/tests/features/ir.feature b/tests/features/ir.feature index 9a5e453b..20610b19 100644 --- a/tests/features/ir.feature +++ b/tests/features/ir.feature @@ -20,3 +20,7 @@ Feature: BuildGraph Scenario: Rule not found during IR generation When the manifest file "tests/data/missing_rule.yml" is compiled to IR Then parsing the manifest fails + + Scenario: Duplicate target outputs + When the manifest file "tests/data/duplicate_outputs.yml" is compiled to IR + Then parsing the manifest fails diff --git a/tests/ir_from_manifest_tests.rs b/tests/ir_from_manifest_tests.rs index e5f6ff62..8baca43f 100644 --- a/tests/ir_from_manifest_tests.rs +++ b/tests/ir_from_manifest_tests.rs @@ -28,3 +28,10 @@ fn missing_rule_fails() { let err = BuildGraph::from_manifest(&manifest).expect_err("error"); matches!(err, IrGenError::RuleNotFound { .. }); } + +#[test] +fn duplicate_outputs_fail() { + let manifest = manifest::from_path("tests/data/duplicate_outputs.yml").expect("load"); + let err = BuildGraph::from_manifest(&manifest).expect_err("error"); + matches!(err, IrGenError::DuplicateOutput { .. }); +} From ca270c842d2b1f449ae1170714ca191b6a743b73 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 27 Jul 2025 01:54:19 +0100 Subject: [PATCH 6/6] Refine IR generation and tests --- src/ir.rs | 28 +++++++++++++----------- tests/data/multiple_rules_per_target.yml | 18 +++++++++++++++ tests/features/ir.feature | 8 +++++-- tests/ir_from_manifest_tests.rs | 15 +++++++++---- tests/steps/ir_steps.rs | 8 +++++++ 5 files changed, 58 insertions(+), 19 deletions(-) create mode 100644 tests/data/multiple_rules_per_target.yml diff --git a/src/ir.rs b/src/ir.rs index 3a1b4c27..412115f8 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -128,23 +128,13 @@ impl BuildGraph { let action_id = match &target.recipe { Recipe::Rule { rule } => { let name = extract_single(rule).ok_or_else(|| IrGenError::MultipleRules { - target_name: outputs - .first() - .cloned() - .unwrap_or_default() - .display() - .to_string(), + target_name: get_target_display_name(&outputs), })?; rule_map .get(name) .cloned() .ok_or_else(|| IrGenError::RuleNotFound { - target_name: outputs - .first() - .cloned() - .unwrap_or_default() - .display() - .to_string(), + target_name: get_target_display_name(&outputs), rule_name: name.to_string(), })? } @@ -154,7 +144,7 @@ impl BuildGraph { }; let edge = BuildEdge { - action_id: action_id.clone(), + action_id, inputs: to_paths(&target.sources), explicit_outputs: outputs.clone(), implicit_outputs: Vec::new(), @@ -200,6 +190,11 @@ fn register_action( hash } +/// Computes a hash for an [`Action`]. +/// +/// Note: The hash depends on the order and formatting of the fields as they +/// are serialized. If stability across code or format changes is required, +/// consider using a canonical serialization format via `serde`. fn hash_action(action: &Action) -> String { let mut hasher = Sha256::new(); hash_recipe(&mut hasher, &action.recipe); @@ -276,6 +271,13 @@ fn extract_single(sol: &StringOrList) -> Option<&str> { } } +fn get_target_display_name(paths: &[PathBuf]) -> String { + paths + .first() + .map(|p| p.display().to_string()) + .unwrap_or_default() +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/data/multiple_rules_per_target.yml b/tests/data/multiple_rules_per_target.yml new file mode 100644 index 00000000..e18b665d --- /dev/null +++ b/tests/data/multiple_rules_per_target.yml @@ -0,0 +1,18 @@ +netsuke_version: "1.0.0" +rules: + - name: compile1 + recipe: + kind: command + command: "cc -c $in -o $out" + - name: compile2 + recipe: + kind: command + command: "cc -c $in -o $out" +targets: + - name: hello.o + sources: hello.c + recipe: + kind: rule + rule: + - compile1 + - compile2 diff --git a/tests/features/ir.feature b/tests/features/ir.feature index 20610b19..9ff8d7e3 100644 --- a/tests/features/ir.feature +++ b/tests/features/ir.feature @@ -19,8 +19,12 @@ Feature: BuildGraph Scenario: Rule not found during IR generation When the manifest file "tests/data/missing_rule.yml" is compiled to IR - Then parsing the manifest fails + Then IR generation fails + + Scenario: Multiple rules specified for target + When the manifest file "tests/data/multiple_rules_per_target.yml" is compiled to IR + Then IR generation fails Scenario: Duplicate target outputs When the manifest file "tests/data/duplicate_outputs.yml" is compiled to IR - Then parsing the manifest fails + Then IR generation fails diff --git a/tests/ir_from_manifest_tests.rs b/tests/ir_from_manifest_tests.rs index 8baca43f..14e668fd 100644 --- a/tests/ir_from_manifest_tests.rs +++ b/tests/ir_from_manifest_tests.rs @@ -22,16 +22,23 @@ fn duplicate_rules_are_deduped() { assert_eq!(graph.targets.len(), 2); } -#[test] +#[rstest] fn missing_rule_fails() { let manifest = manifest::from_path("tests/data/missing_rule.yml").expect("load"); let err = BuildGraph::from_manifest(&manifest).expect_err("error"); - matches!(err, IrGenError::RuleNotFound { .. }); + assert!(matches!(err, IrGenError::RuleNotFound { .. })); } -#[test] +#[rstest] fn duplicate_outputs_fail() { let manifest = manifest::from_path("tests/data/duplicate_outputs.yml").expect("load"); let err = BuildGraph::from_manifest(&manifest).expect_err("error"); - matches!(err, IrGenError::DuplicateOutput { .. }); + assert!(matches!(err, IrGenError::DuplicateOutput { .. })); +} + +#[rstest] +fn multiple_rules_per_target_fails() { + let manifest = manifest::from_path("tests/data/multiple_rules_per_target.yml").expect("load"); + let err = BuildGraph::from_manifest(&manifest).expect_err("error"); + assert!(matches!(err, IrGenError::MultipleRules { .. })); } diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index bbb1d958..53533182 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -46,3 +46,11 @@ fn compile_manifest(world: &mut CliWorld, path: String) { } } } + +#[then("IR generation fails")] +fn ir_generation_fails(world: &mut CliWorld) { + assert!( + world.manifest_error.is_some(), + "expected IR generation error" + ); +}