This commit will fix issue#2653.

[moritzx22]

This Pull request 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.
For a detailed problem description, see issue#2653

[moritzx22]

Test Bench

Example build file for testing:

rule cp
  command = cp $in $out

rule cp_dynB
  command = cp $in $out && touch $stamp && touch $output
  dyndep = $stamp.dd

rule mkdyn
  command = printf 'ninja_dyndep_version = 1\nbuild  $st_out | $dyn_out: dyndep | $dyn_in\n' > $stamp.dd

build b1: cp  b
build b2: cp  b1
build b3: cp  b2
build b4: cp  b3
build b5: cp  b4

build a1: cp  a
build a2: cp  a1
build a3: cp_dynB a2 || stamp-a.dd
  stamp = stamp-a
  output = b
build a4: cp  a3
build a5: cp  a4

build stamp-a.dd: mkdyn
  stamp = stamp-a
  dyn_out = b
  st_out = a3

now do the following commands:

$ touch a b && ninja
$ touch a && ninja b5 a5
$ touch a && ninja b5 a5

In the master branch are issues, not the required processes are always started. The solution of this Pull Request works well.

Analysis

the next figure reports the dependency graph without dyndep[noDyndep]:

[noDyndep]

The next figure reports the dependency graph with dyndep[complete], using this Pull Request, and target a is touched:

[complete]

The red means dirty, blue means clean.

The next figure reports the dependency graph using master branch, and target a is touched:

[master]

It can be easily seen that targets with b are not considered to be dirty by the master branch.

Scanning Algorithm Master branch

1. First target b5 will be added. There are still 2 independent graphs a and b, see figure [noDyndep]. Nothing in graph b is dirty, and nothing is detected.
2. Second target a5 is added. Now the dyndep is added, the graph dependency changes. Only nodes with an a are visited and are marked as dirty. The targets with an b are not marked as dirty.
3. no further action, as b5 has already been added, any b is missed to be marked as dirty.

The algorithm does not reflect that the graph may change. This issue occurs if a dyndep adds an output target (here b) and if this target is dirty and has other outputs.

Proposed Scanning Algorithm enhancement

if the dyndep is applied, there is an additional iterating algorithm across the graph following the outputs. The outputs are only followed if they have been visited before. Therefore a node can now be dirty, clean and spare. This does prevent to mark nodes as dirty which are not requested to be built.
The command $ touch a && ninja a5 would not follow the b targets.

The additional iterating over the graph starts here: iterating

[moritzx22]

The recently pushed commit will address issue#2641, it is a simular dyndep issue as #2653.

Test Bench

Similar as reported in the issue, just one target was added abc

rule dyndep
  command = printf 'ninja_dyndep_version = 1\nbuild stamp | other: dyndep\n' > $out

rule build_with_dyndep
  command = touch $out other
  dyndep = $out.dd

rule depends_on_dyndep
  command = printf '%s: other\n' $out > $out.d && cp other $out
  depfile = $out.d
  
rule cp
  command = cp $in $out

build stamp.dd: dyndep
build stamp: build_with_dyndep || stamp.dd
build abc: cp stamp
build copy: depends_on_dyndep || abc

Following graph with proposed solution

Following graph without proposed solution (copy is not dirty and therefore not build)

blue is not build, red is build.
The following commands has been used to generate the graphics.

$ ninja -v -d explain
adding spaces to the build_with_dyndep command
$ ninja -v -d explain

[moritzx22]

updates:

• fix for issue 2641
• fix and update in unittest

[dseomn]

A previous version of this fixed #2662 (see #2653 (comment)), but I just tested c74beb4...moritzx22:8e0cf13f9d2bafb03f5ff02e66862e45c53dac42 on ninja 1.13.2, and it doesn't fix #2662 anymore. (It doesn't claim to fix that either, so that's fine. Just FYI in case you thought it did still fix that.)

[moritzx22]

A previous version of this fixed #2662 (see #2653 (comment)), but I just tested c74beb4...moritzx22:8e0cf13f9d2bafb03f5ff02e66862e45c53dac42 on ninja 1.13.2, and it doesn't fix #2662 anymore. (It doesn't claim to fix that either, so that's fine. Just FYI in case you thought it did still fix that.)

This PR was never intended to fix 2662. I previously created a separate branch quickfixIssue2653 as a quick workaround for issue 2663, without opening a PR for it. As described in this comment, that branch also happened to fix 2662 — likely by coincidence.

• Issue 2663 is well understood and is directly addressed by this PR.
• Issue 2662 is not well understood and is not targeted here.
  • not reproducible, difficult to address

Both the quickfix branch and this PR attempt to solve similar underlying problems (Certain dynamic dependencies are not loaded in time for correctly computing the dirty state of a node), but in very different ways:

• the quickfix branch uses a broad “sledgehammer” approach (load all dynamic dependencies in a separate algorithm, prior to computing the dirty state of the nodes),
• this PR applies a much more targeted fix for a specific issue.

[jhasse]

@bradking What do you think?

[mathstuf]

Adding a comment here to remind myself to review this for #2666 effects as well.

[bradking]

The analysis and proposed solution in #2675 (comment) LGTM. I've not reviewed the implementation though.

[mathstuf]

This does not solve #2666 (but wanted to check).

[mathstuf]

I have verified that CMake's test suite for C++ modules and Fortran still work with this PR as well.

I've not reviewed the code.

[moritzx22]

This does not solve #2666 (but wanted to check).

• 2666
  • Root issue: the build graph is updated based on outdated depfiles, which can lead to false cycle detection.
    • This is not directly tied to dynamic dependency handling.
  • The problem also appears in the CMake/module area.
  • The underlying cause is already well understood.
  • In terms of impact, 2666 is more critical than PR 2675.
    • build will fail, and resolving the issue may require a full rebuild.
  • I agree with @mathstuf that PR 2675 is unrelated to 2666.