Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
744203002:
GYP: fix compile flake for second-hand use of generated headers. (Closed)
|
Created:
6 years, 1 month ago by Nick Bray (chromium) Modified:
6 years ago Reviewers:
Nico CC:
mojo-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
git@github.com:domokit/mojo.git@master Project:
mojo Visibility:
Public. |
DescriptionGYP: fix compile flake for second-hand use of generated headers.
BUG=https://code.google.com/p/chromium/issues/detail?id=435144
R=thakis@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/93289d2600dbf0fb7a11e0c96a007a293a1bfe52
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update #
Total comments: 1
Patch Set 3 : Edits #
Messages
Total messages: 21 (5 generated)
ncbray@chromium.org changed reviewers: + jamesr@chromium.org
Try jobs here: https://codereview.chromium.org/745823002/ Here's the Ninja diff for the flaky build in question: (It looks reasonable, but I am not an expert.) +build obj/content/content_common_mojo_bindings.actions_depends.stamp: stamp $ + obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp $ + obj/mojo/public/libmojo_application_bindings.a + +build obj/content/content_common_mojo_bindings.compile_depends.stamp: stamp $ + obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp $ + obj/mojo/public/mojo_application_bindings_mojom.actions_rules_copies.stamp + defines = -DCONTENT_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS $ -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=218707 $ -DCOMPONENT_BUILD -DTOOLKIT_VIEWS=1 -DUI_COMPOSITOR_IMAGE_TRANSPORT $ @@ -38,24 +46,23 @@ build $ obj/content/gen/content/common/content_common_mojo_bindings.geolocation_service.mojom.o: $ cxx gen/content/common/geolocation_service.mojom.cc || $ - obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp + obj/content/content_common_mojo_bindings.compile_depends.stamp build $ obj/content/gen/content/common/content_common_mojo_bindings.permission_service.mojom.o: $ cxx gen/content/common/permission_service.mojom.cc || $ - obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp + obj/content/content_common_mojo_bindings.compile_depends.stamp build $ obj/content/gen/content/common/content_common_mojo_bindings.render_frame_setup.mojom.o: $ cxx gen/content/common/render_frame_setup.mojom.cc || $ - obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp + obj/content/content_common_mojo_bindings.compile_depends.stamp build $ obj/content/gen/content/public/common/content_common_mojo_bindings.mojo_geoposition.mojom.o: $ cxx gen/content/public/common/mojo_geoposition.mojom.cc || $ - obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp + obj/content/content_common_mojo_bindings.compile_depends.stamp build obj/content/libcontent_common_mojo_bindings.a: alink_thin $ obj/content/gen/content/common/content_common_mojo_bindings.geolocation_service.mojom.o $ obj/content/gen/content/common/content_common_mojo_bindings.permission_service.mojom.o $ obj/content/gen/content/common/content_common_mojo_bindings.render_frame_setup.mojom.o $ obj/content/gen/content/public/common/content_common_mojo_bindings.mojo_geoposition.mojom.o $ - || $ - obj/content/content_common_mojo_bindings_mojom.actions_rules_copies.stamp + || obj/content/content_common_mojo_bindings.actions_depends.stamp
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (left): https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:68: 'hard_dependency': 1, Don't you need both this too? https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:57: 'direct_dependent_settings': { Having a direct_dependent_settings child in a direct_depend_setting section looks wrong to me. Either children should have to re-export stuff ( cf https://code.google.com/p/chromium/codesearch#search/&q=export_dependent&sq=p... ) or this should be all_dependent_settings.
https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:57: 'direct_dependent_settings': { On 2014/11/20 21:33:28, Nico wrote: > Having a direct_dependent_settings child in a direct_depend_setting section > looks wrong to me. Either children should have to re-export stuff ( cf > https://code.google.com/p/chromium/codesearch#search/&q=export_dependent&sq=p... > ) or this should be all_dependent_settings. all_dependent_settings seems too extreme (that means that the generator has to run before any targets downstream can even start, even ones that have nothing to do with the mojom.h files). export_dependent_settings should work for this but I got piles of duplicate symbol errors in various content/ targets no matter what I tried.
https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:57: 'direct_dependent_settings': { On 2014/11/20 21:41:31, jamesr wrote: > On 2014/11/20 21:33:28, Nico wrote: > > Having a direct_dependent_settings child in a direct_depend_setting section > > looks wrong to me. Either children should have to re-export stuff ( cf > > > https://code.google.com/p/chromium/codesearch#search/&q=export_dependent&sq=p... > > ) or this should be all_dependent_settings. > > all_dependent_settings seems too extreme (that means that the generator has to > run before any targets downstream can even start, even ones that have nothing to > do with the mojom.h files). export_dependent_settings should work for this but > I got piles of duplicate symbol errors in various content/ targets no matter > what I tried. Try harder? As is, this works fine for 2 levels of deps and then fails silently, with brokenness like in the linked bug.
On 2014/11/20 21:45:15, Nico wrote: > https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... > File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): > > https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... > mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:57: > 'direct_dependent_settings': { > On 2014/11/20 21:41:31, jamesr wrote: > > On 2014/11/20 21:33:28, Nico wrote: > > > Having a direct_dependent_settings child in a direct_depend_setting section > > > looks wrong to me. Either children should have to re-export stuff ( cf > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=export_dependent&sq=p... > > > ) or this should be all_dependent_settings. > > > > all_dependent_settings seems too extreme (that means that the generator has to > > run before any targets downstream can even start, even ones that have nothing > to > > do with the mojom.h files). export_dependent_settings should work for this > but > > I got piles of duplicate symbol errors in various content/ targets no matter > > what I tried. > > Try harder? As is, this works fine for 2 levels of deps and then fails silently, > with brokenness like in the linked bug. That's up to Nick. My vote is to drop this completely.
https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (left): https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:68: 'hard_dependency': 1, On 2014/11/20 21:33:28, Nico wrote: > Don't you need both this too? I believe hard_dependency only affects dependencies between libraries. This line would set the flag on an action. Ninja produces the same output with and without this line. It appears to have no effect, at least for the Ninja generator. I could leave it in with heady commenting if there is a possibility other generators might behave differently. https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:57: 'direct_dependent_settings': { On 2014/11/20 21:33:28, Nico wrote: > Having a direct_dependent_settings child in a direct_depend_setting section > looks wrong to me. Either children should have to re-export stuff ( cf > https://code.google.com/p/chromium/codesearch#search/&q=export_dependent&sq=p... > ) or this should be all_dependent_settings. all_dependent_settings was deemed too extreme on code review. I was unaware of export_dependent_settings. I don't think it would work because we only want to inject the sources and hard_dependency one level deep, but the include directories two levels deep. I don't think it's particularly pretty, but I don't see a better way to do this without being explicit and redundant on a non-trivial number of targets. Thoughts? Also, I am explicitly _not_ supporting n-th level transitive dependencies. If you use header files for the library, you must explicitly depend on the library. But this has always been the case for mojo's libraries.
On 2014/11/20 21:48:55, Nick Bray (chromium) wrote: > https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... > File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (left): > > https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... > mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:68: > 'hard_dependency': 1, > On 2014/11/20 21:33:28, Nico wrote: > > Don't you need both this too? > > I believe hard_dependency only affects dependencies between libraries. This > line would set the flag on an action. Ninja produces the same output with and > without this line. It appears to have no effect, at least for the Ninja > generator. I could leave it in with heady commenting if there is a possibility > other generators might behave differently. > > https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... > File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): > > https://codereview.chromium.org/744203002/diff/1/mojo/public/tools/bindings/m... > mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:57: > 'direct_dependent_settings': { > On 2014/11/20 21:33:28, Nico wrote: > > Having a direct_dependent_settings child in a direct_depend_setting section > > looks wrong to me. Either children should have to re-export stuff ( cf > > > https://code.google.com/p/chromium/codesearch#search/&q=export_dependent&sq=p... > > ) or this should be all_dependent_settings. > > all_dependent_settings was deemed too extreme on code review. > I was unaware of export_dependent_settings. I don't think it would work because > we only want to inject the sources and hard_dependency one level deep, but the > include directories two levels deep. If you want the includes to be two levels deep, you want the hard_dependency two deep too, no? (because generated headers) What's generated_srcs injected for? > I don't think it's particularly pretty, but I don't see a better way to do this > without being explicit and redundant on a non-trivial number of targets. > Thoughts? > > > Also, I am explicitly _not_ supporting n-th level transitive dependencies. If > you use header files for the library, you must explicitly depend on the library. > But this has always been the case for mojo's libraries.
> Try harder? As is, this works fine for 2 levels of deps and then fails silently, > with brokenness like in the linked bug. The problem that you are citing is fundamental to the interaction of GYP with code generation, not this particular implementation of it. The only way to avoid the potential for brokenness would be to mark all transitive dependencies as hard dependencies - which is rather extreme. Apparently GN avoids the lib => lib => action problem that GYP has, it will perform the action before compiling either of the libs. I spent quite a bit of time trying to find the best way to do this in GYP with Brad. > That's up to Nick. My vote is to drop this completely. As eluded to in the bug, this would make a number of important things impossible. Dropping this is not an option, notwithstanding a complete re-assessment of priorities. We can dive deeper into the background if you want to spend the time. > If you want the includes to be two levels deep, you want the hard_dependency two > deep too, no? (because generated headers) Nope. Assume libA => libB => generator action. Marking libB has hard_dependancy means libB will be build before libA. The generator will always be run before building libB. This needs to occur because libA depending on libB means libA will need to use the headers to access libB's functionality. Strictly speaking, libA does not actually care about libB being built, but this is the cleanest / simplest way to ensure the generator runs before libA is built. So: libA needs the include directory, but marking it as a hard dependency would be wrong. It's the relationship between libA and libB that needs to be ordered, not between libA and everything that depends on libA. > What's generated_srcs injected for? generated_src_dirs? Android. This preserves the behavior of the original script, which I eventually hope to remove: https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/... I am sure you're curious about why the original script can't be used... but we've already wandered into reopening a design review inside of a bugfix (and delaying said bugfix). I will be happy to explain the entire reasoning to you in person or over VC, but doing a design re-review inside a CL is really not the purpose of a CL.
I understand the desire to use mojom in nacl targets in the near future. Currently, the situation is that the gyp build in chromium is broken. I don't know how to make it work with this pattern after having spent some time on it, and I know you've spent even more time on it. I think in order to get the gyp build working again in chromium we should go back to the earlier version which AFAIK did work and then we can discuss how to add new functionality (nacl generation) to the build in a way that doesn't break anything. Having the existing functionality (ability to compile chromium with gyp) is more important than having future abilities (ability to compile chromium with gyp with nacl). Having the future capabilities is nice, but it's unclear exactly how to make that happen. Now that we know the _explicit.gypi pattern doesn't work for existing functionality, I think we need to back it out to get the chromium gyp build working again.
If we need to all_dependent_settings { hard_dependency: 1 } and that's the only
way that gyp will ever be able to handle code generation like this, I guess we
should do that. That'll make builds a lot slower. The question then is if the
hit in build time for all chromium (gyp) builds is more valuable than the
capability of using gyp+nacl+mojo. I don't feel qualified to make that call.
PTAL. Talked with Nico. Added some opportunistic cleanups after talking with Colin. Trys are green: https://codereview.chromium.org/745823002/ Although I had to peel off a fix for a bug that Nico noticed: https://codereview.chromium.org/747253002/ Because of a bug in GYP's android generator: http://code.google.com/p/chromium/issues/detail?id=435772
jamesr@chromium.org changed reviewers: - jamesr@chromium.org
OK, I think Nico should review this.
BTW, I have a growing suspicion that everywhere export dependent settings is used on a generated library in the Mojo GYP build is wrong, because of the lack of a hard dependency.
lgtm Nick explained to me how this file works (a version of this in a comment at the top of the file would maybe be helpful): One includes this gypi in an empty target that runs an action generating cc files, and then depends on that empty target from a static_library target that then builds the source files. Dependencies of that static_library target then need to see include paths. The includes should never be visible more than 2 hops away from the source generating target. This makes sense to me. https://codereview.chromium.org/744203002/diff/20001/mojo/public/tools/bindin... File mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi (right): https://codereview.chromium.org/744203002/diff/20001/mojo/public/tools/bindin... mojo/public/tools/bindings/mojom_bindings_generator_explicit.gypi:11: '<!(python <(DEPTH)/build/inverse_depth.py <(DEPTH))', (nit: i'd dedented this one instead of indenting 2 lines further down)
The CQ bit was checked by thakis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 93289d2600dbf0fb7a11e0c96a007a293a1bfe52. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
