From 4fa29293c42891f7398f7974c2a588cdca9dc472 Mon Sep 17 00:00:00 2001 From: Klaus Aehlig Date: Fri, 24 Jan 2025 12:12:00 +0100 Subject: "generic" rule: verify staging conflicts on inputs The "generic" rules deliberately resolves conflicts on identical paths in a latest-wins fashion (seeing all artifacts as later than all runfiles) to allow an easy way to define actions. However, the inputs stage obtained by this resolution can still contain conflicts and those are an error. Properly detect those. Also clarify in the documentation, that only conflicts on identical paths are resolved in the described priority, not semantic overlap. (cherry-picked from 5e104a526cf76fe75312d2fd288a3c88f506fb0a) --- CHANGELOG.md | 4 ++++ doc/concepts/built-in-rules.md | 10 ++++++---- src/buildtool/build_engine/target_map/built_in_rules.cpp | 9 +++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c3e776e..b7a15b8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ Bug fixes on top of `1.4.2`. +### Fixes + +- The "generic" rule now properly detects staging conflicts. + ## Release `1.4.2` (2025-01-13) Bug fixes on top of `1.4.1`. diff --git a/doc/concepts/built-in-rules.md b/doc/concepts/built-in-rules.md index 6c80ffb4..e7a5e2e3 100644 --- a/doc/concepts/built-in-rules.md +++ b/doc/concepts/built-in-rules.md @@ -86,10 +86,12 @@ choice. The `"deps"` argument is evaluated and has to evaluate to a list of target names. The runfiles and artifacts of these targets form the -inputs of the action. Conflicts are not an error and resolved by giving -precedence to the artifacts over the runfiles; conflicts within -artifacts or runfiles are resolved in a latest-wins fashion using the -order of the targets in the evaluated `"deps"` argument. +inputs of the action. Conflicting definitions for individual paths +are not an error and resolved by giving precedence to the artifacts +over the runfiles; conflicts within artifacts or runfiles are +resolved in a latest-wins fashion using the order of the targets in +the evaluated `"deps"` argument. However, the input stage obtained +by those resolution rules has to be free of semantic conflicts. The fields `"cmds"`, `"cwd"`, `"sh -c"`, `"out_dirs"`, `"outs"`, and `"env"` are evaluated fields where `"cmds"`, `"out_dirs"`, and `"outs"` diff --git a/src/buildtool/build_engine/target_map/built_in_rules.cpp b/src/buildtool/build_engine/target_map/built_in_rules.cpp index 9623f816..a227c30b 100644 --- a/src/buildtool/build_engine/target_map/built_in_rules.cpp +++ b/src/buildtool/build_engine/target_map/built_in_rules.cpp @@ -1351,9 +1351,18 @@ void GenericRuleWithDeps( for (auto const& dep : dependency_values) { inputs = ExpressionPtr{Expression::map_t{inputs, (*dep)->RunFiles()}}; } + auto inputs_conflict = BuildMaps::Target::Utils::tree_conflict(inputs); for (auto const& dep : dependency_values) { inputs = ExpressionPtr{Expression::map_t{inputs, (*dep)->Artifacts()}}; } + // While syntactical conflicts are resolved in a latest wins (with artifacts + // after runfiles), semantic path conclicts are an error. + if (inputs_conflict) { + (*logger)(fmt::format("Input artifacts have staging conflict on {}", + nlohmann::json(*inputs_conflict).dump()), + /*fatal=*/true); + return; + } std::vector trees{}; inputs = BuildMaps::Target::Utils::add_dir_for( cwd_value->String(), inputs, &trees); -- cgit v1.2.3