Yep, doesn't look like we depend on it.  Should be in here: https://chromium.googlesource.com/chromium/src/+/d47e2e3c448b8ef0df05bc42597c7c456a8ef88a/chrome/browser/extensions/BUILD.gn#794.  I can take this, if all we need to do is add a dep.

bratell@, how were you trying to build, so I can verify a fix?  (And any particular platform?)

It was a debug component build with jumbo enabled of chrome/browser/extensions without building chrome before it (but the tree was not clean so I don't know exactly what it had to rebuild and what it did not rebuild).

ninja -C out/Default chrome/browser/extensions

Cool, thanks bratell@.

I was able to repro this with the following build args, and a clean build:
is_component_build = true
is_debug = true
use_jumbo_build = true

ninja -C <out_dir> chrome/browser/extensions

There was also another similar error:

In file included from gen/chrome/browser/extensions/extensions_jumbo_7.cc:7:
In file included from ./../../chrome/browser/extensions/extension_util.cc:15:
In file included from ../../chrome/browser/banners/app_banner_manager.h:15:
In file included from ../../chrome/browser/engagement/site_engagement_observer.h:10:
../../chrome/browser/engagement/site_engagement_service.h:18:10: fatal error: 'chrome/browser/engagement/site_engagement_details.mojom.h' file not found
#include "chrome/browser/engagement/site_engagement_details.mojom.h"

(chrome/browser/extensions doesn't depend on chrome/browser/engagement.)

The problem is indeed that there isn't a proper dependency.  Unfortunately, it's not quite as simple as just adding a missing line to c/b/extensions/BUILD.gn with c/b/resource_coordinator, because there isn't a separate resource_coordinator target.  c/b/resource_coordinator sources are all defined in the monolithic c/b/BUILD.gn under the `browser` target.

It seems like the right thing to do here would be to add a separate target for the c/b/resource_coordinator sources in c/b/resource_coordinator/BUILD.gn, and then depend on that with c/b/extensions.  But I'd like to make sure that approach is something desirable at a higher level.  We never really intend to build chrome/browser/extensions as an entirely separate target (i.e., it inherently depends on other parts of chrome/browser), so does it make sense to support doing so?

From a code cleanliness and dependency level, I rather like having specific BUILD.gn files, but I'm not sure if there's a reason we haven't split up c/b/BUILD.gn more already.

+dpranke@, as resident expert on gn, any opinions here?  +thakis@, IIRC, you've done some jumbo work as well - any opinions?

Can we just pull the mojom files into their own target and depend on that?

(I'm not doing any jumbo work.)

> Can we just pull the mojom files into their own target and depend on that?
I believe they already are, so yes, this seems to work - though I admit I'm not entirely sure why.  Extensions code is relying on more than just the mojo bindings - it's importing (and using) c/b/resource_coordinator/tab_manager.h.  Without depending on more than the mojo bindings, wouldn't linking fail for undefined symbols for the TabManager methods?  (In practice, this seems not to be the case - do we not link when building specific directories in jumbo builds?)

[1] https://chromium.googlesource.com/chromium/src/+/2fa606297281c7b931d27565f6ef40fe6337c2c2/chrome/browser/resource_coordinator/BUILD.gn#23

We need dependencies on targets for:

1. Generated header files. These dependencies ensure the generated header exists before a file gets compiled including it.

2. Linking.


For 1, these are always needed, and omitting them causes flaky builds. For 2, if libs A and B depend each other, but are only used by executable E that depends on both A and B, then A and B don't need a dep. Omitting it is a bit hacky, but causes no issues and enables splitting a huge monolithic target into two (this is important if we run into .lib file size issues or whatnot -- less a problem nowadays, but it was a problem when we moved c/b/e into its own target without actually making it an independent target).


This bug is about fixing (1).

Got it.  Thanks for the explanation, thakis@!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c517449b36ff4543f2841089203daf3a46594c5

commit 8c517449b36ff4543f2841089203daf3a46594c5
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Mon Jul 02 20:01:41 2018

[Extensions] Add missing build deps

Extensions relies upon the mojo bindings for resource_coordinator and
egagement. Add these explicitly to the BUILD.gn file.

Bug:  857027 
Change-Id: I2e6a8c99ec6eda77a31c3747ba66d807809ae8f4
Reviewed-on: https://chromium-review.googlesource.com/1122972
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571980}
[modify] https://crrev.com/8c517449b36ff4543f2841089203daf3a46594c5/chrome/browser/extensions/BUILD.gn

Should be fixed.  Thanks for filing, bratell@!