New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 725996 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
OoO until Feb 4th
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Auto-generate GNI entries for .h/.cpp files generated for IDL dictionaries

Project Member Reported by jsb...@chromium.org, May 24 2017

Issue description

When adding a dictionary to IDL, it is necessary to add the dictionary .idl file to modules_dictionary_idl_files in modules_idl_files.gni - which makes sense - but then the generated .h and .cpp files need to be added to the generated_modules_dictionary_files section.

Example:

https://codereview.chromium.org/2713793004/diff/120001/third_party/WebKit/Source/modules/modules_idl_files.gni

Just like non-dictionary IDL files, the list of targets to build should be auto-generated rather than require manual maintenance. 

There's already a TODO in the GNI file explaining this:

 # TODO ideally this would not be listed explicitly. Rather, we would have
 # different categories of .idl files that produce certain patterns of	  
 # source files. Then these sources files can be programatically expanded	
 # from the .idl file list(s).

Just filing this as a tracking bug.

 

Comment 1 by bashi@chromium.org, May 24 2017

Owner: bashi@chromium.org
Status: Assigned (was: Untriaged)
bashi, do you have plans to start working on this soon? I can take a look if you don't.

Comment 3 by bashi@chromium.org, May 29 2017

Cc: peria@chromium.org
Owner: raphael....@intel.com
I really appreciate if you can take a look! Feel free to re assign to me:)

+peria@ who may know how to generate gn entries dynamically.

Comment 4 by bashi@chromium.org, May 29 2017

Cc: bashi@chromium.org
Status: Started (was: Assigned)

Comment 6 by peria@chromium.org, May 29 2017

If each .idl file will be converted into a set of .cpp/.h files,
the definition of "bindings_core_generated_interface_files" is a good reference.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/BUILD.gn?type=cs&q=bindings+process_file_template+file:%5C.gn+file:webkit&l=283-289

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 1 2017

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

commit a031ee0c27808d2dcb4d4c54ceb147b29874317d
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Thu Jun 01 10:11:45 2017

bindings: Add an intermediate target for generated module bindings.

Follow what's already done in bindings/core/v8 and add a
"bindings_modules_impl" target to bindings/modules/v8 to aggregate all the
.cpp/.h files generated by the other targets in the same BUILD.gn (the ones
processing the modules IDL files).

By declaring the target in the same GN file, we can use get_target_outputs()
to obtain some output names, which then allows us to get rid of
|bindings_modules_generated_interface_files| as well as
|bindings_modules_generated_partial_interface_files|.

This cleanup is also being done in preparation for getting rid of the
hardcoded list of generated dictionary impl files we currently maintain in
GN, as Source/modules:modules would not be able to retrieve any similar list
of generated files that it needs.

Bug:  725996 
Change-Id: Ia53de40ef394af197351f218050af6d8a10f2c3a
Reviewed-on: https://chromium-review.googlesource.com/517955
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#476243}
[modify] https://crrev.com/a031ee0c27808d2dcb4d4c54ceb147b29874317d/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/a031ee0c27808d2dcb4d4c54ceb147b29874317d/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/a031ee0c27808d2dcb4d4c54ceb147b29874317d/third_party/WebKit/Source/modules/BUILD.gn

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 1 2017

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

commit 5d0edd94b0f86092c267bc9f5fc77396a994a135
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Thu Jun 01 12:41:13 2017

bindings: Generate entries for .cpp/.h files from IDL dictionaries in GN

By auto-generating those entries, we can stop requiring people to manually
update lists such as |generated_core_dictionary_files| every time Blink's
lists of IDL dictionary files change.

Since the names of the generated .cpp/.h follow a fixed, specific format
that only depends on their respective IDL file's path and file name, we can
use GN itself to do some path introspection and generate the file names we
want in idl_impl().

While here, change the names of idl_impl()'s required arguments to make
their purpose more explicit: it is not immediately obvious that we
differentiate unions and callback functions from dictionary files in terms
of where the latter are generated and their file names, as well as why
idl_impl() only expects non-dictionary files in its outputs list. A good
next step would be generating dictionary impl files separately from unions
and callbacks to avoid the confusion altogether.

