Make EventInterfaces.in build step not depend on bindings generation

Make EventInterfaces.in build step not depend on bindings generation

Currently EventInterfaces.in generation is causing build problems
(fail to rebuild) because it does not specify dependencies correctly,
and it is not depended on correctly.
Fixing this is hard because of wonky circular dependency between core/
and bindings/.

This fixes it by moving EventInterfaces.in generation
to a separate file, core_bindings_generated.gyp,
and having a static list of event interfaces,
so EventInterfaces.in generation does not depend on any
other build steps, breaking the cycle.

The build action stays in bindings/,
because it depends on .idl files in modules/,
and we don't want build dependencies of core/ on modules/.

This also does some cleanup of generated_bindings.gyp
and bindings.gypi,
which incidentally moves
ServiceWorkerGlobalScopeConstructors.idl
into gen/blink (where it belongs), not gen/.

(For ease of reviewing it doesn't move the Python file,
but I can move it to build/scripts in a followup.)


== Details ==
The simple fix for EventInterfaces.in generation (adding dependencies)
creates a circular dependency between .gyp files, which causes Mac build bot
to fail. (This isn't a circular dependency between GYP *targets*, so builds
correctly on other platforms),
Breaking the circular dependency by omitting it breaks Android build.

The specific dependency problem is that
core/core_generated.gyp:make_core_generated
...needs to depend on generation of EventInterfaces.in,
...which is currently in
bindings/generated_bindings.gyp:generated_bindings
but this depends on
core/core_generated.gyp:generated_testing_idls
(for generated IDL files),
which creates a cycle of *files*:
core/core_generated.gyp ->
bindings/generated_bindings.gyp ->
core/core_generated.gyp

We break this by having
core/core_generated.gyp:make_core_generated
depend on
bindings/core_bindings_generated.gyp:core_bindings_generated
...which has no dependency targets.

The dependency on bindings/ is because current EventInterfaces.in
generation dynamically computes the list of event interfaces (based on
ancestry: inherits from Event), which depends on the global
'compute_interfaces_info' step in bindings/, which computes ancestry and
a whole lot more.

This can be solved by instead having a static list of event interface IDL
files, which ｂreaks the dependency,
and also avoids unnecessary regenerations (only regen on event
.idl file changes, not any .idl file change) and simplifies the code.


Previous build failure/proposed fix:
Add RuntimeEnabled info to InterfaceInfo.pickle.
https://codereview.chromium.org/208953004/
Quick fix:
Fix dependencies for EventInterfaces.in
https://codereview.chromium.org/209723002/
...reverted in:
Revert of Fix dependencies for EventInterfaces.in
https://codereview.chromium.org/212713002/

R=abarth, haraken
BUG=341748
BUG=358074
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170628

[Nils Barth (inactive), 2014-03-27 06:43:51]

Hi Adam and haraken,
Per description, EventInterfaces.in is causing build problems,
and simple 1-line fixes die with dependency problems
(due to circular core/bindings dependency).

This fixes it by breaking the circle and moving to core/.

WDYT?

https://codereview.chromium.org/212983010/diff/1/Source/bindings/bindings.gypi
File Source/bindings/bindings.gypi (right):

https://codereview.chromium.org/212983010/diff/1/Source/bindings/bindings.gyp...
Source/bindings/bindings.gypi:286: 'write_file_only_if_changed':
'--write-file-only-if-changed 1',
Needed by core.gyp, so move into .gypi, which is where it belongs anyway.

https://codereview.chromium.org/212983010/diff/1/Source/bindings/generated_bi...
File Source/bindings/generated_bindings.gyp (left):

https://codereview.chromium.org/212983010/diff/1/Source/bindings/generated_bi...
Source/bindings/generated_bindings.gyp:440: 'event_interfaces',
! Now 'generated_bindings' actually only generates bindings!

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.gyp
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.g...
Source/core/core_generated.gyp:35: '../modules/modules.gypi',  # for list of
event IDL files
I suspect we don't want core/ to depend on modules/,
but this is making the dependency explicit.
(Currently depends on modules/ through bindings/.)

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.g...
Source/core/core_generated.gyp:437:
'../bindings/scripts/generate_event_interfaces.py',
Will move to build/scripts in followup.

[haraken, 2014-03-27 07:08:59]

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.gyp
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.g...
Source/core/core_generated.gyp:35: '../modules/modules.gypi',  # for list of
event IDL files
On 2014/03/27 06:43:51, Nils Barth wrote:
> I suspect we don't want core/ to depend on modules/,
> but this is making the dependency explicit.
> (Currently depends on modules/ through bindings/.)

