Bug 84396 – Wrap create_hash_tables in a Python Script for GYP Integration

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Don Olmstead]

GYP actions work on multiple inputs and outputs while create_hash_tables only works on a single input and output. A wrapper around create_hash_tables can be used to act on multiple files, thus making the GYP action more succinct.

[Don Olmstead]

Created attachment 138006 [details]
Patch

[Eric Seidel (no email)]

I thought that we already had the mac port building JavaScriptCore (using JavaScirptCore/JavaScriptCore.gyp).  I'm surprised this script doesn't already exist somewhere?  Maybe it got deleted a while ago due to lack ofuse?

[Eric Seidel (no email)]

Comment on attachment 138006 [details]
Patch

I'm confused why we need to wrap again?  Does gyp not play nice with perl?

[Eric Seidel (no email)]

Comment on attachment 138006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138006&action=review

> Source/JavaScriptCore/JavaScriptCore.gyp/scripts/action_createhashtables.py:32
> +# action_createregextables.py is a harness script to connect actions sections of
> +# gyp-based builds to create_regex_tables.

We should really explain why this script is needed, and that we don't just have GYP call perl directly.

[Eric Seidel (no email)]

Comment on attachment 138006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138006&action=review

> Source/JavaScriptCore/JavaScriptCore.gyp/scripts/action_createhashtables.py:42
> +def vararg_callback(option, opt_str, value, parser):

I'm confused why this is needed?  Why not just --input foo --output bar --input foo2 --output bar and use action="append"?

[Eric Seidel (no email)]

Comment on attachment 138006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138006&action=review

This sort of build script doesn't really need a lot of review.  It doesn't matter.  But we are adding some additional complexity here, and I'm just tryign to take pause of ra second and understand the choices involved.  we don't need to go 16 rounds trying to repaint this bikeshed.  Just making sure you and I are on somethign resembling the same page. :)

>> Source/JavaScriptCore/JavaScriptCore.gyp/scripts/action_createhashtables.py:42
>> +def vararg_callback(option, opt_str, value, parser):
> 
> I'm confused why this is needed?  Why not just --input foo --output bar --input foo2 --output bar and use action="append"?

Not suggesting this approach is wrong.  I just don't understand exactly why you chose this way.

[Tony Chang]

Comment on attachment 138006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138006&action=review

>> Source/JavaScriptCore/JavaScriptCore.gyp/scripts/action_createhashtables.py:32
>> +# gyp-based builds to create_regex_tables.
> 
> We should really explain why this script is needed, and that we don't just have GYP call perl directly.

FWIW, it should work to call perl directly from gyp as long as it is in your path.  We shouldn't need to add a wrapper script unless you want more control over your inputs and outputs (i.e., have some sort of pre/post processing you need to do on the files).

[Tony Chang]

Comment on attachment 138006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138006&action=review

> Source/JavaScriptCore/JavaScriptCore.gyp/scripts/action_createhashtables.py:88
> +    for index in range(file_count):
> +        input = input_files[index]
> +        output = output_files[index]

This should be the same as: for input, output in zip(input_files, output_files):

[Eric Seidel (no email)]

I'm unclear as to the path forward here.  Tony seemed to suggest this was not needed.  Did this work get abandoned (as my other gyp work in the past has)? :)

[Tony Chang]

Comment on attachment 138006 [details]
Patch

I'll r- for now to get this out of the review queue.  Don, if this is really necessary, please clean up the style and upload a new patch.