The chromium build is flaky around code generators (gn misses checking generated files?)
Reported by
tsniatow...@opera.com,
Oct 12 2016
|
||||||||||||||||||
Issue descriptionVersion: master@2016-10-11 / a20d844fcd1e386d8bbbd2782e5946b4bcb02978 / refs/heads/master@{#424427} OS: any What steps will reproduce the problem? (1) Try to build Chromium, preferably with a slightly odd setup that makes things build in a slightly different order than usually (2) Make sure this is a clean build as incremental builds are fine (3) To trigger the issue in a non-random manner, try to build just an object file that's identified as failing. For example: gn gen out gn clean out ninja -C out2 obj/chrome/browser/ui/ui/predictors_ui.o What is the expected output? Build works reliably. What do you see instead? Multiple random build failures where files don't compile because they try to include generated code that hasn't been generated yet. Errors look like: In file included from ../chrome/browser/ui/webui/predictors/predictors_ui.cc:8: In file included from ../chrome/browser/ui/webui/predictors/predictors_handler.h:10: In file included from ../chrome/browser/predictors/resource_prefetch_predictor.h:23: ../chrome/browser/predictors/resource_prefetch_predictor_tables.h:20:10: fatal error: 'chrome/browser/predictors/resource_prefetch_predictor.pb.h' file not found #include "chrome/browser/predictors/resource_prefetch_predictor.pb.h" We hit issues like these occasionally downstream as random build failures. They probably don't happen too often in Chromium itself because build edges happen to be in an order that works. Using a tool mentioned in https://bugs.chromium.org/p/chromium/issues/detail?id=536641 I was able to identify *1880* .o files that can fail when built individually, from obj/chrome/browser/devtools/devtools/android_rsa. through obj/content/renderer/mus/mus/compositor_mus_connection.o to obj/third_party/WebKit/Source/modules/notifications/notifications/NotificationData.o. That's almost two thousand filesand that's only for the chrome target on Linux. See attached file for a full list. This situation is getting worse now that there are more and more instances of code generators in the build. Most of the issues are mojo headers or blink generated name headers. I tried fixing some issues like these a while back but it feels somewhat pointless. I think there should be some enforcement of this so a successful build is reliable and not a matter of lucky ordering.
,
Oct 14 2016
I think this will never be fully reliable in gn only. Things like //nogncheck or non-c++ dependencies on generated code iwll make this only catch some issues (though maybe most). It's only in ninja, and only after a build, do we really have a way of knowing that a dependency is flaky. Doing a ninja-level check after a build, like the current "ninja: nothing to do" check would be the way to go IMO. The snag is that it currently takes a patched ninja and the upstreaming attempt went nowhere. Perhaps it would make sense to push a bit more in https://github.com/ninja-build/ninja/pull/1031 so the check can eventually be a FYI step on bots.
,
Oct 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f795cde5fb6c8218a465d9dfa4b0f9fae71bcdbc commit f795cde5fb6c8218a465d9dfa4b0f9fae71bcdbc Author: tsniatowski <tsniatowski@opera.com> Date: Wed Oct 26 10:26:43 2016 Fix some mojo dependencies in blink Several places in blink were using mojo headers without a dependency on mojo targets that generate said headers, causing build flakiness. BUG=655123 Review-Url: https://codereview.chromium.org/2453653003 Cr-Commit-Position: refs/heads/master@{#427659} [modify] https://crrev.com/f795cde5fb6c8218a465d9dfa4b0f9fae71bcdbc/third_party/WebKit/Source/bindings/modules/BUILD.gn [modify] https://crrev.com/f795cde5fb6c8218a465d9dfa4b0f9fae71bcdbc/third_party/WebKit/Source/modules/geolocation/BUILD.gn [modify] https://crrev.com/f795cde5fb6c8218a465d9dfa4b0f9fae71bcdbc/third_party/WebKit/Source/modules/notifications/BUILD.gn
,
Oct 26 2016
Issue 659503 has been merged into this issue.
,
Oct 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c50ed22545a79b7947f3b3fc2efe602c63db3d8d commit c50ed22545a79b7947f3b3fc2efe602c63db3d8d Author: tsniatowski <tsniatowski@opera.com> Date: Wed Oct 26 22:41:23 2016 Fix a large number of missing dependencies in the blink gn build Make all blink_core_sources targets public_dep on all the code generators in core to ensure required headers are always generated first and a successful build does not depend on lucky ordering. Manually fix similar dep issues in core/inspector. There are now more dependencies than strictly necessary, but they will only trigger the generators with no effect on build commands (tested by checking that the patch doesn't trigger a rebuild of any c++ code). The end result is that the total number of targets that don't have proper deps in the 'chrome' target build goes down from over 1800 to about 40, and no missing dependencies on gen/blink files exist. BUG=655123 R=dpranke@chromium.org Review-Url: https://codereview.chromium.org/2452473004 Cr-Commit-Position: refs/heads/master@{#427856} [modify] https://crrev.com/c50ed22545a79b7947f3b3fc2efe602c63db3d8d/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/c50ed22545a79b7947f3b3fc2efe602c63db3d8d/third_party/WebKit/Source/core/core.gni [modify] https://crrev.com/c50ed22545a79b7947f3b3fc2efe602c63db3d8d/third_party/WebKit/Source/core/inspector/BUILD.gn
,
Oct 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e414cb39fc1746256be57f3d364b6e90a0211a59 commit e414cb39fc1746256be57f3d364b6e90a0211a59 Author: tsniatowski <tsniatowski@opera.com> Date: Fri Oct 28 17:43:40 2016 Add a //chrome/common dep to //chrome/browser/devtools Devtools include chrome/common headers which include the generated features header, so without the dep the build is flaky. BUG=655123 Review-Url: https://codereview.chromium.org/2454943004 Cr-Commit-Position: refs/heads/master@{#428413} [modify] https://crrev.com/e414cb39fc1746256be57f3d364b6e90a0211a59/chrome/browser/devtools/BUILD.gn
,
Oct 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef21ead8498674c5e7ccaccd2f50f89ab969cccd commit ef21ead8498674c5e7ccaccd2f50f89ab969cccd Author: tsniatowski <tsniatowski@opera.com> Date: Fri Oct 28 18:12:51 2016 Add missing generator dependencies in content/renderer/mus Building //content/renderer/mus could fail due to transitive dependencies on header generators pulled in via render_frame_impl.h and render_thread_impl.h (building render_widget_mus_connection.cc or compositor_mus_connection.cc could fail). Unfortunately //content/renderer deps on //content/renderer/mus, so there's no easy way to get these deps for free (cyclic dep). BUG=655123 Review-Url: https://codereview.chromium.org/2461643002 Cr-Commit-Position: refs/heads/master@{#428428} [modify] https://crrev.com/ef21ead8498674c5e7ccaccd2f50f89ab969cccd/content/renderer/mus/BUILD.gn
,
Nov 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23 commit dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23 Author: tsniatowski <tsniatowski@opera.com> Date: Thu Nov 03 11:51:52 2016 Fix a bunch of generated file build flakes in //extensions Several files in //extensions could randomly fail to build due to missing dependencies on header generator targets, mostly mojo and grit. Add the dependencies so builds are not flaky. BUG=655123 Review-Url: https://codereview.chromium.org/2452943003 Cr-Commit-Position: refs/heads/master@{#429543} [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/chrome/browser/extensions/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/api/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/api/bluetooth_low_energy/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/api/serial/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/api/web_request/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/app_window/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/guest_view/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/guest_view/app_view/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/guest_view/extension_options/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/guest_view/extension_view/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/guest_view/mime_handler_view/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/browser/guest_view/web_view/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/common/api/BUILD.gn [add] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/common/api/schema.gni [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/renderer/BUILD.gn [modify] https://crrev.com/dc44a53ed3244a4a68cb1fd4ccfaacf1e97e5d23/extensions/shell/BUILD.gn
,
Mar 3 2017
,
Mar 3 2017
,
Sep 20 2017
,
Sep 20 2017
I'm attempting to make ninja include a tool to detect these cases in https://github.com/ninja-build/ninja/pull/1331. I'd like for this to eventually become a post-build sanity check much like the current "ninja: nothing to do" check verifies a build is not dirty after it's done. For reference, there's a whole bunch of problems detected by the tool in chrome right now; 156 missing dependency paths and 161 generated files depended on, but not listed as outputs (so the tool can't check). Attaching full log here too, for reference.
,
Sep 21 2017
,
Sep 21 2017
,
Sep 21 2017
,
Oct 4 2017
Ping? Opinions/comments?
,
Oct 4 2017
I have verified that after doing a full build, deleting gen/devtools/object_ui/object_ui_module.js breaks incremental builds. While deleting a generated file is an unusual thing to do, I have done it at times, and I think it is reasonable to expect that it would be rebuilt correctly. Having correct and complete dependencies is a key requirement for reproducible builds, so it would be very good to have this check on the CQ, provided the cost is not too high.
,
Oct 9 2017
ricea@: The errors where if you just try to build one output and it will reliably fail (without anyone deleting anything, just a clean build) are more "fun". Especially if you hit them at random doing a normal build because things happened to order that way. The cost looks to be several to a dozen or so seconds after a build. Main obstacle now is getting the change in Ninja -- I think doing it externally is not very good, as it will be duplicating most the Ninja build-graph code just for the sake of being external, and will likely be slower too. Sadly, not much is happening in the pull request. Ping @ thakis, I guess.
,
Oct 17 2017
,
Oct 19 2017
see also https://chromium-review.googlesource.com/c/727443/, would this fix it?
,
Oct 19 2017
It would possibly fix some of the issues. Some will still remain, and there will still be no protection from this state rotting away in the future.
,
Dec 14 2017
Status as of a recent master (2017-12-11, 61453dff32d108bb227fe1f9ee024b70fae612a4) for a full default Linux build is: Processed 68617 nodes. Error: There are 371 missing dependency paths. 136 targets had depfile dependencies on 149 distinct generated inputs (from 73 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. Full from the patched ninja run log attached.
,
Feb 19 2018
As of 4fc388b1a58ab9240d2fb3c8772ef309be1664a4 things are not any better: Error: There are 607 missing dependency paths. 283 targets had depfile dependencies on 136 distinct generated inputs (from 59 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.
,
Feb 19 2018
See also bug 712456 which lists a couple of missing dependencies which don't seem to show up in the most recent missing-deps report.
,
Jun 7 2018
Issue 712456 has been merged into this issue.
,
Jul 31
This issue hits sheriffs again and again. https://bugs.chromium.org/p/chromium/issues/detail?id=869241 Can we raise a priority of this task?
,
Jul 31
We could raise the priority, but more importantly we don't actually have someone to own the problem and make sure people are working on fixing it, so finding that person (or people) would also have to happen.
,
Jul 31
Right now, it's true that this is a problem, but there are a lot of build problems (like flaky tests) and the limited resources we have are working on other things instead :(.
,
Dec 10
Maybe a $2000 prize for fixing this issue? :) CC'ing more affected people.
,
Dec 11
For reference, here's the missing deps report from today's master > Error: There are 135 missing dependency paths. > 73 targets had depfile dependencies on 62 distinct generated inputs (from 32 rules) without a non-depfile dep path to the generator. Fixing this class of bugs would require getting the check working in ninja, and enforcing it in CQ. Unfortunately there seems to be no interest on the ninja (maintainers') side of things. Making it a separate tool does not seem like a great idea as it would need to be kept in sync with ninja file formats, so here we are.
,
Dec 11
+tikuta@, you might be interested in this as another sort of GN/Ninja thing ...
,
Dec 11
I don't know if it's this bug exactly, but my problem was moving a generated file to a subdirectory. Simplified: - a code generator creates files in out/Debug/gen/foo.h. - move that target to a subdirectory bar/BUILD.gn, and its generated file moves to out/Debug/gen/bar/foo.h. - a target has both out/Debug/gen and out/Debug/gen/bar in its include path (and needs both), and #includes "foo.h". It picks up a stale version in out/Debug/gen/foo.h, and the build fails. A clobber build would work, of course, but there's no way to mark a CL as needing a clobber (IWBN). I'm attempting to work around it for now by using an action which runs a python script to remove the old generated file.
,
Dec 11
That'd be a different class of problems, with no obvious solution other than clobber or possibly renaming the header in question. Or #include "bar/foo.h" I guess. Removing files during the build is probably fairly non-kosher. There are //build/get_landmines.py for when a clobber is really unavoidable, but I guess they should not be a first (or even second) choice.
,
Dec 14
@tsniatowski - do you know if there's a significant difference in functionality (or results) between the ninja tool you've built and what we get in check_gn_headers (as per bug 661774)? I'm wondering what it would take to fix this more broadly, and whether the check_gn_headers-based approach should be mostly sufficient or if we really need something in ninja itself.
,
Dec 14
If I'm reading things right, check_gn_headers appears to be slightly related in that it also uses ninja depfiles, but that's aboutit. Check_gn_headers is about making sure headers that live in the repository are listed in gn so gn analyze can correctly decide what to build, ninja -t missingdeps is about headers generated during the build that _are_ listed as outputs, but do not have a reliable dependency path to all usages, making the build itself flaky. There is probably overlap between check_gn_headers complaining about generated headers that are not listed as outputs of any target, and the '-g' switch of ninja -t missingdeps, but that's not the primary use of the ninja check, as it's not fully stable (gets confused by ../../out/Default paths). |
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by dpranke@chromium.org
, Oct 14 2016Labels: OS-All
Status: Available (was: Untriaged)
Summary: The chromium build is flaky around code generators (gn misses checking generated files?) (was: The chromium build is flaky around code generators)