Yeah, I don't think it's a right thing to make core/ depend on modules/. Would
you elaborate on why you need the dependency?

[Nils Barth (inactive), 2014-03-27 07:14:12]

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.gyp
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/212983010/diff/1/Source/core/core_generated.g...
Source/core/core_generated.gyp:35: '../modules/modules.gypi',  # for list of
event IDL files
On 2014/03/27 07:08:59, haraken wrote:
> Yeah, I don't think it's a right thing to make core/ depend on modules/. Would
> you elaborate on why you need the dependency?

core/ already depends on bindings/ that depends on modules/;
this isn't adding a new dependency, but
just makes a transitive dependency direct.

The depedency is needed b/c the generated file Event.cpp (below) depends on the
event
.idl files in both core/ and modules/.
Perhaps bindings/ and this file (and others?) should be moved
elsewhere.

[abarth-chromium, 2014-03-27 20:44:00]

On 2014/03/27 07:14:12, Nils Barth wrote:
> core/ already depends on bindings/ that depends on modules/;
> this isn't adding a new dependency, but
> just makes a transitive dependency direct.

That's something we'd eventually like to fix.  The context is that we'd
eventually like to link a blink_modules.dll that contains the modules as a
separate binary from blink_core.dll.  Today, we can't do that because of the
circular dependency you mentioned.  I believe the solution is to split bindings
up into two separate targets: one that blink_core depends on and a second one
that can depend on blink_modules.  That's a lot of work, and it's going to be
some time before we make that a reality, but we don't want to add dependencies
from core to modules because that will make it harder rather than easier.

[Nils Barth (inactive), 2014-03-28 11:38:07]

On 2014/03/27 20:44:00, abarth wrote:
> On 2014/03/27 07:14:12, Nils Barth wrote:
> > core/ already depends on bindings/ that depends on modules/;
> > this isn't adding a new dependency, but
> > just makes a transitive dependency direct.
> 
> That's something we'd eventually like to fix.  The context is that we'd
> eventually like to link a blink_modules.dll that contains the modules as a
> separate binary from blink_core.dll.  Today, we can't do that because of the
> circular dependency you mentioned.  I believe the solution is to split
bindings
> up into two separate targets: one that blink_core depends on and a second one
> that can depend on blink_modules.  That's a lot of work, and it's going to be
> some time before we make that a reality, but we don't want to add dependencies
> from core to modules because that will make it harder rather than easier.

Got it:
* core -> modules dependency is a known and hard problem
* we're having these dependencies go through bindings (bindings themselves and
events interfaces)
** ...and will eventually split it there
* we don't want to introduce direct dependencies between core and modules

I'll rework this to stay in bindings -- the dependency fix does improve things
(removes a dependency on a build step), and will make things easier in future,
but don't need to solve everything now.

Question:
* would you like me to do anything of this splitting now?

It would be easy to split the bindings themselves into two targets:
* core bindings
* module bindings
(two separate aggregate bindings targets)

There would still be the dependency on all .idl files for the global context,
but this make the problem smaller and hopefully get us closer;
we might even be able to mostly fix this by computing core context separately
from core+modules context.

[Nils Barth (inactive), 2014-03-28 11:58:30]

Just chatted with haraken about this, and recalled your Feb 7 email about this:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/A8QOwy6coqA/JWkh-g-h...
...so understand the complexity a bit more.

[haraken, 2014-03-28 12:00:17]

On 2014/03/28 11:58:30, Nils Barth wrote:
> Just chatted with haraken about this, and recalled your Feb 7 email about
this:
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/A8QOwy6coqA/JWkh-g-h...
> ...so understand the complexity a bit more.

But we Tokyo (nbarth@, tasak@ and haraken@) are very happy to take the work if
no one is yet working on :)

[abarth-chromium, 2014-03-28 17:33:21]

On 2014/03/28 12:00:17, haraken wrote:
> On 2014/03/28 11:58:30, Nils Barth wrote:
> > Just chatted with haraken about this, and recalled your Feb 7 email about
> this:
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/A8QOwy6coqA/JWkh-g-h...
> > ...so understand the complexity a bit more.
> 
> But we Tokyo (nbarth@, tasak@ and haraken@) are very happy to take the work if
> no one is yet working on :)

Yeah, go for it!  We won't actually be able to break up the DLL until we solve
the dependency issues with Source/web, but we can make progress splitting the
bindings using DEPS.  Roughly speaking, the classes like ScriptValue,
Dictionary, and ScriptController that blink_core uses to talk to V8 should go in
the new bindings module inside core.  Everything in
http://src.chromium.org/viewvc/blink/trunk/Source/bindings/v8/custom/ should
stay in the current bindings module ("above core").  I'm not quite sure where
all the rest of the classes go.  :)

