RESOLVED INVALID 84396
Wrap create_hash_tables in a Python Script for GYP Integration
https://bugs.webkit.org/show_bug.cgi?id=84396
Summary Wrap create_hash_tables in a Python Script for GYP Integration
Don Olmstead
Reported 2012-04-19 16:07:19 PDT
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.
Attachments
Patch (5.00 KB, patch)
2012-04-19 16:37 PDT, Don Olmstead
tony: review-
Don Olmstead
Comment 1 2012-04-19 16:37:08 PDT
Eric Seidel (no email)
Comment 2 2012-04-23 15:43:55 PDT
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 3 2012-04-23 16:02:00 PDT
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 4 2012-04-23 16:13:33 PDT
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 5 2012-04-23 16:16:06 PDT
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 6 2012-04-23 16:17:48 PDT
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 7 2012-04-27 10:00:44 PDT
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 8 2012-04-27 10:02:10 PDT
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)
Comment 9 2012-08-22 15:27:55 PDT
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 10 2012-08-22 15:52:17 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.