Reland: Generate separate files for union type containers

Reland: Generate separate files for union type containers

Change from https://codereview.chromium.org/1961883002/ is that
adding a hack to accept long file names on win.

Original description:

Generating union type containers into single files (per component)
is the root cause of cyclic dependencies when we use union types
with IDL dictionaries. We should put generated containers into
separate files.

Unfortunately this CL brings a burden on developers.
See IDLUnionTypes.md for more details.

BUG=524424
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/bf70474d005be73bafa2ea45b23388904907dfe0
Cr-Commit-Position: refs/heads/master@{#393815}

[haraken, 2016-05-13 09:15:50]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-13 09:15:51]

LGTM to reland once dpranke is okay with the hack.

[Dirk Pranke, 2016-05-13 15:20:41]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-05-13 15:20:43]

https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/utilities.py:306:
os.path.abspath(os.path.join(os.getcwd(), destination_filename)))
Do we know for sure that GN, ninja, and any other tools (compilers, linkers,
etc.) that might need to access this file will actually work?

Frankly, I'm not sure that any hack that doesn't solve the problem of keeping
the filenames < 260 chars will work reliably.

[Nico, 2016-05-13 15:21:29]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-13 15:21:31]

https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/utilities.py:306:
os.path.abspath(os.path.join(os.getcwd(), destination_filename)))
On 2016/05/13 15:20:43, Dirk Pranke wrote:
> Do we know for sure that GN, ninja, and any other tools (compilers, linkers,
> etc.) that might need to access this file will actually work?
> 
> Frankly, I'm not sure that any hack that doesn't solve the problem of keeping
> the filenames < 260 chars will work reliably.

+1

[haraken, 2016-05-13 15:28:01]

On 2016/05/13 15:21:31, Nico wrote:
>
https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/scripts/utilities.py (right):
> 
>
https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/scripts/utilities.py:306:
> os.path.abspath(os.path.join(os.getcwd(), destination_filename)))
> On 2016/05/13 15:20:43, Dirk Pranke wrote:
> > Do we know for sure that GN, ninja, and any other tools (compilers, linkers,
> > etc.) that might need to access this file will actually work?
> > 
> > Frankly, I'm not sure that any hack that doesn't solve the problem of
keeping
> > the filenames < 260 chars will work reliably.
> 
> +1

FWIW, we discussed the issue for a couple of hours in Tokyo. If there is no way
to circumvent the filename-length problem, we need to take one of the following
options:

- Stop using types like AOrBOrC. It will take a couple of months to implement
this in the IDL compiler. This won't be acceptable for the intent-to-ship of
WebRTC.

- Generate a short hash for union types and use it as a filename. Developers
need to include "UnionType26.h" instead of "AOrBOrC.h".

[Nico, 2016-05-13 15:32:33]

On 2016/05/13 15:28:01, haraken wrote:
> On 2016/05/13 15:21:31, Nico wrote:
> >
>
https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/scripts/utilities.py (right):
> > 
> >
>
https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/scripts/utilities.py:306:
> > os.path.abspath(os.path.join(os.getcwd(), destination_filename)))
> > On 2016/05/13 15:20:43, Dirk Pranke wrote:
> > > Do we know for sure that GN, ninja, and any other tools (compilers,
linkers,
> > > etc.) that might need to access this file will actually work?
> > > 
> > > Frankly, I'm not sure that any hack that doesn't solve the problem of
> keeping
> > > the filenames < 260 chars will work reliably.
> > 
> > +1
> 
> FWIW, we discussed the issue for a couple of hours in Tokyo. If there is no
way
> to circumvent the filename-length problem, we need to take one of the
following
> options:
> 
> - Stop using types like AOrBOrC. It will take a couple of months to implement
> this in the IDL compiler. This won't be acceptable for the intent-to-ship of
> WebRTC.
> 
> - Generate a short hash for union types and use it as a filename. Developers
> need to include "UnionType26.h" instead of "AOrBOrC.h".

Do you need to include the hash? Long names should be rare -- there's only one
that's too long at the moment, so the idl compiler could just truncate the name
at N characters -- and if that happens to cause a collision, the IDL compiler
should be able to notice that and print an error.

The type in question here is a typedef
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
-- could we name union types after their typedef? Or is it very common that a
union type has many typedefs pointing to it?

[haraken, 2016-05-13 15:36:01]

