missingdeps tool, take 2

[ilor]

This is a tool that detects some classes of random build failure causes around generated files (in Chrome, these are usually generated headers for C/C++). It takes several seconds to run, (more if the optional scan for files with no generator is used), and allows fixing a significant number of build system issues before they happen to randomly break someone's build.

The check relies on the correctness of depfile information, and on having all generator outputs listed. These things are usually easier to get right than proper deps. There's some overlap between this and gn check, but unlike gn check, this checks the actual ninja build graph and sees all issues.

This is a redo of #1031, it's been a while so I think it maybe better to start from scratch.

[ilor]

Thanks for the sugesstions so far. I'm also uploading example output of this tool, when run on a fairly recent chrome-linux build for the chrome target: missingdeps-chrome-output.gz

The summary is:

Processed 39546 nodes.
Error: 161 file(s) under /src/chromium/src/out are deps log dependencies of other targets, but have no generator target listing them as outputs.
Error: There are 156 missing dependency paths.
75 targets had depfile dependencies on 87 distinct generated inputs (from 34 rules) without a non-depfile dep path to the generator.
There might be build flakiness if any of the targets listed above are built alone, or not late enough, in a clean output directory.

There's one case that's arguably a false-positive: Missing dep: gen/components/resources/about_credits.html uses build.ninja (generated by gn) which may require a special case to not treat it as an error.

I also would like to have some sort of test for this, but don't really know where such a test (that needs an after-a-build state, basically) would hae tobe.

[ilor]

Ping?

[ilor]

Ping? I still believe this would be useful to have in ninja.

[ilor]

Ping? This could help fix a lot of rare clean-build build flakes in the chrome gn/ninja build, see https://bugs.chromium.org/p/chromium/issues/detail?id=655123 for a list of issues found.

[jcrben]

I'm just a random web developer wanting to try building chromium on macOS 10.12.6 using the instructions at https://chromium.googlesource.com/chromium/src/+/67.0.3360.2/docs/mac_build_instructions.md, and running into issues.

Hoping that any efforts to reduce flakiness / missing deps can be merged, so +1 on the ping.

When I ran this tool, I got

(note: this was not enough to help me fix my issue - error was specifically ../../services/network/public/cpp/resource_response_info.h:24:10: fatal error: 'services/network/public/mojom/fetch_api.mojom-shared.h' file not found, which is apparently that mojom files are not generating their C++ files - chromium could perhaps use more instructions or more of a scripted setup)

[bratell-at-opera]

I just ran into missing mojom.h files in a Chromium build and I wonder if this tool would have caught the problem when it was introduced. This seems like a good thing to have so I hope some maintainer can take a look.

[atetubou]

Can I ask you to add some tests showing the behavior of this new tool?

[ilor]

Thanks for taking a look. What would be the preferred form of these tests?

My initial idea was that to show the behavior, I'd need several build directories with manually written ninja files for some types of mistakes. Then from some runner script I'd build in each of these and then run the tool to make sure I get the expected diagnostic. Would that be okay? I don't recall anything quite like it in the repo, so I'm not quite sure where I'd put it.

Something like this, assuming there's a generator for foo.h but foo.o doesn't dep on it:

cd missing-deps-test1
ninja clean  # assuming this is clean enough
ninja foo.h   # must be manually built first -- full builds are flaky
ninja foo.o
ninja -t missingdeps   # should report foo.o lacks a dep to foo.h's generator

then repeated for a few other cases.

[atetubou]

I'd rather to have more unittest like test.

[jhasse]

My initial idea was that to show the behavior, I'd need several build directories with manually written ninja files for some types of mistakes. Then from some runner script I'd build in each of these and then run the tool to make sure I get the expected diagnostic. Would that be okay? I don't recall anything quite like it in the repo, so I'm not quite sure where I'd put it.

Not sure if this is what you're looking for, but there's https://github.com/ninja-build/ninja/blob/master/misc/output_test.py which runs the ninja binary on a temporary file and checks its output.

[ilor]

Admittedly, the timing is not great with regards to me being able to spend as much time as I'd like on this. That said, I've rebased to master and fixed some minor issues, but will need a while for the tests.

The "main" obstacle is that the code uses classes like ImplicitDepLoader which want to load a real file and aren't triviallyfed test data; I guess I can use a temp file for now as I see some tests do that.

[ilor]

I'm working on this and should have something this week. It will be simplifies without the "files that lack a generator target" part (-g switch) because that adds a lot of complexity that's not needed to see the primary feature of the tool.

[ilor]

Highlights from the update:

• Rebased onto master
• Separate cc/h pair for the tool, as it is large enough
• Unit test for the primary thing the tool should catch
• Remove -g switch and related code
• Avoid touching state.h/cc, keep the edge adjacency map inside the tool

Removing -g and extracting to a separate source file simplified things a lot, and as a bonus now Windows should be able to build this without excessive ifdeffery or build config. I never found the -g tool to be very useful, it can be added later if there's a serious need.

Please take a look

[ilor]

Thanks for the comments so far, should be all addressed. Please let me know what I can do next to get this into a merge-able state.

[ilor]

Friendly ping

[jhasse]

Sorry, I don't have time right now to give this a closer look. I'm not sure if it's gonna make 1.10.

[ilor]

Thanks for replying, I understand. While I think this can be very useful, it's not something I have to rush to any particular version.

I'll try to ping again in a week or a couple, maybe clean-up the branch history in the meantime (unless that's not welcome at this stage; I'm not sure how helpful/disruptive it'd be in the middle of a pull request).

[ilor]

ping?

[deepakcse87]

Nice tool. Looking forward to use it when it gets merged into master.

[ilor]

@jhasse friendly ping

[jhasse]

This PR is on the 1.11.0 milestone, I will definitely have a look at it before doing that release. Currently I'm working on 1.10.2 though, please be patient.

[ilor]

That's good to hear, I'll be waitng.

[jhasse]

Can you rebase on master? I think I found a crash in the unit test due to an assert.

[ilor]

The test crash seems to be because the test code that writes a depfile needs to be adjusted after cc79afb changed some bits in deps_log.cc

[ilor]

ping -- this should be better now, the macos failure looks like a infra issue

[jhasse]

Thanks! The code looks good :) There's one performance impact though:

While implicit_dep is still preallocated, the new temporary vector depfile_deps will grow on the fly. It's also now two loops because of the temporary vector.

Either

1. show that this has no significant performance impact with an appropriate benchmark,
2. or pass a callback to LoadDepfileDeps instead of a pointer to a vector. Call it and pass the node instead of doing depfile_deps->push_back(node);.

[ilor]

@jhasse thanks, with all the quaint pre-C++11 support going on "use a callback" is a bit less enticing than it should otherwise be but I'll probably end up doing that. I may follow up with some perf experiments later if I find the time because I do feel like one extra vector and some pointer copies there should be immaterial.

[jhasse]

I hope it's okay I've tried the "Allow changes from maintainers" feature of GitHub and pushed to your branch 🙈

It isn't pretty, especially PreallocateSpace and CreatePhonyInEdge shouldn't be public. Maybe you have a better idea.

[ilor]

I think the virtual function approach works out better now, especially when viewed vs the base branch. Should be little effectively no changes on the main code path now.

[ilor]

friendly ping

[jhasse]

That's a good solution :)