Bug 113448 – Move preprocessing of idl to pre-pass for gyp

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Scott Graham]

Reduces number of invocations of preprocessor from 20071 to 633.

[Scott Graham]

Created attachment 195405 [details]
patch

[Scott Graham]

Created attachment 195406 [details]
patch

[Scott Graham]

preprocessor.pm still does the preprocessing, and if available returns an already preprocessed idl file. This allows a few other users of preprocessor (in css, etc.) to not bother having a pre-pass.

If it looks reasonable, I'll add a ChangeLog, etc.

[Scott Graham]

Created attachment 195409 [details]
tidy up

[Tony Chang]

Comment on attachment 195409 [details]
tidy up

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

Nice, I like this better than my patch.  Let's clean this up and land this instead.

> WebCore/WebCore.gypi:716
> +            'svg/SVGURIReference.idl',

Is this change intentional?

> WebCore/bindings/scripts/preprocessor.pm:56
> +        print "$fileName was not pre-preprocessed, looking for $preprocessedFile\n";

We probably don't want this to print this.  I think some other port may use $outputDirectory but without the preprocess build step, this will just spam to the console.

[Tony Chang]

*** Bug 113442 has been marked as a duplicate of this bug. ***

[Nico Weber]

I think this looks reasonable.

It's a bit unfortunate that we still need to shell out to perl for the preprocessing step, but oh well.

[Scott Graham]

Created attachment 195415 [details]
remove print

[Scott Graham]