Bug:  725996 
Change-Id: I41b06e8d71f33b21d77944216fcb07d0557ac47b
Reviewed-on: https://chromium-review.googlesource.com/517795
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476262}
[modify] https://crrev.com/5d0edd94b0f86092c267bc9f5fc77396a994a135/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/5d0edd94b0f86092c267bc9f5fc77396a994a135/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/5d0edd94b0f86092c267bc9f5fc77396a994a135/third_party/WebKit/Source/bindings/scripts/scripts.gni
[modify] https://crrev.com/5d0edd94b0f86092c267bc9f5fc77396a994a135/third_party/WebKit/Source/modules/modules_idl_files.gni

Labels: M-61
Status: Fixed (was: Started)
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 2 2017

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

commit d072de9f074b21d4dda66627f9a0a27a4ac87271
Author: Peter Kasting <pkasting@chromium.org>
Date: Fri Jun 02 02:48:40 2017

Revert "bindings: Add an intermediate target for generated module bindings."

This reverts commit a031ee0c27808d2dcb4d4c54ceb147b29874317d.

Reason for revert: Possible cause of Chromium Win x64 PGO Builder compile failures; hypothesis is that filenames are exceeding the maximum path length.  See https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20x64%20PGO%20Builder/builds/19881 .

Original change's description:
> bindings: Add an intermediate target for generated module bindings.
> 
> Follow what's already done in bindings/core/v8 and add a
> "bindings_modules_impl" target to bindings/modules/v8 to aggregate all the
> .cpp/.h files generated by the other targets in the same BUILD.gn (the ones
> processing the modules IDL files).
> 
> By declaring the target in the same GN file, we can use get_target_outputs()
> to obtain some output names, which then allows us to get rid of
> |bindings_modules_generated_interface_files| as well as
> |bindings_modules_generated_partial_interface_files|.
> 
> This cleanup is also being done in preparation for getting rid of the
> hardcoded list of generated dictionary impl files we currently maintain in
> GN, as Source/modules:modules would not be able to retrieve any similar list
> of generated files that it needs.
> 
> Bug:  725996 
> Change-Id: Ia53de40ef394af197351f218050af6d8a10f2c3a
> Reviewed-on: https://chromium-review.googlesource.com/517955
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
> Cr-Commit-Position: refs/heads/master@{#476243}

TBR=peria@chromium.org,yukishiino@chromium.org,raphael.kubo.da.costa@intel.com,bashi@chromium.org,haraken@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  725996 

Change-Id: I416da9cb0234ec7a184edae527645805453a9177
Reviewed-on: https://chromium-review.googlesource.com/522226
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476532}
[modify] https://crrev.com/d072de9f074b21d4dda66627f9a0a27a4ac87271/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/d072de9f074b21d4dda66627f9a0a27a4ac87271/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/d072de9f074b21d4dda66627f9a0a27a4ac87271/third_party/WebKit/Source/modules/BUILD.gn

Status: Assigned (was: Fixed)
Confirmed that reverting r476243 allows the Chromium Win x64 PGO builder to successfully compile again.
Raphael, looks like your CL hit crbug.com/711464. Fixing the root cause is not easy but you can work around the issue by adding alias for long union names.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scripts/utilities.py?q=shorten_union_name&l=419

It may be better to lower the upper limit of the name to raise an exception to warn before submitting CLs...
FYI, this is the error output.

ninja: build stopped: .
ninja: error: WriteFile(obj/third_party/WebKit/Source/bindings/modules/v8/bindings_modules_impl/CSSImageValueOrHTMLImageElementOrSVGImageElementOrHTMLVideoElementOrHTMLCanvasElementOrImageBitmapOrOffscreenCanvas.obj.rsp): Unable to create file. No such file or directory
step returned non-zero exit code: 1
Status: Started (was: Assigned)
Ahh, our good friend Windows :-)
Project Member

