Add a -t missingdeps tool that helps fight build flakiness #1031
Conversation
The tool looks for targets that depend on a generated file, but do not properly specify a dependency on the generator. The search relies on the correctness of depfile information. The tool needs to be run after a build completes and depfile data is collected. On my machine it takes around 1.5 seconds to process a chromium "chrome" target's data, with 15 targets found with missing dependencies on generated files.
| @@ -279,6 +279,19 @@ http://clang.llvm.org/docs/JSONCompilationDatabase.html[JSON format] expected | |||
| by the Clang tooling interface. | |||
| _Available since Ninja 1.2._ | |||
|
|
|||
| `missingdeps`:: given a list of targets, look for targets that depend on | |||
There was a problem hiding this comment.
You should add an "Available since Ninja 1.x" line to this, see how it's done above.
|
I'm not sure this always works. You can have a build set up like: That is, there's some "generate_headers" rule that generates a bunch of files, where Ninja doesn't even know their names, and only discovers the names of the actual headers via the depfiles. (I imagine this is the common way header generation works.) |
|
Right, this case would not be detected, so the tool depends on code generators listing their outputs properly. Arguably, the build is not always correct if generators don't list their outputs like in the example above: if a.h generated from a.idl gets removed, a rebuild will fail with 'foo.o needs a.h but no targets generate this file'. AFAICT most generators in Chrome do list their outputs. The tool is still useful (http://crbug.com/536641 lists missing dependencies found so far), but it might be useful to try and detect the other class of missing deps too. It should be possible as long as the generated files are generated below the outdir, nothing really possible if the files are created in the source tree. Do you think it makes sense to add "x depends on y, which is in outdir, but no targets list y as an output" error detection to the tool? Alternative is to document the assumption that generators list outputs. |
| of a generator-target) via a depfile, but does not have a non-depfile | ||
| dependency path to the generator-target, is considered broken. | ||
| + | ||
| The tool's finidings can be verified by trying to build the listed targets in |
Add a secondary check for the missingdeps tool that looks for files under the build dir, or a specified subdirectory of the build dir, for files that are deps log dependencies of other targets, but do not appear as outputs of any target. This suggest they are generated by a generator that does not list outputs properly.
|
It's been a while but I've managed to find some time for this and I'd like to continue. I added a path-based check looking for cases where a generated file has no generator target, had to shuffle code a bit to avoid excessive nesting or passing 10 parameters around a recursive function. The new check seems to work and finds around 20 such files in a Chromium build. It has however lengthened the tool runtime from 2 to 8 seconds on my machine, and will not work for in-source builds (i.e., in src/ninja itself), so I added a way to disable the extra check. The tool can really help squash build flakiness before it breaks builds and I would love to see it in the official builds. |
This makes the missing dependency check more thorough. Practically speakiung, it now catches problems around GN actions with depfiles just like it did catch compilatior header dependdency problems.
|
Ping? |
|
ping |
|
Ping again. There are 1800+ instances of build edges having incomplete dependencies in Chromium currently (https://bugs.chromium.org/p/chromium/issues/detail?id=655123) that only work because ninja happens to choose an ordering that work. Ninja should make it easy to keep a build non-flaky. |
|
I worked on the Chrome build many years ago, and at that time the IDL pattern (where steps don't list their outputs) I mentioned was quite common. But if this really is helping then maybe it makes sense. I skimmed the code and it mostly looks good to me. The one part that makes me a bit concerned is how it needs to deal with both DepsLog and DepFileParser. Perhaps there's a pre-refactoring you could do to avoid needing to duplicate this code? |
|
Pushed a merge to have things build again, but the issue evmar points out is regrettable code duplication with build.cc that should really be avoided. Will have to clean that up then clean up history. Generator steps that don't list the outputs are a slightly separate case, also risky from a build stability POV. Right now there's code that will try to list files under the outdir that are depfile-depended on, but don't have a generator. It's not super useful in Chrome because there's tons of instances, plus numerous sort-of false positives where a generator lists gen/foo.h but a depfile lists ../Release/gen/foo.h (or the other way around, don't remember now). |
|
Closing in favor of #1331. |
The tool looks for targets that depend on a generated file, but do not
properly specify a dependency on the generator. The search relies on
the correctness of depfile information.
The tool needs to be run after a build completes and depfile data is
collected. On my machine it takes around 1.5 seconds to process a
chromium "chrome" target's data, with 15 targets found with missing
dependencies on generated files.