(In reply to comment #5)
> (From update of attachment 195409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195409&action=review
> 
> Nice, I like this better than my patch.  Let's clean this up and land this instead.
> 
> > WebCore/WebCore.gypi:716
> > +            'svg/SVGURIReference.idl',
> 
> Is this change intentional?

Yes, it wasn't included in the top-level gyp list, but is referenced in other idls, so it ended up being processed a few hundred times via references. It was the only one in that situation, so I assumed the omission was unintentional. (?)

> 
> > WebCore/bindings/scripts/preprocessor.pm:56
> > +        print "$fileName was not pre-preprocessed, looking for $preprocessedFile\n";
> 
> We probably don't want this to print this.  I think some other port may use $outputDirectory but without the preprocess build step, this will just spam to the console.

Done.

[Scott Graham]

Created attachment 195420 [details]
Patch

[Scott Graham]

with ChangeLog now.

[Tony Chang]

Comment on attachment 195420 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        * WebCore.gypi:

Nit: Please explain why you added the idl file here.

> Source/WebCore/WebCore.gyp/WebCore.gyp:649
>        'sources': [
>          # bison rule

Should we remove the preprocess flag from our calls to generate-bindings.pl?

[EFL EWS Bot]

Comment on attachment 195420 [details]
Patch

Attachment 195420 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17288684

[WebKit Review Bot]

Comment on attachment 195420 [details]
Patch

Attachment 195420 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17319242

[Scott Graham]

Created attachment 195423 [details]
add note to changelog

[Tony Chang]

Looks like File::Slurp and File::Basename doesn't exist on all platforms.

[Scott Graham]

(In reply to comment #12)
> (From update of attachment 195420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195420&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        * WebCore.gypi:
> 
> Nit: Please explain why you added the idl file here.

Done.

> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:649
> >        'sources': [
> >          # bison rule
> 
> Should we remove the preprocess flag from our calls to generate-bindings.pl?

I might be confused, but I think we shouldn't because of cases like the idl above -- if someone forgets to add a subreferenced idl, this way it'll still work.

[Scott Graham]

Created attachment 195427 [details]
fix for older perl versions

[WebKit Review Bot]

Comment on attachment 195427 [details]
fix for older perl versions

Attachment 195427 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17329205

[Scott Graham]

(In reply to comment #19)
> (From update of attachment 195427 [details])
> Attachment 195427 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://webkit-commit-queue.appspot.com/results/17329205

Hm, apparently that's why. Sigh.

I can either add it manually to the preprocess, or let it do a couple hundred extra invocations I guess.

[Peter Beverloo (cr-android ews)]

Comment on attachment 195427 [details]
fix for older perl versions

Attachment 195427 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17130990

[Scott Graham]

Created attachment 195437 [details]
fix SVGURIReference.idl

[WebKit Review Bot]

Comment on attachment 195437 [details]
fix SVGURIReference.idl

Rejecting attachment 195437 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 195437, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 14075 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
55>At revision 14075.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/17239764

[WebKit Review Bot]

Comment on attachment 195437 [details]
fix SVGURIReference.idl

Rejecting attachment 195437 [details] from commit-queue.

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

Last 500 characters of output:
2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/bindings/scripts/CodeGenerator.pm.rej
patching file Source/WebCore/bindings/scripts/IDLParser.pm
patching file Source/WebCore/bindings/scripts/generate-bindings.pl
patching file Source/WebCore/bindings/scripts/generate-preprocessed-idls.pl
patching file Source/WebCore/bindings/scripts/preprocessor.pm

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

Full output: http://webkit-commit-queue.appspot.com/results/17342025

[Scott Graham]

Created attachment 195579 [details]
rebase

[Kentaro Hara]

Comment on attachment 195579 [details]
rebase

here we go!

[WebKit Review Bot]

Comment on attachment 195579 [details]
rebase

Rejecting attachment 195579 [details] from commit-queue.

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

Last 500 characters of output:
2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/bindings/scripts/CodeGenerator.pm.rej
patching file Source/WebCore/bindings/scripts/IDLParser.pm
patching file Source/WebCore/bindings/scripts/generate-bindings.pl
patching file Source/WebCore/bindings/scripts/generate-preprocessed-idls.pl
patching file Source/WebCore/bindings/scripts/preprocessor.pm

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

Full output: http://webkit-commit-queue.appspot.com/results/17342159

[Scott Graham]

(In reply to comment #27)
> (From update of attachment 195579 [details])
> Rejecting attachment 195579 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 195579, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue
> 
> Last 500 characters of output:
> 2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/bindings/scripts/CodeGenerator.pm.rej
> patching file Source/WebCore/bindings/scripts/IDLParser.pm
> patching file Source/WebCore/bindings/scripts/generate-bindings.pl
> patching file Source/WebCore/bindings/scripts/generate-preprocessed-idls.pl
> patching file Source/WebCore/bindings/scripts/preprocessor.pm
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue
> 
> Full output: http://webkit-commit-queue.appspot.com/results/17342159

Ah, looks like it's because of http://trac.webkit.org/changeset/147041/trunk/Source/WebCore/bindings/scripts/CodeGenerator.pm which I also had to modify here.

I only rebased to the Chromium DEPS revision. I will wait until WebKit is rolled in Chromium and then try again.

[Nico Weber]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 195579 [details] [details])
> > Rejecting attachment 195579 [details] [details] from commit-queue.
> > 
> > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 195579, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue
> > 
> > Last 500 characters of output:
> > 2 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/bindings/scripts/CodeGenerator.pm.rej
> > patching file Source/WebCore/bindings/scripts/IDLParser.pm
> > patching file Source/WebCore/bindings/scripts/generate-bindings.pl
> > patching file Source/WebCore/bindings/scripts/generate-preprocessed-idls.pl
> > patching file Source/WebCore/bindings/scripts/preprocessor.pm
> > 
> > Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue
> > 
> > Full output: http://webkit-commit-queue.appspot.com/results/17342159
> 
> Ah, looks like it's because of http://trac.webkit.org/changeset/147041/trunk/Source/WebCore/bindings/scripts/CodeGenerator.pm which I also had to modify here.
> 
> I only rebased to the Chromium DEPS revision. I will wait until WebKit is rolled in Chromium and then try again.

If you're on old git, you can `git checkout master; git pull; git rebase master yourbranch` and reupload to rebase to webkit head (and then `git checkout gclient` again to get back to deps)

[Scott Graham]

Created attachment 195587 [details]
rebase again

[WebKit Review Bot]

Comment on attachment 195587 [details]
rebase again

Clearing flags on attachment: 195587

Committed r147130: <http://trac.webkit.org/changeset/147130>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 113539

[Tony Chang]

This patch is now unnecessary.