Comment 15 by bugdroid1@chromium.org, Jun 2 2017

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

commit c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Fri Jun 02 14:00:22 2017

bindings: Use a smaller union name length limit (80 instead of 120).

While we do not reach a proper solution to the underlying problem of
generating union file names that are too long, make the accepted length
threshold 80 instead of 120 and convert all names longer than the new
threshold.

This should (at least for now) prevent errors such as the one reported in
https://bugs.chromium.org/p/chromium/issues/detail?id=725996#c10, where the
combination of the builder's name, union file name and target path in the
build directory went over Windows' length limit of 260 characters.

Bug: 711464,  725996 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I69d0afa9baaef1f6c5bd2103d965ac1ad8253488
Reviewed-on: https://chromium-review.googlesource.com/522685
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#476632}
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/bindings/scripts/utilities.py
[rename] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/bindings/tests/results/core/NestedUnionType.cpp
[rename] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/bindings/tests/results/core/NestedUnionType.h
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.h
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContext.cpp
[modify] https://crrev.com/c0cdd99d33f708afb6ba0a7694482b4bdffbbaeb/third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp

Project Member

Comment 16 by bugdroid1@chromium.org, Jun 2 2017

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

commit 2cf29accc26785b68a66aa11186a235e447d561f
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Fri Jun 02 14:39:03 2017

Rename bindings_modules_generated to bindings_modules_v8_generated

Small cosmetic GN change: renaming the target makes it look more similar to
its counterpart in bindings/core/v8/BUILD.gn, and reduces the confusion
around having this target as well as the one in bindings/modules/BUILD.gn
have the same name.

Bug:  725996 
Change-Id: If131e1184737a1b102f3953c7fe8cc3c7d61c34c
Reviewed-on: https://chromium-review.googlesource.com/521048
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476640}
[modify] https://crrev.com/2cf29accc26785b68a66aa11186a235e447d561f/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/2cf29accc26785b68a66aa11186a235e447d561f/third_party/WebKit/Source/core/BUILD.gn

https://chromium-review.googlesource.com/c/525514 ("bindings: Add an intermediate target for generated module bindings (reland)") was merged a few minutes ago but only referenced bug 728584 :/
Project Member

Comment 18 by bugdroid1@chromium.org, Jun 7 2017

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

commit fb587d19c5edf19494d947d6504bc67d4710c30e
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Wed Jun 07 13:32:28 2017

bindings: Generate entries for .cpp/.h files from IDL dictionaries in GN (reland)

By auto-generating those entries, we can stop requiring people to manually
update lists such as |generated_core_dictionary_files| every time Blink's
lists of IDL dictionary files change.

Since the names of the generated .cpp/.h follow a fixed, specific format
that only depends on their respective IDL file's path and file name, we can
use GN itself to do some path introspection and generate the file names we
want in idl_impl().

While here, change the names of idl_impl()'s required arguments to make
their purpose more explicit: it is not immediately obvious that we
differentiate unions and callback functions from dictionary files in terms
of where the latter are generated and their file names, as well as why
idl_impl() only expects non-dictionary files in its outputs list. A good
next step would be generating dictionary impl files separately from unions
and callbacks to avoid the confusion altogether.

Bug:  725996 
Change-Id: I63dc848b0f8b4d13568585f1ebf4e418c7218c0d
Reviewed-on: https://chromium-review.googlesource.com/527172
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#477626}
[modify] https://crrev.com/fb587d19c5edf19494d947d6504bc67d4710c30e/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/fb587d19c5edf19494d947d6504bc67d4710c30e/third_party/WebKit/Source/bindings/modules/v8/BUILD.gn
[modify] https://crrev.com/fb587d19c5edf19494d947d6504bc67d4710c30e/third_party/WebKit/Source/bindings/scripts/scripts.gni
[modify] https://crrev.com/fb587d19c5edf19494d947d6504bc67d4710c30e/third_party/WebKit/Source/modules/modules_idl_files.gni

Status: Fixed (was: Started)
Let's see if the change sticks this time.

Sign in to add a comment