912197 - Move WebIDL binding definitions to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

WebIDL binding files are currently defined in dom/webidl/WebIDL.mk. We should move the definition of dom bindings to moz.build so we can easily extract the set of relevant files (for use in install manifests, etc).

This is probably low priority. Just wanted to have a bug on file in case someone wants to tackle it.

Comment 1

[(not currently active) Ted Mielczarek]

We decided we were just going to have a high-level representation in moz.build, like WEBIDL_SOURCES, and have all the logic in the backend, right?

Comment 2

[Boris Zbarsky [:bzbarsky]]

That's basically all WebIDL.mk has right now: a list of IDL files, and a second list of preprocessable IDL files.  Both possibly with #ifdefs.

Comment 3

[Gregory Szorc [:gps]]

WEBIDL_SOURCES - a list - (or equivalent) should be all that is needed in moz.build files. And, I'm even fine with the logic for "build the WebIDL files" to live in a make file forever. Maybe we move the logic from dom/bindings/Makefile to config/makefiles or something. But WebIDL generation is wonky and unless some larger build system project warrants us moving away from the existing make rules, I see no reason for us to ditch them. After all, we should be able to tell Tup "build WebIDL files by running |make -C webidl|, right?

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add classes for WebIDL source files

Little pieces are easier to review, right?  Simple classes first.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add WebIDL variables to the sandbox

Bikeshedding on the variable names welcome.  I made WEBIDL_FILES a |list|
because sorting it according to Python's rules was getting on my nerves.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 3 - emit WebIDL objects during traversal

Another simple piece.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 4 - write out lists of WebIDL source files for dom/bindings/

I kept the variable names the same in the generated makefile as the ones WebIDL.mk
uses, so that the next part wouldn't be cluttered.  Don't know if we have a policy
for this or not...

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 5 - move WebIDL.mk over to moz.build

The final piece of the puzzle.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 5 - move WebIDL.mk over to moz.build

Knew I was going to screw this up.  How about this slightly more complete patch?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 800183 [details] [diff] [review]
part 2 - add WebIDL variables to the sandbox

Review of attachment 800183 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +388,5 @@
>  
>          These are .ipdl files that will be parsed and converted to .cpp files.
>          """),
>  
> +    'WEBIDL_FILES': (list, list, [],

StrictOrderingOnAppendList for the first

Comment 11

[(no longer active)]

<https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings> needs to be updated when this lands.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 800182 [details] [diff] [review]
part 1 - add classes for WebIDL source files

Review of attachment 800182 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Although I'm curious how all the logic will hook up. We typically do single patches for adding things to the frontend to make this easier to follow.

Nit: Some style guidelines want you to use 2 lines between classes. If you install my Mercurial extension from https://hg.mozilla.org/users/gszorc_mozilla.com/hgext-gecko-dev, it will automatically apply Python style checking as you qref patches and alert you of style violations. It's even smart enough to only report on the lines your patch has changed!

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 800183 [details] [diff] [review]
part 2 - add WebIDL variables to the sandbox

Review of attachment 800183 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +402,5 @@
> +
> +    'GENERATED_WEBIDL_FILES': (StrictOrderingOnAppendList, list, [],
> +         """Generated WebIDL source files.
> +
> +         These will be generated from some other files."""),

We'll eventually need the "how they are generated" bit to be defined in moz.build. But we can punt this to a followup bug.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 800184 [details] [diff] [review]
part 3 - emit WebIDL objects during traversal

Review of attachment 800184 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +23,5 @@
>      XpcshellManifests,
> +    WebIDLFile,
> +    TestWebIDLFile,
> +    PreprocessedWebIDLFile,
> +    GeneratedWebIDLFile,

Nit: Sort.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 800186 [details] [diff] [review]
part 4 - write out lists of WebIDL source files for dom/bindings/

Review of attachment 800186 [details] [diff] [review]:
-----------------------------------------------------------------

Normally I'd ask for tests. But this is only used by a very small part of the build system and it would be pretty obvious if it stopped working. So I'm inclined to let the tests slide. Don't let me discourage you from writing them, however.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +29,5 @@
>      XpcshellManifests,
> +    WebIDLFile,
> +    TestWebIDLFile,
> +    PreprocessedWebIDLFile,
> +    GeneratedWebIDLFile,

Nit: sort

@@ +327,5 @@
> +            webidls.write('test_webidl_files += %s\n' % os.path.basename(webidl))
> +        for webidl in sorted(self._generated_webidl_sources):
> +            webidls.write('generated_webidl_files += %s\n' % os.path.basename(webidl))
> +        for webidl in sorted(self._preprocessed_webidl_sources):
> +            webidls.write('preprocessed_webidl_files += %s\n' % os.path.basename(webidl))

I'd just write these out on a single line. If nothing else, it's less work for the make parser and for variable evaluation, which profiling has shown is the bit slowing pymake down.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 800195 [details] [diff] [review]
part 5 - move WebIDL.mk over to moz.build

Review of attachment 800195 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/moz.build
@@ +71,5 @@
>      'camera',
>      'audiochannel',
>      'promise',
> +    'wappush',
> +    'webidl'

This remind me: we need a primitive for evaluating a moz.build in the context of a directory without necessarily adding it to directory traversal. I'll file a bug.

Comment 17

[Gregory Szorc [:gps]]

Something else I forgot: we'll want to add the destined-for-dist/include .h files to self._install_manifests['dist_include']. This will cause the header files to be tracked by the install manifest which will in turn prevent them from being deleted at the top of the build. You'll want to pretty much copy the code in _handle_idl_manager. Once things are in the install manifest, the webidl processing can generate the headers directly into dist/include, saving an extra file copy/install step and hopefully making the build a little faster.

We can punt this to a followup if you'd like.

Comment 18

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc86c80d01c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1272d731770c
https://hg.mozilla.org/integration/mozilla-inbound/rev/91ba20da3b79
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8391be568d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90fe3a1e259

Comment 19

[Nathan Froyd [:froydnj]]

And a follow-up to address b2g bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f52bbf813be

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc86c80d01c0
https://hg.mozilla.org/mozilla-central/rev/1272d731770c
https://hg.mozilla.org/mozilla-central/rev/91ba20da3b79
https://hg.mozilla.org/mozilla-central/rev/5b8391be568d
https://hg.mozilla.org/mozilla-central/rev/e90fe3a1e259
https://hg.mozilla.org/mozilla-central/rev/5f52bbf813be
https://hg.mozilla.org/mozilla-central/rev/db3e565b7586