On 2016/05/13 15:32:33, Nico wrote:
> On 2016/05/13 15:28:01, haraken wrote:
> > On 2016/05/13 15:21:31, Nico wrote:
> > >
> >
>
https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/scripts/utilities.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1975993003/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/scripts/utilities.py:306:
> > > os.path.abspath(os.path.join(os.getcwd(), destination_filename)))
> > > On 2016/05/13 15:20:43, Dirk Pranke wrote:
> > > > Do we know for sure that GN, ninja, and any other tools (compilers,
> linkers,
> > > > etc.) that might need to access this file will actually work?
> > > > 
> > > > Frankly, I'm not sure that any hack that doesn't solve the problem of
> > keeping
> > > > the filenames < 260 chars will work reliably.
> > > 
> > > +1
> > 
> > FWIW, we discussed the issue for a couple of hours in Tokyo. If there is no
> way
> > to circumvent the filename-length problem, we need to take one of the
> following
> > options:
> > 
> > - Stop using types like AOrBOrC. It will take a couple of months to
implement
> > this in the IDL compiler. This won't be acceptable for the intent-to-ship of
> > WebRTC.
> > 
> > - Generate a short hash for union types and use it as a filename. Developers
> > need to include "UnionType26.h" instead of "AOrBOrC.h".
> 
> Do you need to include the hash? Long names should be rare -- there's only one
> that's too long at the moment, so the idl compiler could just truncate the
name
> at N characters -- and if that happens to cause a collision, the IDL compiler
> should be able to notice that and print an error.
> 
> The type in question here is a typedef
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> -- could we name union types after their typedef? Or is it very common that a
> union type has many typedefs pointing to it?

Yeah, the union type : typedef is 1 : N.

Truncating the extreme case will work.

Either way, we need some bandaid here :) If you don't think this CL will work,
we'll try the truncation approach.

[Dirk Pranke, 2016-05-13 15:36:44]

On 2016/05/13 15:28:01, haraken wrote:
> > On 2016/05/13 15:20:43, Dirk Pranke wrote:
> > > Do we know for sure that GN, ninja, and any other tools (compilers,
linkers,
> > > etc.) that might need to access this file will actually work?
> > > 
> > > Frankly, I'm not sure that any hack that doesn't solve the problem of
> keeping
> > > the filenames < 260 chars will work reliably.
> > 
> > +1
> 
> FWIW, we discussed the issue for a couple of hours in Tokyo. If there is no
way
> to circumvent the filename-length problem, we need to take one of the
following
> options:
> 
> - Stop using types like AOrBOrC. It will take a couple of months to implement
> this in the IDL compiler. This won't be acceptable for the intent-to-ship of
> WebRTC.
> 
> - Generate a short hash for union types and use it as a filename. Developers
> need to include "UnionType26.h" instead of "AOrBOrC.h".

To be clear, I'm very sympathetic to your situation, but these long-path
problems are fragile, fail in weird ways, and are hard to debug. This is
why I really want to get some sort of build-time checks on path lengths
of generated files into the builds, so we're not caught by surprise.

The short hash (or any sort of hard-coded alias) seems like a  viable solution. 
It looked like this particular file was maybe only referenced in a
couple places, so updating callers shouldn't be too bad, and hopefully we
can avoid creating too many more similar types before we get a proper
solution.

I would much prefer something like this.

[bashi, 2016-05-16 05:01:37]

Used some abbreviations to shrink file names of generated code. Updated
IDLUnionTypes.md to describe how we shorten file names and added a script called
print_union_shorthands.py which parses an IDL file, finds union types, then
prints file names of union types.

[haraken, 2016-05-16 05:04:32]

https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLUnionTypes.md (right):

https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLUnionTypes.md:36: Unsigned -> U

Is all of the abbreviations needed? I'd prefer minimizing the special-casing
list.

[bashi, 2016-05-16 05:15:53]

https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/IDLUnionTypes.md (right):

https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/IDLUnionTypes.md:36: Unsigned -> U
On 2016/05/16 05:04:32, haraken wrote:
> 
> Is all of the abbreviations needed? I'd prefer minimizing the special-casing
> list.

No. Keeping Canvas, Rendering, Context, Image, Bitmap would be enough.
Though minimizing the list could cause another long file names in the future, I
don't have strong opinion here.

[haraken, 2016-05-16 05:21:56]

On 2016/05/16 05:15:53, bashi1 wrote:
>
https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/IDLUnionTypes.md (right):
> 
>
https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/IDLUnionTypes.md:36: Unsigned -> U
> On 2016/05/16 05:04:32, haraken wrote:
> > 
> > Is all of the abbreviations needed? I'd prefer minimizing the special-casing
> > list.
> 
> No. Keeping Canvas, Rendering, Context, Image, Bitmap would be enough.
> Though minimizing the list could cause another long file names in the future,
I
> don't have strong opinion here.

