diff --git a/include/pup/core/hash.hpp b/include/pup/core/hash.hpp index 03a21068..80f5ab5f 100644 --- a/include/pup/core/hash.hpp +++ b/include/pup/core/hash.hpp @@ -64,6 +64,10 @@ auto hex_to_hash(std::string_view hex) -> Result; [[nodiscard]] auto hash_equal(Hash256 const& a, Hash256 const& b) -> bool; +/// Byte-lexicographic order, for sorting and binary-searching hash-keyed ranges. +[[nodiscard]] +auto hash_less(Hash256 const& a, Hash256 const& b) -> bool; + /// Zero hash constant inline constexpr auto ZERO_HASH = Hash256 {}; diff --git a/include/pup/graph/dag.hpp b/include/pup/graph/dag.hpp index 7042f4d3..480e6c33 100644 --- a/include/pup/graph/dag.hpp +++ b/include/pup/graph/dag.hpp @@ -390,15 +390,23 @@ auto expand_instruction( [[nodiscard]] auto expand_instruction(Graph const& graph, NodeId cmd_id) -> StringId; -/// Compute a command's structural identity: a content hash that determines whether -/// the command must re-run. It folds the fully-expanded command text together with -/// the values (content hashes) of every Variable node the command depends on via a -/// Sticky edge — capturing config/env vars that affect the output without appearing -/// in the rendered text (e.g. an exported env var the subprocess reads via $VAR). -/// Two builds yield the same identity iff the command's effective definition is the -/// same, so it is the correct cross-build join key (unlike the rendered string). -[[nodiscard]] -auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256; +/// What the command will do, for deciding whether it must re-run: the fully-expanded +/// command text plus the values (content hashes) of every Variable node it depends on +/// via a Sticky edge — capturing config/env vars that affect the output without +/// appearing in the rendered text (e.g. an exported env var read as $VAR). +/// +/// Not a cross-build key. Editing a recipe changes the signature and must not make the +/// rule a different rule; that is what compute_command_key answers. +[[nodiscard]] +auto compute_command_signature(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256; + +/// Which rule this is, for joining a command against the previous build. A command that +/// produces files is keyed by the files themselves — output ownership is unique among +/// guard-satisfied commands, enforced when the output edge is created — so this textual +/// key is the fallback for output-less commands, which have nothing else to be named by. +/// It deliberately excludes variable values: a var change means re-run, not a new rule. +[[nodiscard]] +auto compute_command_key(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256; /// Set the build root name (relative path from source root to build root) /// For in-tree builds, this should be empty. For variant builds, e.g. "build". diff --git a/include/pup/index/entry.hpp b/include/pup/index/entry.hpp index e7b3d720..806a588b 100644 --- a/include/pup/index/entry.hpp +++ b/include/pup/index/entry.hpp @@ -54,7 +54,8 @@ struct CommandEntry { StringId display = StringId::Empty; ///< Display text (from ^ ^ markers) StringId env = StringId::Empty; ///< Environment variables - Hash256 identity = {}; ///< Structural identity: command text + values of vars it depends on + Hash256 key = {}; ///< Which rule this is: the cross-build join key + Hash256 signature = {}; ///< What it will do: changes mean re-run, not a different rule Vec inputs = {}; ///< Input file operands (for %f expansion) Vec outputs = {}; ///< Output file operands (for %o expansion) diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index c05bd157..cea9614b 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -50,7 +50,15 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// both the membership and the order of %f changed for any pattern a /// generated file matches (issues #177, #178); v16 identities no longer /// join for those commands. -inline constexpr auto INDEX_VERSION = std::uint32_t { 17 }; +/// 18 - A dep-scan command's identity folds its parent command's identity. Two +/// compiles of one source with equal flags render byte-identical scans, so +/// v17 gave them one identity and the join between them was arbitrary +/// (issue #170); every dep-scan command's identity changes. +/// 19 - The single `identity` splits into `key` (which rule this is) and +/// `signature` (what it will do), because one value cannot both stay stable +/// across a recipe edit and change when the recipe changes (issue #188). +/// v18 entries carry neither field. +inline constexpr auto INDEX_VERSION = std::uint32_t { 19 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { @@ -98,11 +106,11 @@ struct alignas(8) RawCommandEntry { std::uint32_t cmd_offset = 0; ///< Offset to template string with %f/%o patterns (v8) std::uint32_t display_offset = 0; ///< Display text offset (length-prefixed) std::uint32_t env_offset = 0; ///< Environment variables offset (length-prefixed) - Hash256 identity = {}; ///< Structural identity (v11): SHA-256 over command text - ///< plus the values of vars it depends on (sticky/exported) + Hash256 key = {}; ///< Which rule this is, for the cross-build join (v19) + Hash256 signature = {}; ///< What it will do, for deciding whether to re-run (v19) }; -static_assert(sizeof(RawCommandEntry) == 48, "RawCommandEntry must be 48 bytes"); +static_assert(sizeof(RawCommandEntry) == 80, "RawCommandEntry must be 80 bytes"); /// Raw edge entry (16 bytes) /// Represents dependencies between nodes diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index e1384ef1..0836e29f 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -734,7 +734,8 @@ auto serialize_command_nodes( .instruction_pattern = pup::graph::get(g, id), .display = pup::graph::get(g, id), .env = pup::StringId::Empty, - .identity = pup::graph::compute_command_identity(g, id, state.path_cache), + .key = pup::graph::compute_command_key(g, id, state.path_cache), + .signature = pup::graph::compute_command_signature(g, id, state.path_cache), .inputs = std::move(inputs), .outputs = std::move(outputs), }; @@ -831,6 +832,136 @@ auto process_implicit_deps( } } +/// How a set of commands is addressed for joining, in one place so that every join -- +/// graph to index, index to graph, index to index -- agrees on what "the same command" +/// means. A command that produces files is addressed by the files: output ownership is +/// unique among guard-satisfied commands, enforced where the output edge is created, so a +/// produced path names its producer and survives any edit to the recipe. A command that +/// produces nothing has no such name and is addressed by its textual key instead. +/// +/// The two are exclusive. A command that produced files is never looked up by key, or the +/// two directions of a join would disagree about whether two commands correspond. +struct CommandLookup { + Vec> by_output; ///< produced path -> producer + Vec> by_key; +}; + +/// What one command answers to. Empty outputs is what selects the textual key. +struct CommandAddress { + Vec outputs; + pup::Hash256 key = {}; +}; + +auto sort_lookup(CommandLookup& lookup) -> void +{ + std::sort(lookup.by_output.begin(), lookup.by_output.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); }); + std::sort(lookup.by_key.begin(), lookup.by_key.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); +} + +auto graph_command_address(pup::graph::BuildGraph const& state, pup::NodeId id) -> CommandAddress +{ + auto address = CommandAddress {}; + for (auto output_id : pup::graph::get_outputs(state.graph, id)) { + auto path_sv = pup::graph::get_full_path(state.graph, output_id, state.path_cache); + if (!path_sv.empty()) { + address.outputs.push_back(pup::global_pool().intern(path_sv)); + } + } + if (address.outputs.empty()) { + address.key = pup::graph::compute_command_key(state.graph, id, state.path_cache); + } + return address; +} + +auto index_command_address(pup::index::Index const& idx, pup::index::CommandEntry const& cmd) -> CommandAddress +{ + auto address = CommandAddress { .outputs = {}, .key = cmd.key }; + for (auto const* edge : idx.edges_from(cmd.id)) { + auto const* file = idx.find_file_by_id(edge->to); + if (file && file->type == pup::NodeType::Generated && !pup::is_empty(file->path)) { + address.outputs.push_back(file->path); + } + } + return address; +} + +/// Guard-satisfied only, matching the set the index records. +auto graph_command_lookup(pup::graph::BuildGraph const& state) -> CommandLookup +{ + auto lookup = CommandLookup {}; + for (auto id : pup::graph::all_nodes(state.graph)) { + if (!pup::node_id::is_command(id) || !pup::graph::is_guard_satisfied(state.graph, id)) { + continue; + } + auto address = graph_command_address(state, id); + for (auto path : address.outputs) { + lookup.by_output.emplace_back(path, id); + } + if (address.outputs.empty()) { + lookup.by_key.emplace_back(address.key, id); + } + } + sort_lookup(lookup); + return lookup; +} + +auto index_command_lookup(pup::index::Index const& idx) -> CommandLookup +{ + auto lookup = CommandLookup {}; + for (auto const& cmd : idx.commands()) { + auto address = index_command_address(idx, cmd); + for (auto path : address.outputs) { + lookup.by_output.emplace_back(path, cmd.id); + } + if (address.outputs.empty()) { + lookup.by_key.emplace_back(address.key, cmd.id); + } + } + sort_lookup(lookup); + return lookup; +} + +auto find_joined(CommandLookup const& lookup, CommandAddress const& address) -> std::optional +{ + for (auto path : address.outputs) { + auto const* it = std::lower_bound( + lookup.by_output.begin(), lookup.by_output.end(), path, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + if (it != lookup.by_output.end() && it->first == path) { + return it->second; + } + } + if (!address.outputs.empty()) { + return std::nullopt; + } + + auto const* it = std::lower_bound( + lookup.by_key.begin(), lookup.by_key.end(), address.key, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } + ); + if (it != lookup.by_key.end() && pup::hash_equal(it->first, address.key)) { + return it->second; + } + return std::nullopt; +} + +/// Whether any command in the lookup still produces this path. +auto has_live_producer(CommandLookup const& lookup, StringId path) -> bool +{ + auto const* it = std::lower_bound( + lookup.by_output.begin(), lookup.by_output.end(), path, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + return it != lookup.by_output.end() && it->first == path; +} + +auto find_joined_command( + CommandLookup const& lookup, + pup::index::Index const& idx, + pup::index::CommandEntry const& cmd +) -> std::optional +{ + return find_joined(lookup, index_command_address(idx, cmd)); +} + /// Preserve implicit edges from the old index for commands that weren't rebuilt. auto preserve_old_implicit_edges( pup::index::Index const& old_index, @@ -846,37 +977,28 @@ auto preserve_old_implicit_edges( } } - // Map a command's structural identity -> its id in the new index. Command ids are - // positional and shift across builds (e.g. when an earlier-created command is - // removed), so the old edge's `to` id cannot be trusted to mean the same command. - // Identity is stable, so we re-resolve each carried edge's command through it. - auto hash_less = [](pup::Hash256 const& a, pup::Hash256 const& b) { - return std::memcmp(a.data(), b.data(), a.size()) < 0; - }; - auto identity_to_new_id = pup::Vec> {}; - identity_to_new_id.reserve(ctx.index.commands().size()); - for (auto const& cmd : ctx.index.commands()) { - identity_to_new_id.emplace_back(cmd.identity, cmd.id); - } - std::sort(identity_to_new_id.begin(), identity_to_new_id.end(), [&](auto const& a, auto const& b) { return hash_less(a.first, b.first); }); + // Command ids are positional and shift across builds (e.g. when an earlier-created + // command is removed), so the old edge's `to` id cannot be trusted to mean the same + // command; re-resolve each carried edge's command through the same join every other + // consumer uses. + auto const new_lookup = index_command_lookup(ctx.index); for (auto const& edge : old_index.edges()) { if (edge.type != pup::LinkType::Implicit) { continue; } - // Re-resolve the old command to its identity-matched counterpart in the new - // index. If it's gone or its definition changed (no identity match), drop the - // edge: the command either no longer exists or rebuilt and rediscovered its deps. + // If the command is gone, drop the edge. If it survived but re-ran, the branch + // below drops it too: it rediscovered its own deps. auto const* old_cmd = old_index.find_command_by_id(edge.to); if (!old_cmd) { continue; } - auto match = std::lower_bound(identity_to_new_id.begin(), identity_to_new_id.end(), old_cmd->identity, [&](auto const& p, pup::Hash256 const& key) { return hash_less(p.first, key); }); - if (match == identity_to_new_id.end() || match->first != old_cmd->identity) { + auto joined = find_joined(new_lookup, index_command_address(old_index, *old_cmd)); + if (!joined) { continue; } - auto new_to_id = match->second; + auto new_to_id = *joined; if (commands_with_new_deps.contains(new_to_id)) { continue; @@ -904,39 +1026,6 @@ auto preserve_old_implicit_edges( } } -auto hash_less(pup::Hash256 const& a, pup::Hash256 const& b) -> bool -{ - return std::memcmp(a.data(), b.data(), a.size()) < 0; -} - -/// Sorted (identity → graph NodeId) map: the cross-build join key for commands. -using IdentityMap = Vec>; - -auto build_identity_map(pup::graph::BuildGraph const& state) -> IdentityMap -{ - auto map = IdentityMap {}; - for (auto id : pup::graph::all_nodes(state.graph)) { - if (pup::node_id::is_command(id)) { - map.emplace_back( - pup::graph::compute_command_identity(state.graph, id, state.path_cache), id - ); - } - } - std::sort(map.begin(), map.end(), [](auto const& a, auto const& b) { return hash_less(a.first, b.first); }); - return map; -} - -auto find_by_identity(IdentityMap const& map, pup::Hash256 const& identity) -> std::optional -{ - auto const* it = std::lower_bound( - map.begin(), map.end(), identity, [](auto const& p, auto const& k) { return hash_less(p.first, k); } - ); - if (it == map.end() || std::memcmp(it->first.data(), identity.data(), identity.size()) != 0) { - return std::nullopt; - } - return it->second; -} - auto contains_id(pup::Vec const& v, pup::StringId id) -> bool { return !pup::is_empty(id) && std::find(v.begin(), v.end(), id) != v.end(); @@ -982,7 +1071,7 @@ auto is_dir_authoritative( /// (stat data preserved) and every edge except Implicit — OrderOnly among them, /// which carries removal routing for order-only inputs — remapping ids to the new /// index's dense sequences; implicit edges are re-attached afterwards by -/// preserve_old_implicit_edges through the identity match. +/// preserve_old_implicit_edges through the same join. auto merge_out_of_scope_commands( pup::index::Index const& old_index, pup::Vec const& parse_scopes, @@ -995,12 +1084,7 @@ auto merge_out_of_scope_commands( { auto& pool = pup::global_pool(); - auto new_identities = pup::Vec {}; - new_identities.reserve(ctx.index.commands().size()); - for (auto const& cmd : ctx.index.commands()) { - new_identities.push_back(cmd.identity); - } - std::sort(new_identities.begin(), new_identities.end(), hash_less); + auto new_lookup = index_command_lookup(ctx.index); // serialize_graph_nodes registers File/Generated/Directory paths but not // Ghosts; without this the merge would duplicate a ghost's entry by path. @@ -1096,7 +1180,7 @@ auto merge_out_of_scope_commands( if (is_dir_authoritative(old_index, cmd.dir_id, parse_scopes, excludes, parsed_dirs, available_dirs, pruned_dirs)) { continue; } - if (std::binary_search(new_identities.begin(), new_identities.end(), cmd.identity, hash_less)) { + if (find_joined(new_lookup, index_command_address(old_index, cmd))) { continue; } if (any_dep_changed(cmd)) { @@ -1136,7 +1220,8 @@ auto merge_out_of_scope_commands( .instruction_pattern = cmd.instruction_pattern, .display = cmd.display, .env = cmd.env, - .identity = cmd.identity, + .key = cmd.key, + .signature = cmd.signature, .inputs = std::move(new_inputs), .outputs = std::move(new_outputs), }); @@ -1178,7 +1263,7 @@ auto expand_implicit_deps( pup::Vec const& changed, pup::index::Index const& index, pup::graph::BuildGraph const& state, - IdentityMap const& identity_map + CommandLookup const& join ) -> pup::Vec { auto result = pup::Vec { changed }; @@ -1227,7 +1312,7 @@ auto expand_implicit_deps( continue; } - auto cmd_node_id = find_by_identity(identity_map, cmd->identity); + auto cmd_node_id = find_joined_command(join, index, *cmd); if (!cmd_node_id) { continue; } @@ -1340,9 +1425,13 @@ struct NewCommands { pup::Vec forced_cmds; }; -/// Detect new commands (in graph but not index). Their outputs join the changed-file -/// set; a command that contributes no output paths cannot be reached through that -/// currency, so it is returned for direct scheduling instead. +/// Commands that must run because of what they are rather than because a file changed: +/// no counterpart in the previous build, or a counterpart whose signature differs. Their +/// outputs join the changed-file set; a command that contributes no output paths cannot +/// be reached through that currency, so it is returned for direct scheduling instead. +/// +/// The join runs graph -> index, the mirror of find_joined_command: a produced path names +/// its producer, and output-less commands fall back to the textual key. auto detect_new_commands( pup::graph::BuildGraph const& state, pup::index::Index const& idx, @@ -1353,16 +1442,23 @@ auto detect_new_commands( auto const& g = state.graph; auto result = NewCommands {}; - // Identity-keyed membership: a command must (re)build when its structural identity - // is absent from the previous index. Identity folds command text together with the - // values of the vars it depends on, so a change invisible to the rendered string - // (e.g. an exported env var the subprocess reads via $VAR) still flips the identity. - auto old_identities = pup::Vec {}; - old_identities.reserve(idx.commands().size()); + auto old_by_output = pup::Vec> {}; + auto old_by_key = pup::Vec> {}; for (auto const& cmd : idx.commands()) { - old_identities.push_back(cmd.identity); + auto produced_any = false; + for (auto const* edge : idx.edges_from(cmd.id)) { + auto const* file = idx.find_file_by_id(edge->to); + if (file && file->type == pup::NodeType::Generated && !pup::is_empty(file->path)) { + old_by_output.emplace_back(file->path, &cmd); + produced_any = true; + } + } + if (!produced_any) { + old_by_key.emplace_back(cmd.key, &cmd); + } } - std::sort(old_identities.begin(), old_identities.end(), hash_less); + std::sort(old_by_output.begin(), old_by_output.end(), [](auto const& a, auto const& b) { return pup::handle_less(a.first, b.first); }); + std::sort(old_by_key.begin(), old_by_key.end(), [](auto const& a, auto const& b) { return pup::hash_less(a.first, b.first); }); for (auto id : pup::graph::all_nodes(g)) { if (!pup::node_id::is_command(id)) { @@ -1371,24 +1467,50 @@ auto detect_new_commands( if (!pup::graph::is_guard_satisfied(g, id)) { continue; } - auto identity = pup::graph::compute_command_identity(g, id, state.path_cache); - if (!std::binary_search(old_identities.begin(), old_identities.end(), identity, hash_less)) { - auto pushed_outputs = false; - for (auto output_id : pup::graph::get_outputs(g, id)) { - auto output_path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); - if (!output_path_sv.empty()) { - result.changed_outputs.push_back(pup::global_pool().intern(output_path_sv)); - pushed_outputs = true; - } + + auto output_paths = pup::Vec {}; + for (auto output_id : pup::graph::get_outputs(g, id)) { + auto output_path_sv = pup::graph::get_full_path(g, output_id, state.path_cache); + if (!output_path_sv.empty()) { + output_paths.push_back(pup::global_pool().intern(output_path_sv)); } - if (!pushed_outputs) { - result.forced_cmds.push_back(id); + } + + pup::index::CommandEntry const* previous = nullptr; + for (auto path_id : output_paths) { + auto const* it = std::lower_bound( + old_by_output.begin(), old_by_output.end(), path_id, [](auto const& p, StringId k) { return pup::handle_less(p.first, k); } + ); + if (it != old_by_output.end() && it->first == path_id) { + previous = it->second; + break; } - if (verbose) { - auto display_sv = pup::global_pool().get(pup::graph::get(g, id)); - vprint(variant_name, " New command: {}\n", display_sv); + } + if (!previous && output_paths.empty()) { + auto key = pup::graph::compute_command_key(g, id, state.path_cache); + auto const* it = std::lower_bound( + old_by_key.begin(), old_by_key.end(), key, [](auto const& p, auto const& k) { return pup::hash_less(p.first, k); } + ); + if (it != old_by_key.end() && pup::hash_equal(it->first, key)) { + previous = it->second; } } + + auto signature = pup::graph::compute_command_signature(g, id, state.path_cache); + if (previous && pup::hash_equal(previous->signature, signature)) { + continue; + } + + for (auto path_id : output_paths) { + result.changed_outputs.push_back(path_id); + } + if (output_paths.empty()) { + result.forced_cmds.push_back(id); + } + if (verbose) { + auto display_sv = pup::global_pool().get(pup::graph::get(g, id)); + vprint(variant_name, " {}: {}\n", previous ? "Changed command" : "New command", display_sv); + } } return result; } @@ -1407,7 +1529,7 @@ auto reconcile_input_set( pup::graph::BuildGraph const& state, pup::index::Index const& idx, pup::Vec const& changed, - IdentityMap const& identity_map, + CommandLookup const& join, std::string_view variant_name, bool verbose ) -> InputSetDelta @@ -1481,7 +1603,7 @@ auto reconcile_input_set( if (!cmd) { continue; } - if (auto cmd_node_id = find_by_identity(identity_map, cmd->identity)) { + if (auto cmd_node_id = find_joined_command(join, idx, *cmd)) { orphaned.push_back(*cmd_node_id); if (verbose) { vprint( @@ -1516,7 +1638,7 @@ auto reconcile_input_set( /// Remove stale outputs from removed commands and report them. auto remove_stale_outputs( pup::index::Index const& idx, - IdentityMap const& identity_map, + CommandLookup const& join, pup::Vec const& parse_scopes, pup::parser::IgnoreList const& excludes, pup::Vec const& parsed_dirs, @@ -1535,15 +1657,17 @@ auto remove_stale_outputs( continue; } - if (find_by_identity(identity_map, cmd.identity)) { - continue; - } - + // Staleness is per file, not per command: a rule that drops one of its outputs + // still joins through the ones it kept, so asking only "did this command survive" + // would leave the dropped file owned by nothing and never delete it. for (auto const* edge : idx.edges_from(cmd.id)) { auto const* file = idx.find_file_by_id(edge->to); if (!file || file->type != pup::NodeType::Generated) { continue; } + if (has_live_producer(join, file->path)) { + continue; + } // Paths now include build root (e.g., "build/program") auto file_path_sv = pup::global_pool().get(file->path); @@ -1561,7 +1685,7 @@ auto remove_stale_outputs( } } - if (verbose) { + if (verbose && !find_joined_command(join, idx, cmd)) { vprint(variant_name, " Removed command: {}\n", pup::global_pool().get(cmd.display)); } } @@ -1703,7 +1827,7 @@ auto build_single_variant( // Build the identity → NodeId map: the cross-build join key for commands. // Must happen after parsing (operands set) but before incremental logic. auto cmd_index_start = pup::SteadyClock::now(); - auto const identity_map = build_identity_map(bs); + auto const join = graph_command_lookup(bs); auto cmd_index_elapsed = pup::SteadyClock::now() - cmd_index_start; pup::thread_metrics().command_index_time = std::chrono::duration_cast(cmd_index_elapsed); @@ -1754,13 +1878,13 @@ auto build_single_variant( auto change_detect_elapsed = pup::SteadyClock::now() - change_detect_start; pup::thread_metrics().change_detection_time = std::chrono::duration_cast(change_detect_elapsed); - auto input_delta = reconcile_input_set(bs, idx, changed_files, identity_map, variant_name, opts.verbose); + auto input_delta = reconcile_input_set(bs, idx, changed_files, join, variant_name, opts.verbose); for (auto path_id : input_delta.changed_paths) { changed_files.push_back(path_id); } auto implicit_deps_start = pup::SteadyClock::now(); - changed_files = expand_implicit_deps(changed_files, idx, bs, identity_map); + changed_files = expand_implicit_deps(changed_files, idx, bs, join); auto implicit_deps_elapsed = pup::SteadyClock::now() - implicit_deps_start; pup::thread_metrics().implicit_deps_time = std::chrono::duration_cast(implicit_deps_elapsed); @@ -1794,7 +1918,7 @@ auto build_single_variant( auto stale_start = pup::SteadyClock::now(); remove_stale_outputs( idx, - identity_map, + join, parse_scopes, excludes, ctx.parsed_dirs(), diff --git a/src/cli/context.cpp b/src/cli/context.cpp index 79697b21..ab1b2b8a 100644 --- a/src/cli/context.cpp +++ b/src/cli/context.cpp @@ -948,7 +948,10 @@ auto build_context( } } - (void)graph::finalize_graph(ctx.impl_->graph, builder_state); + auto finalized = graph::finalize_graph(ctx.impl_->graph, builder_state); + if (!finalized) { + return unexpected(finalized.error()); + } for (auto warning_id : builder_state.warnings) { eprint("warning: {}\n", pool.get(warning_id)); diff --git a/src/core/hash.cpp b/src/core/hash.cpp index 52e34478..dec02ce8 100644 --- a/src/core/hash.cpp +++ b/src/core/hash.cpp @@ -213,4 +213,9 @@ auto hash_equal(Hash256 const& a, Hash256 const& b) -> bool return std::memcmp(a.data(), b.data(), a.size()) == 0; } +auto hash_less(Hash256 const& a, Hash256 const& b) -> bool +{ + return std::memcmp(a.data(), b.data(), a.size()) < 0; +} + } // namespace pup diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index c56d3960..bbdfca8f 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -2500,6 +2500,47 @@ auto add_tupfile( return {}; } +/// A command that produces files is joined across builds by the files, and output ownership +/// is already unique among guard-satisfied commands. Output-less commands are joined by +/// their textual key instead, and nothing enforced that key's uniqueness: two such rules in +/// one directory are indistinguishable to every later build. Reject the graph here rather +/// than join it wrong later. +/// +/// Guard-satisfied only, matching the set the index records: complementary branches of one +/// conditional legitimately render the same text, and at most one of them is ever live. +auto reject_ambiguous_keys(BuildGraph& build_state) -> Result +{ + auto& g = build_state.graph; + + auto keys = Vec> {}; + for (auto id : all_nodes(g)) { + if (!node_id::is_command(id) || !is_guard_satisfied(g, id) || !get_outputs(g, id).empty()) { + continue; + } + keys.emplace_back(compute_command_key(g, id, build_state.path_cache), id); + } + + std::sort(keys.begin(), keys.end(), [](auto const& a, auto const& b) { + return hash_less(a.first, b.first); + }); + auto const* dup = std::adjacent_find(keys.begin(), keys.end(), [](auto const& a, auto const& b) { + return hash_equal(a.first, b.first); + }); + if (dup == keys.end()) { + return {}; + } + + auto dir_sv = str(get(g, dup->second)); + auto err = Buf {}; + err.fmt( + "Duplicate command in '{}': two rules produce no output and render the same command " + "line, so no build can tell them apart:\n {}", + dir_sv.empty() ? "." : dir_sv, + str(expand_instruction(g, dup->second, build_state.path_cache)) + ); + return make_error(ErrorCode::DuplicateNode, err.view()); +} + auto finalize_graph( BuildGraph& build_state, Builder& state @@ -2628,7 +2669,14 @@ auto finalize_graph( } state.deferred_edges.clear(); - return {}; + + // The configure pass runs before tup.config exists, so rendered text does not yet + // distinguish anything and a key collision there means nothing; it schedules only the + // config-generating rules anyway. Last, because pass 2 rewrites command text. + if (!state.options.reject_empty_commands) { + return {}; + } + return reject_ambiguous_keys(build_state); } } // namespace pup::graph diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 7b9a85d7..dd6bbae6 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -1031,17 +1031,40 @@ auto expand_instruction(Graph const& graph, NodeId cmd_id) -> StringId return expand_instruction(graph, cmd_id, cache); } -auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256 +auto compute_command_key(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256 { auto& pool = global_pool(); auto state = sha256_init(); auto constexpr SEP = std::byte { 0 }; - // Base: the fully-expanded command text (instruction + operand paths + in-text vars). state = sha256_update(state, pool.get(expand_instruction(graph, cmd_id, cache))); // Command text is Tupfile-relative, so the same rule in sibling directories renders - // identically; without the directory those distinct rules share one identity. + // identically; without the directory those distinct rules share one key. + state = sha256_update(state, std::span { &SEP, 1 }); + state = sha256_update(state, pool.get(get(graph, cmd_id))); + + // A dep-scan command is output-less and drops its parent's -o, so two compiles of one + // source with equal flags render byte-identical scans; whose deps they inject is the + // only thing that tells them apart. One level: a parent is rule-authored, so has none. + if (auto parent = get_parent_command(graph, cmd_id); parent != INVALID_NODE_ID) { + auto parent_key = compute_command_key(graph, parent, cache); + state = sha256_update(state, std::span { &SEP, 1 }); + state = sha256_update(state, std::span { parent_key.data(), parent_key.size() }); + } + + return sha256_finalize(state); +} + +auto compute_command_signature(Graph const& graph, NodeId cmd_id, PathCache& cache) -> Hash256 +{ + auto& pool = global_pool(); + auto state = sha256_init(); + auto constexpr SEP = std::byte { 0 }; + + // Base: the fully-expanded command text (instruction + operand paths + in-text vars). + state = sha256_update(state, pool.get(expand_instruction(graph, cmd_id, cache))); + state = sha256_update(state, std::span { &SEP, 1 }); state = sha256_update(state, pool.get(get(graph, cmd_id))); diff --git a/src/index/entry.cpp b/src/index/entry.cpp index 559c09a8..77ec8396 100644 --- a/src/index/entry.cpp +++ b/src/index/entry.cpp @@ -66,7 +66,8 @@ auto CommandEntry::to_raw( raw.cmd_offset = instruction_offset; raw.display_offset = display_offset; raw.env_offset = env_offset; - raw.identity = identity; + raw.key = key; + raw.signature = signature; return raw; } @@ -86,7 +87,8 @@ auto CommandEntry::from_raw( .instruction_pattern = global_pool().intern(instruction_pattern), .display = global_pool().intern(display_str), .env = global_pool().intern(env_str), - .identity = raw.identity, + .key = raw.key, + .signature = raw.signature, .inputs = std::move(inputs), .outputs = std::move(outputs), }; diff --git a/test/e2e/fixtures/duplicate_command/Tupfile.fixture b/test/e2e/fixtures/duplicate_command/Tupfile.fixture new file mode 100644 index 00000000..0a8e649f --- /dev/null +++ b/test/e2e/fixtures/duplicate_command/Tupfile.fixture @@ -0,0 +1,2 @@ +: |> ./check |> +: |> ./check |> diff --git a/test/e2e/fixtures/duplicate_command/Tupfile.ini b/test/e2e/fixtures/duplicate_command/Tupfile.ini new file mode 100644 index 00000000..05e60154 --- /dev/null +++ b/test/e2e/fixtures/duplicate_command/Tupfile.ini @@ -0,0 +1 @@ +# Project root marker diff --git a/test/e2e/fixtures/duplicate_command/tup.config b/test/e2e/fixtures/duplicate_command/tup.config new file mode 100644 index 00000000..f4bd2fae --- /dev/null +++ b/test/e2e/fixtures/duplicate_command/tup.config @@ -0,0 +1 @@ +# Test config diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index 32628ed1..f3a68214 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -1892,3 +1892,103 @@ TEST_CASE("GraphBuilder variable assigned under a config branch keeps later cond CHECK(wrapped > baseline); } + +namespace { + +auto finalize_tupfile( + BuilderTestFixture const& fixture, + std::string_view source, + VarDb const* config_vars = nullptr, + bool reject_empty_commands = true +) -> pup::Result +{ + auto bs = make_build_graph(); + auto vars = VarDb {}; + auto ctx = EvalContext { .vars = &vars, .config_vars = config_vars }; + + auto options = BuilderOptions { + .source_root = intern(fixture.root_str()), + .config_root = intern(fixture.root_str()), + .output_root = pup::StringId::Empty, + .config_path = pup::StringId::Empty, + .expand_globs = false, + .validate_inputs = false, + .reject_empty_commands = reject_empty_commands, + }; + auto builder_state = make_builder(options); + + auto parse_result = parse_tupfile(source, fixture.tupfile_path("")); + REQUIRE(parse_result.success()); + REQUIRE(add_tupfile(bs, parse_result.tupfile, ctx, builder_state).has_value()); + return finalize_graph(bs, builder_state); +} + +} // namespace + +TEST_CASE("GraphBuilder rejects two output-less commands that share one key", "[builder][identity]") +{ + auto fixture = BuilderTestFixture {}; + + // No outputs to be named by, and identical text: no later build can tell them apart. + auto result = finalize_tupfile(fixture, ": |> ./check |>\n: |> ./check |>\n"); + + REQUIRE_FALSE(result.has_value()); + CHECK(sv(result.error().message).find("./check") != std::string_view::npos); +} + +TEST_CASE("GraphBuilder accepts identical text when the outputs differ", "[builder][identity]") +{ + auto fixture = BuilderTestFixture {}; + + // Same recipe, but each is named by what it produces, so the join stays unambiguous. + CHECK(finalize_tupfile(fixture, ": |> ./gen |> a.out\n: |> ./gen |> b.out\n").has_value()); +} + +TEST_CASE("GraphBuilder accepts commands whose rendered text differs", "[builder][identity]") +{ + auto fixture = BuilderTestFixture {}; + + CHECK(finalize_tupfile(fixture, ": |> ./gen a |> a.out\n: |> ./gen b |> b.out\n").has_value()); +} + +TEST_CASE("GraphBuilder accepts empty-rendered duplicates in the configure pass", "[builder][identity][configure]") +{ + auto fixture = BuilderTestFixture {}; + + // The configure pass evaluates before tup.config exists, so rendered text does not yet + // distinguish anything. Those commands are never scheduled -- only the config-generating + // rules are -- so no collision among them can matter. + auto result = finalize_tupfile( + fixture, ": |> @(CC) |> a.out\n: |> @(CC) |> b.out\n", nullptr, false + ); + + CHECK(result.has_value()); +} + +TEST_CASE("GraphBuilder accepts config-distinguished commands in the configure pass", "[builder][identity][configure]") +{ + auto fixture = BuilderTestFixture {}; + + // Distinct once tup.config is read; identical before it exists. Rejecting here would + // fail the very pass that creates the file that tells them apart. + auto result = finalize_tupfile( + fixture, ": |> ./run @(A) |>\n: |> ./run @(B) |>\n", nullptr, false + ); + + CHECK(result.has_value()); +} + +TEST_CASE("GraphBuilder accepts identical text under complementary guards", "[builder][identity][phi]") +{ + auto fixture = BuilderTestFixture {}; + auto config = VarDb {}; + config.set("M", "y"); + + auto result = finalize_tupfile( + fixture, + "ifeq (@(M),y)\n: |> ./gen |> out.txt\nelse\n: |> ./gen |> out.txt\nendif\n", + &config + ); + + CHECK(result.has_value()); +} diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 19230bb8..8f33a7ff 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4473,6 +4473,115 @@ SCENARIO("Duplicate output detection", "[e2e][duplicate]") } } +SCENARIO("Duplicate command detection", "[e2e][duplicate][identity]") +{ + GIVEN("a Tupfile with two output-less rules that render the same command line") + { + auto f = E2EFixture { "duplicate_command" }; + + WHEN("pup parses the project") + { + auto result = f.build(); + + THEN("build fails naming the ambiguous command") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("Duplicate command") != std::string::npos); + REQUIRE(result.stderr_output.find("./check") != std::string::npos); + } + } + } +} + +SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][identity][join]") +{ + GIVEN("a built project whose rule produces one output") + { + auto f = E2EFixture { "phi_same_output" }; + f.write_file("Tupfile", ": input.txt |> cp %f %o |> output.txt\n"); + f.write_file("input.txt", "test\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + + WHEN("only the command text changes, with the same inputs and outputs") + { + f.write_file("Tupfile", ": input.txt |> cat %f > %o |> output.txt\n"); + auto result = f.build({ "-B", "build", "-v" }); + + THEN("the rule is recognised as the same one and its output is not deleted") + { + INFO("stdout: " << result.stdout_output); + REQUIRE(result.success()); + REQUIRE(result.stdout_output.find("Removed stale") == std::string::npos); + REQUIRE(f.exists("build/output.txt")); + } + } + } +} + +SCENARIO("Dropping one output of a rule removes that output", "[e2e][identity][join][stale]") +{ + GIVEN("a built rule that declares two outputs") + { + auto f = E2EFixture { "phi_same_output" }; + f.write_file("Tupfile", ": input.txt |> sh -c \"cp input.txt build/a.out && cp input.txt build/b.out\" |> a.out b.out\n"); + f.write_file("input.txt", "test\n"); + f.mkdir("build"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + REQUIRE(f.build({ "-B", "build" }).success()); + REQUIRE(f.exists("build/a.out")); + REQUIRE(f.exists("build/b.out")); + + WHEN("the rule is edited to declare only the first output") + { + f.write_file("Tupfile", ": input.txt |> sh -c \"cp input.txt build/a.out\" |> a.out\n"); + auto result = f.build({ "-B", "build", "-v" }); + + THEN("the output it no longer produces is deleted, and the one it still does survives") + { + INFO("stdout: " << result.stdout_output); + REQUIRE(result.success()); + REQUIRE(f.exists("build/a.out")); + REQUIRE_FALSE(f.exists("build/b.out")); + } + } + } +} + +SCENARIO("Complementary branches may render the same command line", "[e2e][duplicate][identity][phi]") +{ + GIVEN("a project whose two conditional branches carry identical rule text") + { + auto f = E2EFixture { "phi_same_output" }; + f.write_file("Tupfile", R"( +ifeq (@(MODE),release) +: input.txt |> cp %f %o |> output.txt +else +: input.txt |> cp %f %o |> output.txt +endif +)"); + f.write_file("input.txt", "test\n"); + f.mkdir("build"); + f.write_file("build/tup.config", "CONFIG_MODE=debug\n"); + REQUIRE(f.pup({ "configure", "-B", "build" }).success()); + + WHEN("built") + { + auto result = f.build({ "-B", "build" }); + + THEN("only the guard-satisfied branch counts, so the build succeeds") + { + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.exists("build/output.txt")); + } + } + } +} + // ============================================================================= // Platform Conditional Tests // ============================================================================= diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index bfa3c129..c2b6dc07 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1637,7 +1637,7 @@ TEST_CASE("node_id encoding: 30-bit index roundtrips and kinds are mutually excl REQUIRE_FALSE(pup::node_id::is_file(pup::INVALID_NODE_ID)); } -TEST_CASE("compute_command_identity separates rules by directory", "[graph][identity]") +TEST_CASE("compute_command_key separates rules by directory", "[graph][identity]") { auto bs = make_build_graph(); auto& g = bs.graph; @@ -1655,26 +1655,72 @@ TEST_CASE("compute_command_identity separates rules by directory", "[graph][iden REQUIRE(in_a.has_value()); REQUIRE(in_b.has_value()); - SECTION("same text in different directories yields different identities") + SECTION("same text in different directories yields different keys") { - CHECK(compute_command_identity(g, *in_a, bs.path_cache) - != compute_command_identity(g, *in_b, bs.path_cache)); + CHECK(compute_command_key(g, *in_a, bs.path_cache) + != compute_command_key(g, *in_b, bs.path_cache)); } - SECTION("identity is stable for the same command") + SECTION("the key is stable for the same command") { - CHECK(compute_command_identity(g, *in_a, bs.path_cache) - == compute_command_identity(g, *in_a, bs.path_cache)); + CHECK(compute_command_key(g, *in_a, bs.path_cache) + == compute_command_key(g, *in_a, bs.path_cache)); } - SECTION("different text in the same directory yields different identities") + SECTION("different text in the same directory yields different keys") { auto other = add_command_node(g, CommandNode { .source_dir = intern("a"), .instruction_id = intern("cat other.txt > out.txt"), }); REQUIRE(other.has_value()); - CHECK(compute_command_identity(g, *in_a, bs.path_cache) - != compute_command_identity(g, *other, bs.path_cache)); + CHECK(compute_command_key(g, *in_a, bs.path_cache) + != compute_command_key(g, *other, bs.path_cache)); + } +} + +TEST_CASE("compute_command_key separates dep-scan commands by parent", "[graph][identity]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + // A dep-scan command drops its parent's -o, so two compiles of one source with + // equal flags render byte-identical scans; only the parent tells them apart. + auto compile_a = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -c ggc.cc -o one.o"), + }); + auto compile_b = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -c ggc.cc -o two.o"), + }); + REQUIRE(compile_a.has_value()); + REQUIRE(compile_b.has_value()); + + auto scan_a = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -M ggc.cc"), + .output_action = OutputAction::InjectImplicitDeps, + .parent_command = *compile_a, + }); + auto scan_b = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("g++ -M ggc.cc"), + .output_action = OutputAction::InjectImplicitDeps, + .parent_command = *compile_b, + }); + REQUIRE(scan_a.has_value()); + REQUIRE(scan_b.has_value()); + + SECTION("identical scan text under different parents yields different keys") + { + CHECK(compute_command_key(g, *scan_a, bs.path_cache) + != compute_command_key(g, *scan_b, bs.path_cache)); + } + + SECTION("a scan's key is stable") + { + CHECK(compute_command_key(g, *scan_a, bs.path_cache) + == compute_command_key(g, *scan_a, bs.path_cache)); } } diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index 5356c8e4..b9861250 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -80,9 +80,9 @@ TEST_CASE("Index format struct sizes", "[index]") REQUIRE(sizeof(RawFileEntry) == 64); } - SECTION("RawCommandEntry is 48 bytes (v11: + identity hash)") + SECTION("RawCommandEntry is 80 bytes (v19: + key and signature hashes)") { - REQUIRE(sizeof(RawCommandEntry) == 48); + REQUIRE(sizeof(RawCommandEntry) == 80); } SECTION("RawEdge is 16 bytes") @@ -162,9 +162,13 @@ TEST_CASE("FileEntry conversion", "[index]") TEST_CASE("CommandEntry conversion", "[index]") { - auto identity = pup::Hash256 {}; - identity[0] = std::byte { 0xAB }; - identity[31] = std::byte { 0xCD }; + // Distinct values: a roundtrip that swapped the two fields must fail. + auto key = pup::Hash256 {}; + key[0] = std::byte { 0xAB }; + key[31] = std::byte { 0xCD }; + auto signature = pup::Hash256 {}; + signature[0] = std::byte { 0x12 }; + signature[31] = std::byte { 0x34 }; auto cmd = CommandEntry { .id = node_id::make_command(5), @@ -172,7 +176,8 @@ TEST_CASE("CommandEntry conversion", "[index]") .instruction_pattern = intern("gcc -c %f -o %o"), .display = intern("CC main.c"), .env = intern("CC=gcc"), - .identity = identity, + .key = key, + .signature = signature, .inputs = { 10 }, .outputs = { 20 }, }; @@ -183,7 +188,8 @@ TEST_CASE("CommandEntry conversion", "[index]") REQUIRE(raw.cmd_offset == 0); REQUIRE(raw.display_offset == 50); REQUIRE(raw.env_offset == 100); - REQUIRE(raw.identity == identity); + REQUIRE(raw.key == key); + REQUIRE(raw.signature == signature); // ID is computed from array index (4 + 1 = 5, then node_id::make_command) auto& pool = global_pool(); @@ -197,7 +203,8 @@ TEST_CASE("CommandEntry conversion", "[index]") REQUIRE(restored.instruction_pattern == cmd.instruction_pattern); REQUIRE(restored.display == cmd.display); REQUIRE(restored.env == cmd.env); - REQUIRE(restored.identity == identity); + REQUIRE(restored.key == key); + REQUIRE(restored.signature == signature); REQUIRE(restored.inputs == cmd.inputs); REQUIRE(restored.outputs == cmd.outputs); } @@ -440,17 +447,17 @@ TEST_CASE("Index serialization roundtrip", "[e2e][index]") .size = 8192, }); - // Command 1 (v8: template + operands; v11: + identity hash) - auto cmd_identity = pup::Hash256 {}; - cmd_identity[0] = std::byte { 0x11 }; - cmd_identity[31] = std::byte { 0x99 }; + // Command 1 (v8: template + operands; v19: + key and signature hashes) + auto cmd_key = pup::Hash256 {}; + cmd_key[0] = std::byte { 0x11 }; + cmd_key[31] = std::byte { 0x99 }; index.add_command(CommandEntry { .id = cmd_id, .dir_id = 0, .instruction_pattern = intern("g++ -c %f -o %o"), .display = intern("CXX main.cpp"), .env = {}, - .identity = cmd_identity, + .key = cmd_key, .inputs = { 3 }, // main.cpp .outputs = { 4 }, // main.o }); @@ -518,7 +525,7 @@ TEST_CASE("Index serialization roundtrip", "[e2e][index]") REQUIRE(cmd != nullptr); REQUIRE(cmd->instruction_pattern == intern("g++ -c %f -o %o")); REQUIRE(cmd->display == intern("CXX main.cpp")); - REQUIRE(cmd->identity == cmd_identity); + REQUIRE(cmd->key == cmd_key); REQUIRE(cmd->inputs == pup::Vec { 3 }); REQUIRE(cmd->outputs == pup::Vec { 4 });