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

Conversation

johanneslerch
Copy link

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:

rule dyndepRule
   command = cmd /C echo test>$out&&echo foobar>output.dynamic&&(echo ninja_dyndep_version = 1 & echo build $out ^| output.dynamic : dyndep) > dyndep

build output.static | dyndep: dyndepRule input
   dyndep = dyndep

default output.static

In this example we create an output.static explicitly and during execution it is discovered that the build statement will have a dynamic output output.dynamic, which is communicated to Ninja via the dyndep file. Just like one would use the dynout 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?

@digit-google
Copy link
Contributor

Here's an explanation for your cycle error (which is legitimate here). You build plan looks like the following:

      input
        |
        v
  [===========]
  |           |
  v           v
out.static   dyndep

This is the shape of the build graph when it is first loaded by Ninja, where [====] corresponds to your build statement (an Edge instance in Ninja speak).

When you run your build for the first time, the command generates out.static, out.dynamic and dyndep, then Ninja parses the content of the dyndep file to record it in its log. However, what it really records is equivalent to these dependencies:

out.static --> dyndep
out.dynamic --> dyndep

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, a --> b will be injected as "the command that builds a depends on b as an input.

But in your case, the command that generates out.static already produces dyndep, so it cannot depend on its own output as an input. That's the cycle error that you are seeing.

@johanneslerch
Copy link
Author

But in your case, the command that generates out.static already produces dyndep, so it cannot depend on its own output as an input. That's the cycle error that you are seeing.

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) {
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants