Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support single statement usage of dyndep #2481

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,10 @@ bool Plan::EdgeFinished(Edge* edge, EdgeResult result, string* err) {
edge->outputs_ready_ = true;

// Check off any nodes we were waiting for with this edge.
for (vector<Node*>::iterator o = edge->outputs_.begin();
o != edge->outputs_.end(); ++o) {
// Iterate over a copy, since NodeFinished might update (implicit) outputs.
vector<Node*> edgeOutputs(edge->outputs_);
for (vector<Node*>::iterator o = edgeOutputs.begin();
o != edgeOutputs.end(); ++o) {
if (!NodeFinished(*o, err))
return false;
}
Expand Down
17 changes: 17 additions & 0 deletions src/dyndep.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ bool DyndepLoader::LoadDyndeps(Node* node, DyndepFile* ddf,
if (!LoadDyndepFile(node, ddf, err))
return false;

if(node->in_edge() != nullptr && node->in_edge()->dyndep_ == node) {
Edge* const edge = node->in_edge();
DyndepFile::iterator ddi = ddf->find(edge);
if (ddi == ddf->end()) {
*err = ("'" + edge->outputs_[0]->path() + "' "
"not mentioned in its dyndep file "
"'" + node->path() + "'");
return false;
}

ddi->second.used_ = true;
Dyndeps const& dyndeps = ddi->second;
if (!UpdateEdge(edge, &dyndeps, err)) {
return false;
}
}

// Update each edge that specified this node as its dyndep binding.
std::vector<Edge*> const& out_edges = node->out_edges();
for (Edge* edge : out_edges) {
Expand Down
59 changes: 52 additions & 7 deletions src/graph.cc
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector<Node*>* stack,
stack->push_back(node);

bool dirty = false;
edge->outputs_ready_ = true;
edge->deps_missing_ = false;
edge->outputs_ready_ = true;

if (!edge->deps_loaded_) {
// This is our first encounter with this edge.
Expand All @@ -126,18 +126,63 @@ bool DependencyScan::RecomputeNodeDirty(Node* node, std::vector<Node*>* 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))
return false;
if(edge->dyndep_->in_edge() == edge) {
Copy link
Author

@johanneslerch johanneslerch Aug 23, 2024

Choose a reason for hiding this comment

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

Here is the huge change. Previously Ninja would detect a cycle here, which we now handle.
The else branch starting on line 171 is basically the old code and the then branch what we've added for the single statement use case.
This then branch is copying code that repeats further down (and deserves an extract method to avoid repetition).

What we do here is basically evaluating dirtiness without considering the dyndeps. If that turns out to not be dirty, only then we load the dynamic outputs. Note that the original code coming later in this function will evaluate dirtiness again after the dyndeps have been loaded.

The idea we had about this is that we mimic the original check
edge->dyndep_->in_edge()->outputs_ready()
which only loads dyndeps if the outputs of the in_edge are ready.

Node* most_recent_input = NULL;
for (vector<Node*>::iterator i = edge->inputs_.begin();
i != edge->inputs_.end(); ++i) {
// Visit this input.
if (!RecomputeNodeDirty(*i, stack, validation_nodes, err))
return false;

// If an input is not ready, neither are our outputs.
if (Edge* in_edge = (*i)->in_edge()) {
if (!in_edge->outputs_ready_)
edge->outputs_ready_ = 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;
} else {
if (!most_recent_input || (*i)->mtime() > most_recent_input->mtime()) {
most_recent_input = *i;
}
}
}
}

// Load output mtimes so we can compare them to the most recent input below.
for (vector<Node*>::iterator o = edge->outputs_.begin();
o != edge->outputs_.end(); ++o) {
if (!(*o)->StatIfNecessary(disk_interface_, err))
return false;
}

if (!edge->dyndep_->in_edge() ||
edge->dyndep_->in_edge()->outputs_ready()) {
// The dyndep file is ready, so load it now.
if (!LoadDyndeps(edge->dyndep_, err))
if (!RecomputeOutputsDirty(edge, most_recent_input, &dirty, err))
return false;
if(!dirty) {
// The dyndep file is ready, so load it now.
if (!LoadDyndeps(edge->dyndep_, err))
return false;
}
} else {
if (!RecomputeNodeDirty(edge->dyndep_, stack, validation_nodes, err))
return false;

if (!edge->dyndep_->in_edge() ||
edge->dyndep_->in_edge()->outputs_ready()) {
// The dyndep file is ready, so load it now.
if (!LoadDyndeps(edge->dyndep_, err))
return false;
}
}
}
}


// Load output mtimes so we can compare them to the most recent input below.
for (vector<Node*>::iterator o = edge->outputs_.begin();
o != edge->outputs_.end(); ++o) {
Expand Down
6 changes: 5 additions & 1 deletion src/manifest_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,11 @@ bool ManifestParser::ParseEdge(string* err) {
vector<Node*>::iterator dgi =
std::find(edge->inputs_.begin(), edge->inputs_.end(), edge->dyndep_);
if (dgi == edge->inputs_.end()) {
return lexer_.Error("dyndep '" + dyndep + "' is not an input", err);
//dyndep not found in inputs, lets check if we have it as output
dgi = std::find(edge->outputs_.begin(), edge->outputs_.end(), edge->dyndep_);
if (dgi == edge->outputs_.end()) {
return lexer_.Error("dyndep '" + dyndep + "' is not an input nor an output", err);
}
}
assert(!edge->dyndep_->generated_by_dep_loader());
}
Expand Down
2 changes: 1 addition & 1 deletion src/manifest_parser_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1086,7 +1086,7 @@ TEST_F(ParserTest, DyndepNotInput) {
"build result: touch\n"
" dyndep = notin\n",
&err));
EXPECT_EQ("input:5: dyndep 'notin' is not an input\n", err);
EXPECT_EQ("input:5: dyndep 'notin' is not an input nor an output\n", err);
}

TEST_F(ParserTest, DyndepExplicitInput) {
Expand Down