RESOLVED INVALID 113448
Move preprocessing of idl to pre-pass for gyp
https://bugs.webkit.org/show_bug.cgi?id=113448
Summary Move preprocessing of idl to pre-pass for gyp
Scott Graham
Reported 2013-03-27 15:03:55 PDT
Reduces number of invocations of preprocessor from 20071 to 633.
Attachments
patch (7.93 KB, patch)
2013-03-27 15:04 PDT, Scott Graham
no flags
patch (7.93 KB, patch)
2013-03-27 15:04 PDT, Scott Graham
no flags
tidy up (7.79 KB, patch)
2013-03-27 15:10 PDT, Scott Graham
no flags
remove print (7.71 KB, patch)
2013-03-27 15:34 PDT, Scott Graham
no flags
Patch (9.47 KB, patch)
2013-03-27 15:46 PDT, Scott Graham
no flags
add note to changelog (9.65 KB, patch)
2013-03-27 15:57 PDT, Scott Graham
no flags
fix for older perl versions (9.81 KB, patch)
2013-03-27 16:14 PDT, Scott Graham
no flags
fix SVGURIReference.idl (9.43 KB, patch)
2013-03-27 16:56 PDT, Scott Graham
no flags
rebase (9.43 KB, patch)
2013-03-28 08:55 PDT, Scott Graham
no flags
rebase again (9.24 KB, patch)
2013-03-28 09:58 PDT, Scott Graham
no flags
Scott Graham
Comment 1 2013-03-27 15:04:13 PDT
Scott Graham
Comment 2 2013-03-27 15:04:51 PDT
Scott Graham
Comment 3 2013-03-27 15:08:08 PDT
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
Comment 4 2013-03-27 15:10:43 PDT
Tony Chang
Comment 5 2013-03-27 15:16:56 PDT
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
Comment 6 2013-03-27 15:17:31 PDT
*** Bug 113442 has been marked as a duplicate of this bug. ***
Nico Weber
Comment 7 2013-03-27 15:26:38 PDT
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
Comment 8 2013-03-27 15:34:49 PDT
Created attachment 195415 [details] remove print
Scott Graham
Comment 9 2013-03-27 15:35:17 PDT
(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
Comment 10 2013-03-27 15:46:03 PDT
Scott Graham
Comment 11 2013-03-27 15:46:36 PDT
with ChangeLog now.
Tony Chang
Comment 12 2013-03-27 15:48:57 PDT
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 13 2013-03-27 15:55:12 PDT
WebKit Review Bot
Comment 14 2013-03-27 15:56:11 PDT
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
Comment 15 2013-03-27 15:57:44 PDT
Created attachment 195423 [details] add note to changelog
Tony Chang
Comment 16 2013-03-27 15:58:37 PDT
Looks like File::Slurp and File::Basename doesn't exist on all platforms.
Scott Graham
Comment 17 2013-03-27 16:01:04 PDT
(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
Comment 18 2013-03-27 16:14:18 PDT
Created attachment 195427 [details] fix for older perl versions
WebKit Review Bot
Comment 19 2013-03-27 16:30:33 PDT
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
Comment 20 2013-03-27 16:35:19 PDT
(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 21 2013-03-27 16:47:42 PDT
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
Comment 22 2013-03-27 16:56:34 PDT
Created attachment 195437 [details] fix SVGURIReference.idl
WebKit Review Bot
Comment 23 2013-03-27 18:26:53 PDT
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 24 2013-03-27 18:40:06 PDT
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
Comment 25 2013-03-28 08:55:01 PDT
Kentaro Hara
Comment 26 2013-03-28 08:55:42 PDT
Comment on attachment 195579 [details] rebase here we go!
WebKit Review Bot
Comment 27 2013-03-28 08:58:06 PDT
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
Comment 28 2013-03-28 09:02:59 PDT
(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
Comment 29 2013-03-28 09:40:20 PDT
(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
Comment 30 2013-03-28 09:58:54 PDT
Created attachment 195587 [details] rebase again
WebKit Review Bot
Comment 31 2013-03-28 10:25:17 PDT
Comment on attachment 195587 [details] rebase again Clearing flags on attachment: 195587 Committed r147130: <http://trac.webkit.org/changeset/147130>
WebKit Review Bot
Comment 32 2013-03-28 10:25:22 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 33 2013-03-28 14:54:20 PDT
Re-opened since this is blocked by bug 113539
Tony Chang
Comment 34 2013-03-28 16:31:16 PDT
This patch is now unnecessary.
Note You need to log in before you can comment on or make changes to this bug.