From ba7fcc5c78dbc73a330185e266e32b93fccf754a Mon Sep 17 00:00:00 2001 From: Moritz Date: Sun, 21 Sep 2025 13:30:16 +0200 Subject: [PATCH 1/3] This commit will fix issue 2653. This commit will reflect that dynamic dependencies may change the graph during recomputeDirty of nodes. A newly added dyndep output may be dirty and can cause other targets to be dirty as well. This shall take effect if those nodes has been visited before. A new algorithm to iterate over the graph to make nodes dirty has been added. --- src/graph.cc | 44 +++++++++++++++++++++++++++++++++++++++++++- src/graph.h | 25 ++++++++++++++++++++----- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/src/graph.cc b/src/graph.cc index b6cc1f4922..b18044cb88 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -79,6 +79,37 @@ bool DependencyScan::RecomputeDirty(Node* initial_node, return true; } +namespace { +class propagateDirty { + public: + propagateDirty(std::vector* topLevelNodes) + : topLevelNodes_(topLevelNodes) {} + + void process(Edge* edge); + + private: + std::vector* topLevelNodes_; +}; + +void propagateDirty::process(Edge* const edge) { + for (Node* const out : edge->outputs_) { + if (out->clean()) { + out->MarkDirty(); + edge->outputs_ready_ = false; + if (out->out_edges().empty()) { + // add target to top level + topLevelNodes_->push_back(out); + continue; + } + for (Edge* outEdge : out->out_edges()) { + if (outEdge) + process(outEdge); + } + } + } +} +} // namespace + bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, std::vector* validation_nodes, string* err) { @@ -112,6 +143,7 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, bool dirty = false; edge->outputs_ready_ = true; edge->deps_missing_ = false; + bool dyndepLoaded = false; if (!edge->deps_loaded_) { // This is our first encounter with this edge. @@ -134,6 +166,8 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, // The dyndep file is ready, so load it now. if (!LoadDyndeps(edge->dyndep_, err)) return false; + else + dyndepLoaded = true; } } } @@ -202,8 +236,16 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, // Finally, visit each output and update their dirty state if necessary. for (vector::iterator o = edge->outputs_.begin(); o != edge->outputs_.end(); ++o) { - if (dirty) + if (dirty) { (*o)->MarkDirty(); + if (dyndepLoaded) { + propagateDirty propagateDirtyDyndep(validation_nodes); + for (Edge* outedges : (*o)->out_edges()) + propagateDirtyDyndep.process(outedges); + } + } else { + (*o)->MarkClean(); + } } // If an edge is dirty, its outputs are normally not ready. (It's diff --git a/src/graph.h b/src/graph.h index d98f1f9440..0b47abc368 100644 --- a/src/graph.h +++ b/src/graph.h @@ -37,6 +37,12 @@ struct Node; struct Pool; struct State; +enum class DirtyState { + SPARE = 0, + DIRTY = 1, + CLEAN = 2, +}; + /// Information about a node in the dependency graph: the file, whether /// it's dirty, mtime, etc. struct Node { @@ -60,7 +66,7 @@ struct Node { void ResetState() { mtime_ = -1; exists_ = ExistenceStatusUnknown; - dirty_ = false; + dirty_ = DirtyState::SPARE; } /// Mark the Node as already-stat()ed and missing. @@ -90,9 +96,18 @@ struct Node { TimeStamp mtime() const { return mtime_; } - bool dirty() const { return dirty_; } - void set_dirty(bool dirty) { dirty_ = dirty; } - void MarkDirty() { dirty_ = true; } + bool dirty() const { return dirty_ == DirtyState::DIRTY; } + void set_dirty(bool dirty) { + if (dirty) { + dirty_ = DirtyState::DIRTY; + } else if (dirty_ == DirtyState::DIRTY) { + dirty_ = DirtyState::CLEAN; + } + } + void MarkDirty() { dirty_ = DirtyState::DIRTY; } + + bool clean() const { return dirty_ == DirtyState::CLEAN; } + void MarkClean() { dirty_ = DirtyState::CLEAN; } bool dyndep_pending() const { return dyndep_pending_; } void set_dyndep_pending(bool pending) { dyndep_pending_ = pending; } @@ -144,7 +159,7 @@ struct Node { /// Dirty is true when the underlying file is out-of-date. /// But note that Edge::outputs_ready_ is also used in judging which /// edges to build. - bool dirty_ = false; + DirtyState dirty_ = DirtyState::SPARE; /// Store whether dyndep information is expected from this node but /// has not yet been loaded. From c139de0ffee333e4899864b1335e8c0f4a5092e1 Mon Sep 17 00:00:00 2001 From: Moritz Date: Sat, 27 Sep 2025 14:19:29 +0200 Subject: [PATCH 2/3] This commit will fix issue 2641 If dyndep applied, the algorithm will check any input instead of only one. Dyndep may have changed the input. --- src/graph.cc | 48 ++++++++++++++++++++++++++++++++++++++++++++---- src/graph.h | 3 +++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/graph.cc b/src/graph.cc index b18044cb88..ffc8caba6b 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -113,6 +113,13 @@ void propagateDirty::process(Edge* const edge) { bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, std::vector* validation_nodes, string* err) { + bool dummy(false); + return RecomputeNodeDirty(node, stack, validation_nodes, dummy, err); +} + +bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, + std::vector* validation_nodes, bool& dyndepLoadedPath, + string* err) { Edge* edge = node->in_edge(); if (!edge) { // If we already visited this leaf node then we are done. @@ -158,7 +165,8 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, // Later during the build the dyndep file will become ready and be // loaded to update this edge before it can possibly be scheduled. if (edge->dyndep_ && edge->dyndep_->dyndep_pending()) { - if (!RecomputeNodeDirty(edge->dyndep_, stack, validation_nodes, err)) + if (!RecomputeNodeDirty(edge->dyndep_, stack, validation_nodes, + dyndepLoadedPath, err)) return false; if (!edge->dyndep_->in_edge() || @@ -199,13 +207,25 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, validation_nodes->insert(validation_nodes->end(), edge->validations_.begin(), edge->validations_.end()); + if (dyndepLoaded) + dyndepLoadedPath = true; // Visit all inputs; we're dirty if any of the inputs are dirty. Node* most_recent_input = NULL; - for (vector::iterator i = edge->inputs_.begin(); - i != edge->inputs_.end(); ++i) { + for (std::size_t index_i = 0; index_i < edge->inputs_.size(); + index_i++) { // Visit this input. - if (!RecomputeNodeDirty(*i, stack, validation_nodes, err)) + const auto i = std::next(edge->inputs_.begin(), index_i); + + bool dyndepLoadedPathLocal = false; + if (!RecomputeNodeDirty(*i, stack, validation_nodes, dyndepLoadedPathLocal, + err)) { + if (dyndepLoadedPathLocal) + dyndepLoadedPath = true; return false; + } + + if (dyndepLoadedPathLocal) + dyndepLoadedPath = true; // If an input is not ready, neither are our outputs. if (Edge* in_edge = (*i)->in_edge()) { @@ -213,18 +233,38 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector* stack, edge->outputs_ready_ = false; } + bool finish = false; if (!edge->is_order_only(i - edge->inputs_.begin())) { // If a regular input is dirty (or missing), we're dirty. // Otherwise consider mtime. if ((*i)->dirty()) { explanations_.Record(node, "%s is dirty", (*i)->path().c_str()); dirty = true; + finish = true; } else { if (!most_recent_input || (*i)->mtime() > most_recent_input->mtime()) { most_recent_input = *i; } } } + + if (dyndepLoadedPathLocal && !finish) { + // recheck input, without current one. Dyndep may have added additional + // dependencies + for (std::size_t index = 0; index < index_i; ++index) { + if (edge->is_order_only(index)) + continue; + auto input = std::next(edge->inputs_.begin(), index); + if ((*input)->dirty()) { + explanations_.Record(node, "%s is dirty", (*input)->path().c_str()); + dirty = true; + break; + } else if (!most_recent_input || + (*input)->mtime() > most_recent_input->mtime()) { + most_recent_input = *input; + } + } + } } // We may also be dirty due to output state: missing outputs, out of diff --git a/src/graph.h b/src/graph.h index 0b47abc368..c22bda8cef 100644 --- a/src/graph.h +++ b/src/graph.h @@ -389,6 +389,9 @@ struct DependencyScan { private: bool RecomputeNodeDirty(Node* node, std::vector* stack, std::vector* validation_nodes, std::string* err); + bool RecomputeNodeDirty(Node* node, std::vector* stack, + std::vector* validation_nodes, + bool& dyndepLoaded, std::string* err); bool VerifyDAG(Node* node, std::vector* stack, std::string* err); /// Recompute whether a given single output should be marked dirty. From 8e0cf13f9d2bafb03f5ff02e66862e45c53dac42 Mon Sep 17 00:00:00 2001 From: Moritz Date: Sun, 19 Oct 2025 18:42:00 +0200 Subject: [PATCH 3/3] add unittest for issue 2653 and 2641 --- src/build_test.cc | 63 +++++++++++++++++++++++++++++++++++++++- src/graph_test.cc | 74 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/src/build_test.cc b/src/build_test.cc index 05fb949474..ab635c877c 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -655,7 +655,7 @@ bool FakeCommandRunner::StartCommand(Edge* edge) { // Don't do anything. } else if (edge->rule().name() == "cp") { assert(!edge->inputs_.empty()); - assert(edge->outputs_.size() == 1); + assert(edge->outputs_.size() >= 1); string content; string err; if (fs_->ReadFile(edge->inputs_[0]->path(), &content, &err) == @@ -4405,3 +4405,64 @@ TEST_F(BuildTest, ValidationWithCircularDependency) { EXPECT_FALSE(builder_.AddTarget("out", &err)); EXPECT_EQ("dependency cycle: validate -> validate_in -> validate", err); } + +// Test case: Late dyndep merges two initially separate dependency graphs into a +// single unified graph. +// https://github.com/ninja-build/ninja/issues/2653 +TEST_F(BuildTest, LateDynDepPropagateDirty) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, + "rule cp\n" + " command = cp $in $out\n" + "build b1: cp input_b\n" + "build b2: cp b1\n" + "build b3: cp b2\n" + "build b4: cp b3\n" + "build b5: cp b4\n" + "build a1: cp input_a\n" + "build a2: cp a1\n" + "build a3: cp a2 || dd\n" + " dyndep = dd\n" + "build a4: cp a3\n" + "build a5: cp a4\n")); + + fs_.Create("input_b", ""); + fs_.Create("b1", ""); + fs_.Create("b2", ""); + fs_.Create("b3", ""); + fs_.Create("b4", ""); + fs_.Create("b5", ""); + + fs_.Create("input_a", ""); + fs_.Create("a1", ""); + fs_.Create("a2", ""); + fs_.Create("a3", ""); + fs_.Create("a4", ""); + fs_.Create("a5", ""); + fs_.Create("dd", + "ninja_dyndep_version = 1\n" + "build a3 | input_b: dyndep |\n"); + + string err; + + fs_.Tick(); + fs_.Create("input_a", ""); + + command_runner_.commands_ran_.clear(); + state_.Reset(); + + EXPECT_TRUE(builder_.AddTarget("b5", &err)); + EXPECT_EQ("", err); + err.clear(); + EXPECT_TRUE(builder_.AddTarget("a5", &err)); + EXPECT_EQ("", err); + err.clear(); + + EXPECT_TRUE(GetNode("a5")->dirty()); + EXPECT_TRUE(GetNode("b5")->dirty()); + + EXPECT_EQ(builder_.Build(&err), ExitSuccess); + EXPECT_EQ("", err); + + EXPECT_EQ(10u, command_runner_.commands_ran_.size()); +} + diff --git a/src/graph_test.cc b/src/graph_test.cc index d29118ae3a..4f6b80a1b5 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -1140,4 +1140,78 @@ TEST_F(GraphTest, EdgeQueuePriority) { EXPECT_TRUE(queue.empty()); } +// https://github.com/ninja-build/ninja/issues/2641 +// https://github.com/ninja-build/ninja/issues/2653 +TEST_F(GraphTest, LateDynDep) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, R"ninja( +rule cp + command = cp $in $out +rule touch + command = touch $out + +build other3: cp other2 +build other2: cp other +build copy: touch other3 || orderOnly +build orderOnly: cp stamp +build stamp: cp in || dd + dyndep = dd +)ninja")); + + fs_.Create("orderOnly", ""); + fs_.Create("other", ""); + fs_.Create("other2", ""); + fs_.Create("other3", ""); + fs_.Create("copy", ""); + fs_.Create("stamp", ""); + fs_.Create("dd", + "ninja_dyndep_version = 1\n" + "build stamp | other: dyndep\n"); + fs_.Tick(); + fs_.Create("in", ""); + + string err; + EXPECT_TRUE(scan_.RecomputeDirty(GetNode("copy"), NULL, &err)); + ASSERT_EQ("", err); + + EXPECT_TRUE(GetNode("copy")->dirty()); + EXPECT_TRUE(GetNode("orderOnly")->dirty()); + EXPECT_TRUE(GetNode("stamp")->dirty()); + EXPECT_TRUE(GetNode("other")->dirty()); + EXPECT_TRUE(GetNode("other2")->dirty()); + EXPECT_TRUE(GetNode("other3")->dirty()); +} + +// https://github.com/ninja-build/ninja/issues/2641 +TEST_F(GraphTest, LateDynDep2) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, R"ninja( +rule cp + command = cp $in $out +rule touch + command = touch $out + +build copy: touch other || orderOnly +build orderOnly: cp stamp +build stamp: cp in || dd + dyndep = dd +)ninja")); + + fs_.Create("orderOnly", ""); + fs_.Create("other", ""); + fs_.Create("copy", ""); + fs_.Create("stamp", ""); + fs_.Create("dd", + "ninja_dyndep_version = 1\n" + "build stamp | other: dyndep\n"); + fs_.Tick(); + fs_.Create("in", ""); + + string err; + EXPECT_TRUE(scan_.RecomputeDirty(GetNode("copy"), NULL, &err)); + ASSERT_EQ("", err); + + EXPECT_TRUE(GetNode("copy")->dirty()); + EXPECT_TRUE(GetNode("orderOnly")->dirty()); + EXPECT_TRUE(GetNode("stamp")->dirty()); + EXPECT_TRUE(GetNode("other")->dirty()); +}