Another option would be to define a special abbreviation only for
CanvasRenderingContext2DOrWebGLRenderingContextOrWebGL2RenderingContextOrImageBitmapRenderingContext
(This is the only case that causes the too long filename, right?). Then we can
add an assert to the IDL compiler that a union type must not exceed X characters
-- adding a comment to ask the binding team if you need it.

[bashi, 2016-05-16 05:24:20]

On 2016/05/16 05:21:56, haraken wrote:
> On 2016/05/16 05:15:53, bashi1 wrote:
> >
>
https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/IDLUnionTypes.md (right):
> > 
> >
>
https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/IDLUnionTypes.md:36: Unsigned -> U
> > On 2016/05/16 05:04:32, haraken wrote:
> > > 
> > > Is all of the abbreviations needed? I'd prefer minimizing the
special-casing
> > > list.
> > 
> > No. Keeping Canvas, Rendering, Context, Image, Bitmap would be enough.
> > Though minimizing the list could cause another long file names in the
future,
> I
> > don't have strong opinion here.
> 
> Another option would be to define a special abbreviation only for
>
CanvasRenderingContext2DOrWebGLRenderingContextOrWebGL2RenderingContextOrImageBitmapRenderingContext
> (This is the only case that causes the too long filename, right?). Then we can
> add an assert to the IDL compiler that a union type must not exceed X
characters
> -- adding a comment to ask the binding team if you need it.

That's what I'm doing right now. I don't know the exact limit so I won't add any
assertion.

[bashi, 2016-05-16 08:01:54]

On 2016/05/16 05:24:20, bashi1 wrote:
> On 2016/05/16 05:21:56, haraken wrote:
> > On 2016/05/16 05:15:53, bashi1 wrote:
> > >
> >
>
https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/IDLUnionTypes.md (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1975993003/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/IDLUnionTypes.md:36: Unsigned -> U
> > > On 2016/05/16 05:04:32, haraken wrote:
> > > > 
> > > > Is all of the abbreviations needed? I'd prefer minimizing the
> special-casing
> > > > list.
> > > 
> > > No. Keeping Canvas, Rendering, Context, Image, Bitmap would be enough.
> > > Though minimizing the list could cause another long file names in the
> future,
> > I
> > > don't have strong opinion here.
> > 
> > Another option would be to define a special abbreviation only for
> >
>
CanvasRenderingContext2DOrWebGLRenderingContextOrWebGL2RenderingContextOrImageBitmapRenderingContext
> > (This is the only case that causes the too long filename, right?). Then we
can
> > add an assert to the IDL compiler that a union type must not exceed X
> characters
> > -- adding a comment to ask the binding team if you need it.
> 
> That's what I'm doing right now. I don't know the exact limit so I won't add
any
> assertion.

Done.

[haraken, 2016-05-16 08:05:10]

LGTM

[bashi, 2016-05-16 09:20:49]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-05-16 09:21:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1975993003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1975993003/140001

[commit-bot: I haz the power, 2016-05-16 10:29:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-16 10:31:21]

Description was changed from

==========
Reland: Generate separate files for union type containers

Change from https://codereview.chromium.org/1961883002/ is that
adding a hack to accept long file names on win.

Original description:

Generating union type containers into single files (per component)
is the root cause of cyclic dependencies when we use union types
with IDL dictionaries. We should put generated containers into
separate files.

Unfortunately this CL brings a burden on developers.
See IDLUnionTypes.md for more details.

BUG=524424

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Reland: Generate separate files for union type containers

Change from https://codereview.chromium.org/1961883002/ is that
adding a hack to accept long file names on win.

Original description:

Generating union type containers into single files (per component)
is the root cause of cyclic dependencies when we use union types
with IDL dictionaries. We should put generated containers into
separate files.

Unfortunately this CL brings a burden on developers.
See IDLUnionTypes.md for more details.

BUG=524424

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/bf70474d005be73bafa2ea45b23388904907dfe0
Cr-Commit-Position: refs/heads/master@{#393815}
==========

[commit-bot: I haz the power, 2016-05-16 10:31:24]

Patchset 8 (id:??) landed as
https://crrev.com/bf70474d005be73bafa2ea45b23388904907dfe0
Cr-Commit-Position: refs/heads/master@{#393815}

[Dirk Pranke, 2016-05-17 00:16:49]

The change in the idl compiler lgtm, to the extent I understand it. I didn't
really look at the rest of the CL. Let me know if you wanted me to look at
something specifically.

https://codereview.chromium.org/1975993003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/1975993003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/scripts/utilities.py:407: return name
nit: this could just be `return aliases.get(name, name)`.