[acolwell GONE FROM CHROMIUM, 2014-03-28 20:06:10]

If this results in a new CL being created, could you please be sure to CC me on
it. https://codereview.chromium.org/205583009/ can't land cleanly without
something that fixes Event.cpp generation so I'd appreciate being kept in the
loop. Thanks.

[haraken, 2014-03-29 01:53:30]

On 2014/03/28 17:33:21, abarth wrote:
> On 2014/03/28 12:00:17, haraken wrote:
> > On 2014/03/28 11:58:30, Nils Barth wrote:
> > > Just chatted with haraken about this, and recalled your Feb 7 email about
> > this:
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/A8QOwy6coqA/JWkh-g-h...
> > > ...so understand the complexity a bit more.
> > 
> > But we Tokyo (nbarth@, tasak@ and haraken@) are very happy to take the work
if
> > no one is yet working on :)
> 
> Yeah, go for it!  We won't actually be able to break up the DLL until we solve
> the dependency issues with Source/web, but we can make progress splitting the
> bindings using DEPS.  Roughly speaking, the classes like ScriptValue,
> Dictionary, and ScriptController that blink_core uses to talk to V8 should go
in
> the new bindings module inside core.  Everything in
> http://src.chromium.org/viewvc/blink/trunk/Source/bindings/v8/custom/ should
> stay in the current bindings module ("above core").  I'm not quite sure where
> all the rest of the classes go.  :)

Thanks, we'll write a design document shortly.

[Nils Barth (inactive), 2014-03-31 05:26:41]

On 2014/03/28 20:06:10, acolwell wrote:
> If this results in a new CL being created, could you please be sure to CC me
on
> it. https://codereview.chromium.org/205583009/ can't land cleanly without
> something that fixes Event.cpp generation so I'd appreciate being kept in the
> loop. Thanks.

Got it;
I'll fix it in this CL.
Will update shortly.

[Nils Barth (inactive), 2014-04-01 06:20:38]

Revised; PTAL.

This now keeps it in bindings/
and just fixes the dependencies
(adds 2 missing dependencies, removes 1 resulting cycle).

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/bindings...
File Source/bindings/bindings.gypi (right):

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/bindings...
Source/bindings/bindings.gypi:3: 'bindings_v8_dir': 'v8',
Only actually used for bindings/v8, so be explicit,
and using a one-element list for a string is confusing and error-prone.

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/generate...
File Source/bindings/generated_bindings.gyp (left):

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/generate...
Source/bindings/generated_bindings.gyp:39: '../build/win/precompile.gypi',
These includes are unnecessary, and note that scripts.gypi is duplicated!

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/generate...
Source/bindings/generated_bindings.gyp:438: 'event_interfaces',
Now 'generated_bindings' is actually just bindings!

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/generate...
File Source/bindings/generated_bindings.gyp (right):

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/generate...
Source/bindings/generated_bindings.gyp:193: '--write-file-only-if-changed',
List context expansion is confusing here;
better to be explicit about the flag.

https://codereview.chromium.org/212983010/diff/80001/Source/bindings/generate...
Source/bindings/generated_bindings.gyp:205:
'<(blink_output_dir)/ServiceWorkerGlobalScopeConstructors.idl',
Notice the error in ServiceWorker?
(Missed blink/ dir.)
Using a variable for this avoids this possible error.

[haraken, 2014-04-01 07:50:07]

LGTM.

Looks like this CL doesn't add any problematic dependency.

[Nils Barth (inactive), 2014-04-01 08:00:29]

On 2014/04/01 07:50:07, haraken wrote:
> LGTM.
> 
> Looks like this CL doesn't add any problematic dependency.

Thanks! (Yup, and in fact reduces them.)
Adam, look ok?

[abarth-chromium, 2014-04-01 16:49:47]

lgtm

[Nils Barth (inactive), 2014-04-02 00:53:07]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:53:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/212983010/80001

[commit-bot: I haz the power, 2014-04-02 01:10:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 01:10:26]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Nils Barth (inactive), 2014-04-02 01:47:29]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-04-02 01:47:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/212983010/100001

[Nils Barth (inactive), 2014-04-02 01:47:42]

The CQ bit was unchecked by nbarth@chromium.org

[Nils Barth (inactive), 2014-04-02 01:51:53]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-04-02 01:51:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/212983010/100001

[commit-bot: I haz the power, 2014-04-02 02:57:05]

Change committed as 170628