Bug 109969 – [GTK] Improve gyp build JavaScriptCore code generation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 188650 [details]
Patch

[Dirk Pranke]

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.

[Dirk Pranke]

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 ...

[Martin Robinson]

(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. :)

[Martin Robinson]

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.

[Nico Weber]

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.)

[Martin Robinson]

(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.

[Martin Robinson]

Comment on attachment 188650 [details]
Patch

Flipping the review flag again, since I think this patch is good as-is.

[Dirk Pranke]

(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).

[Martin Robinson]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Comment on attachment 188650 [details]
Patch

r+; hopefully nico won't shoot me for this :).

[WebKit Review Bot]

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

[Nico Weber]

(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.

[Martin Robinson]

Committed r143381: <http://trac.webkit.org/changeset/143381>