Auto-generate GNI entries for .h/.cpp files generated for IDL dictionaries |
||||||||
Issue descriptionWhen 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.
,
May 29 2017
bashi, do you have plans to start working on this soon? I can take a look if you don't.
,
May 29 2017
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.
,
May 29 2017
,
May 29 2017
,
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
,
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
,
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
,
Jun 1 2017
,
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
,
Jun 2 2017
Confirmed that reverting r476243 allows the Chromium Win x64 PGO builder to successfully compile again.
,
Jun 2 2017
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...
,
Jun 2 2017
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
,
Jun 2 2017
Ahh, our good friend Windows :-)
,
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
,
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
,
Jun 7 2017
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 :/
,
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
,
Jun 7 2017
Let's see if the change sticks this time. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bashi@chromium.org
, May 24 2017Status: Assigned (was: Untriaged)