-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
Support single statement usage of dyndep #2481
Conversation
Here's an explanation for your cycle error (which is legitimate here). You build plan looks like the following:
This is the shape of the build graph when it is first loaded by Ninja, where When you run your build for the first time, the command generates
If you invoke Ninja a second time, it will load the graph as above from the manifest, then read these logs to inject these additional dependencies into it. More specifically, But in your case, the command that generates |
I've understood that part. And I tried to state that we've dropped the raised errors and added some modifications to Ninja code that make this scenario work. So the example you just explained why it does't work is actually working with the changes of this PR. |
@@ -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) { |
There was a problem hiding this comment.
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.
We've been using dynout from #1953 successfully for some years, until recently we encountered a problem with it #1953 (comment).
#1953 has in the meantime been discontinued in favor of #2366 for which we encountered other issues (there has been progress and improvements addressing them in the meantime).
During all this, we had a closer look at the dyndep implementation and asked ourselves, whether that could be used from within a single build statement, too. Something like this:
In this example we create an
output.static
explicitly and during execution it is discovered that the build statement will have a dynamic outputoutput.dynamic
, which is communicated to Ninja via thedyndep
file. Just like one would use thedynout
from #1953 / #2366.While trying that, we immediately ran into some errors complaining that this is creating a cyclic dependency on itself. But digging deeper, we did not discover why these errors must be thrown. Moreover, we proceeded by removing them. Added some small workarounds and eventually reached a state in which it seems to work. Though, we focused on dynamic outputs and did not do a single test with dynamic inputs yet.
This PR is not meant to be in a state worth merging at this moment, as clearly we didn't add any tests (shame!) and added a lot of code duplication that deserves some extract method refactorings.
Right now, I would wish to use this PR as a means to discuss whether this direction is worth proceeding. Moreover, I want to raise the question: did we miss something conceptually why this approach will not generalize?