Bug 109969

Summary: [GTK] Improve gyp build JavaScriptCore code generation
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: dpranke, gns, pnormand, thakis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109960    
Bug Blocks:    
Attachments:
Description Flags
Patch dpranke: review+, webkit.review.bot: commit-queue-

Description Martin Robinson 2013-02-15 13:40:53 PST
In bug https://bugs.webkit.org/show_bug.cgi?id=109003#c19 Nico suggested a way to improve the code generation for the JavaScriptCore part of the GTK+ gyp build.
Comment 1 Martin Robinson 2013-02-15 15:19:22 PST
Created attachment 188650 [details]
Patch
Comment 2 Dirk Pranke 2013-02-15 17:42:46 PST
Comment on attachment 188650 [details]
Patch

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

This looks basically okay but I'm not knowledgeable enough about the JSC build process to be sure that this is the best way to do this ...

> Source/JavaScriptCore/JavaScriptCore.gyp/JavaScriptCoreGTK.gyp:93
> +          '../runtime/StringPrototype.cpp',

If you're concerned about this list being duplicated between here and JavaScriptCore.gypi, you can define this list as a variable in JavaScriptCore.gypi and include it from both places.
Comment 3 Dirk Pranke 2013-02-15 17:43:18 PST
If no one else wants to take a look at this, I'll take another look in more detail to try and understand it better ...
Comment 4 Martin Robinson 2013-02-15 18:03:39 PST
(In reply to comment #2)
> (From update of attachment 188650 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188650&action=review
> 
> This looks basically okay but I'm not knowledgeable enough about the JSC build process to be sure that this is the best way to do this ...
> 
> > Source/JavaScriptCore/JavaScriptCore.gyp/JavaScriptCoreGTK.gyp:93
> > +          '../runtime/StringPrototype.cpp',
> 
> If you're concerned about this list being duplicated between here and JavaScriptCore.gypi, you can define this list as a variable in JavaScriptCore.gypi and include it from both places.

The issue is that the rule seems to require paths realtive to this gyp file. Now that I look at this again, it seems that I can just prepend ../ to the <(RULE_INPUT_PATH) part of the invocation. I'll fix this. :)
Comment 5 Martin Robinson 2013-02-16 16:30:45 PST
So it seems that there is no way to avoid duplicating the list twice. The paths in JavaScriptCore.gypi are relative to the JavaScriptCore.gypi file, but the rule requires paths relative to JavaScriptCoreGTK.gyp. I'm not sure how to remedy this. Prepending "../" doesn't fix it, because by the time the filename has transformed into <(RULE_INPUT_PATH) it's already been resolve relative to the top-level of the project.
Comment 6 Nico Weber 2013-02-18 02:40:35 PST
Comment on attachment 188650 [details]
Patch

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

Looks pretty good!

>>> Source/JavaScriptCore/JavaScriptCore.gyp/JavaScriptCoreGTK.gyp:93
>>> +          '../runtime/StringPrototype.cpp',
>> 
>> If you're concerned about this list being duplicated between here and JavaScriptCore.gypi, you can define this list as a variable in JavaScriptCore.gypi and include it from both places.
> 
> The issue is that the rule seems to require paths realtive to this gyp file. Now that I look at this again, it seems that I can just prepend ../ to the <(RULE_INPUT_PATH) part of the invocation. I'll fix this. :)

There's also the magic <(DEPTH) variable that you can use for file lists in gypi files.

> Source/JavaScriptCore/JavaScriptCore.gyp/redirect-stdout.sh:4
> +$@ > $OUTPUT_FILE

You can do `exec $1 > $2` here, if you write 'python <@(_inputs)' as a single string in the gyp files. (or $2 > $1 if you prefer putting the output file before the command, but having `command output` matches bash order and also happens to be consistent with a similar script in chromium land.) (the `exec` saves you one process.)
Comment 7 Martin Robinson 2013-02-18 17:29:51 PST
(In reply to comment #6)
 
Thanks for the comments!

> > The issue is that the rule seems to require paths realtive to this gyp file. Now that I look at this again, it seems that I can just prepend ../ to the <(RULE_INPUT_PATH) part of the invocation. I'll fix this. :)
> 
> There's also the magic <(DEPTH) variable that you can use for file lists in gypi files.

Hrm. I think I'll stick with the source list rather than <(DEPTH). It's been very tricky getting out-of-tree builds working for the make generator [1].

> 
> > Source/JavaScriptCore/JavaScriptCore.gyp/redirect-stdout.sh:4
> > +$@ > $OUTPUT_FILE
> 
> You can do `exec $1 > $2` here, if you write 'python <@(_inputs)' as a single string in the gyp files. (or $2 > $1 if you prefer putting the output file before the command, but having `command output` matches bash order and also happens to be consistent with a similar script in chromium land.) (the `exec` saves you one process.)

It seems like this doesn't handle paths with spaces properly?

1. While I am very interested in the ninja generator, our downstream consumers will need to use the make generator.
Comment 8 Martin Robinson 2013-02-18 17:30:15 PST
Comment on attachment 188650 [details]
Patch

Flipping the review flag again, since I think this patch is good as-is.
Comment 9 Dirk Pranke 2013-02-19 12:10:25 PST
(In reply to comment #7)
> (In reply to comment #6)
> 
> Thanks for the comments!
> 
> > > The issue is that the rule seems to require paths realtive to this gyp file. Now that I look at this again, it seems that I can just prepend ../ to the <(RULE_INPUT_PATH) part of the invocation. I'll fix this. :)
> > 
> > There's also the magic <(DEPTH) variable that you can use for file lists in gypi files.
> 
> Hrm. I think I'll stick with the source list rather than <(DEPTH). It's been very tricky getting out-of-tree builds working for the make generator [1].
> 
> > 
> > > Source/JavaScriptCore/JavaScriptCore.gyp/redirect-stdout.sh:4
> > > +$@ > $OUTPUT_FILE
> > 
> > You can do `exec $1 > $2` here, if you write 'python <@(_inputs)' as a single string in the gyp files. (or $2 > $1 if you prefer putting the output file before the command, but having `command output` matches bash order and also happens to be consistent with a similar script in chromium land.) (the `exec` saves you one process.)
> 
> It seems like this doesn't handle paths with spaces properly?
> 

Do we have paths with spaces in them somewhere?

> 1. While I am very interested in the ninja generator, our downstream consumers will need to use the make generator.

I didn't see a mention of ninja anywhere, so I'm not sure what this is referring to?

You should be aware that the make generator doesn't get a lot of attention from chromium-land these days (I think all or at least nearly all of our builds have switched to ninja), so you might be a bit more on the hook for making sure make works for you. You might also want to make sure the make output is what you want it to be (last I looked at it, it had a lot of stuff that seemed like unnecessary boilerplate, but it might be useful or needed to blend in w/ various linux distros, I'm not sure).
Comment 10 Martin Robinson 2013-02-19 12:17:58 PST
(In reply to comment #9)

> > > You can do `exec $1 > $2` here, if you write 'python <@(_inputs)' as a single string in the gyp files. (or $2 > $1 if you prefer putting the output file before the command, but having `command output` matches bash order and also happens to be consistent with a similar script in chromium land.) (the `exec` saves you one process.)
> > 
> > It seems like this doesn't handle paths with spaces properly?
> > 
> 
> Do we have paths with spaces in them somewhere?

Not within WebKit to my knowledge, but theoretically someone could kick off an out-of-tree build into a directory with a space in the name. Since we distribute a source tarball, we have less control over the build environment.

> You should be aware that the make generator doesn't get a lot of attention from chromium-land these days (I think all or at least nearly all of our builds have switched to ninja), so you might be a bit more on the hook for making sure make works for you. You might also want to make sure the make output is what you want it to be (last I looked at it, it had a lot of stuff that seemed like unnecessary boilerplate, but it might be useful or needed to blend in w/ various linux distros, I'm not sure).

That's the impression that I have as well. Since the make generator is so flaky at the moment, I'm worried about making my job bigger by relying too much on DEPTH in the gyp build. To some extent we'll need to fix bugs in the make generator and do some maintenance. This will have to be the state of affairs until I can ensure that it's okay to force the ninja dependency downstream. The great thing about gyp is that it's very hackable and we can easily bundle it with WebKit to include fixes. The only thing that would really worry me is if there are plans to remove the make generator entirely.
Comment 11 Dirk Pranke 2013-02-19 12:28:39 PST
(In reply to comment #10)
> The only thing that would really worry me is if there are plans to remove the make generator entirely.

I don't believe there are, and I don't really see that happening.
Comment 12 Dirk Pranke 2013-02-19 12:37:32 PST
(In reply to comment #7)
> (In reply to comment #6)
> 
> Thanks for the comments!
> 
> > > The issue is that the rule seems to require paths realtive to this gyp file. Now that I look at this again, it seems that I can just prepend ../ to the <(RULE_INPUT_PATH) part of the invocation. I'll fix this. :)
> > 
> > There's also the magic <(DEPTH) variable that you can use for file lists in gypi files.
> 
> Hrm. I think I'll stick with the source list rather than <(DEPTH). It's been very tricky getting out-of-tree builds working for the make generator [1].
> 

Often (but not always) in chromium we use .gypi files that live in the same directory as the .gyp files, to get around issues like this when they crop up. It's not clear to me why we didn't do that in WebKit, but I suspect it was to get around putting too many gyp-related files in the top directories. This reason might be less relevant if we can get people to standardize on gyp.
Comment 13 Dirk Pranke 2013-02-19 12:38:51 PST
Comment on attachment 188650 [details]
Patch

r+; hopefully nico won't shoot me for this :).
Comment 14 WebKit Review Bot 2013-02-19 13:49:05 PST
Comment on attachment 188650 [details]
Patch

Rejecting attachment 188650 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 188650, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
 hunks FAILED -- saving rejects to file Source/JavaScriptCore/JavaScriptCore.gyp/JavaScriptCoreGTK.gyp.rej
patching file Source/JavaScriptCore/JavaScriptCore.gyp/generate-derived-sources.sh
rm 'Source/JavaScriptCore/JavaScriptCore.gyp/generate-derived-sources.sh'
patching file Source/JavaScriptCore/JavaScriptCore.gyp/redirect-stdout.sh

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Dirk Pranke']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16626872
Comment 15 Nico Weber 2013-02-19 13:49:41 PST
(In reply to comment #11)
> (In reply to comment #10)
> > The only thing that would really worry me is if there are plans to remove the make generator entirely.
> 
> I don't believe there are, and I don't really see that happening.

I don't know of any plans to get rid of the make generator either. The old scons generator is still around too, even though it isn't used anywhere in chromium land.

(We might move from make to ninja as the default build system on linux eventually, but we wouldn't delete the generator from gyp if we did that.)

Patch lgtm too.
Comment 16 Martin Robinson 2013-02-19 13:55:46 PST
Committed r143381: <http://trac.webkit.org/changeset/143381>