Bug 94290 - Pre-process CSSGrammar.y before running through bison.
Summary: Pre-process CSSGrammar.y before running through bison.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pablo Flouret
URL:
Keywords:
Depends on: 98946
Blocks: 86146 94099
  Show dependency treegraph
 
Reported: 2012-08-16 21:15 PDT by Shane Stephens
Modified: 2012-10-17 01:31 PDT (History)
23 users (show)

See Also:


Attachments
Patch (94.97 KB, patch)
2012-08-16 21:19 PDT, Shane Stephens
no flags Details | Formatted Diff | Diff
Patch (116.45 KB, patch)
2012-10-09 11:33 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (116.30 KB, patch)
2012-10-09 12:27 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (116.66 KB, patch)
2012-10-09 15:20 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (113.17 KB, patch)
2012-10-12 13:34 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Rebased patch (113.69 KB, patch)
2012-10-12 13:53 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (113.67 KB, patch)
2012-10-12 14:12 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff
Rebased patch (113.69 KB, patch)
2012-10-15 12:53 PDT, Pablo Flouret
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (113.66 KB, patch)
2012-10-15 17:45 PDT, Pablo Flouret
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shane Stephens 2012-08-16 21:15:13 PDT
[WIP] Pre-process CSSGrammar.y before running through bison.
Comment 1 Shane Stephens 2012-08-16 21:19:51 PDT
Created attachment 158988 [details]
Patch
Comment 2 Peter Beverloo (cr-android ews) 2012-08-16 21:26:22 PDT
Comment on attachment 158988 [details]
Patch

Attachment 158988 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13506976
Comment 3 Tony Chang 2012-08-16 21:28:48 PDT
Comment on attachment 158988 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        * DerivedSources.make:

Chromium doens't use DerivedSources.make.  You'll have to update WebCore.gyp/Webcore.gyp too.

> Source/WebCore/ChangeLog:11
> +        * css/makegrammar.pl:

Did you mean to include makegrammar.pl here? I don't see any changes to this file.
Comment 4 Alexey Proskuryakov 2012-08-17 09:56:56 PDT
Neither the patch nor the bug explain the rationale. Please do provide it.
Comment 5 Tony Chang 2012-08-20 12:22:29 PDT
In https://bugs.webkit.org/show_bug.cgi?id=94099#c4 , Antti asked that all code related to CSS hierarchies be wrapped with #ifs.

Antti, I assume you wanted this for the changes to CSSGrammar.y too, right? Is this what you had in mind?
Comment 6 Antti Koivisto 2012-10-04 11:18:24 PDT
Right, I think it would be useful to have feature defines in grammar too.
Comment 7 Pablo Flouret 2012-10-09 11:33:49 PDT
Created attachment 167800 [details]
Patch
Comment 8 Pablo Flouret 2012-10-09 11:39:17 PDT
(In reply to comment #7)
> Created an attachment (id=167800) [details]
> Patch

I could only test this on the mac and chromium ports, so if this works correctly on the first try i'll probably be very surprised :-)

If there's someone from a port not on the cc list that i should cc, let me know.
Comment 9 Early Warning System Bot 2012-10-09 11:45:03 PDT
Comment on attachment 167800 [details]
Patch

Attachment 167800 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14245172
Comment 10 Early Warning System Bot 2012-10-09 11:53:29 PDT
Comment on attachment 167800 [details]
Patch

Attachment 167800 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14229665
Comment 11 Tony Chang 2012-10-09 12:19:26 PDT
Comment on attachment 167800 [details]
Patch

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

Overall, looks great once the ews bots all go green.  I think it would also be OK to use makegrammar.pl in the Chromium build, but this is also fine.

> Source/WebCore/WebCore.gyp/WebCore.gyp:1082
> +            '../css/CSSGrammar.y.in',

Shouldn't CSSGrammar.y.includes be an input too?

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:50
> +inputFile = sys.argv[1]
> +outputDir = sys.argv[2]

Nit: python style variable naming is input_file.

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:54
> +    # featureFlags = ["-D", "ENABLE_FEATURE1=1", "-D", "ENABLE_FEATURE2=0", ...]

Nit: Remove testing comment?

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:57
> +gccExe = "gcc"

gcc_exe and other variables below.

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:61
> +(inputRoot, inputExt) = os.path.splitext(inputFile)

Nit: () on the left side aren't necessary.

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:64
> +output = subprocess.Popen(['ls', '-l'], stdout=subprocess.PIPE).communicate()[0]

Remove testing command?
Comment 12 Pablo Flouret 2012-10-09 12:27:44 PDT
Created attachment 167817 [details]
Patch

Trying to fix qt/gtk build errors.
Comment 13 Pablo Flouret 2012-10-09 12:30:11 PDT
If i understand my own patch correctly, the targets don't really depend on CSSGrammar.y.includes, so changing that file won't trigger a re-generation of CSSGrammar.y, which i imagine could be annoying.

If anyone knows how to do this for each build system i can modify the patch, or do it on a subsequent one.

(PS: i hate build systems)
Comment 14 Pablo Flouret 2012-10-09 12:39:23 PDT
(In reply to comment #11)
> (From update of attachment 167800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167800&action=review
> 
> Overall, looks great once the ews bots all go green.  I think it would also be OK to use makegrammar.pl in the Chromium build, but this is also fine.

I thought about using makegrammar.pl here as well, but it would need some shoehorning. The python way goes a bit better with the gyp ways, i think.

> > Source/WebCore/WebCore.gyp/WebCore.gyp:1082
> > +            '../css/CSSGrammar.y.in',
> 
> Shouldn't CSSGrammar.y.includes be an input too?

Yeah, i guess, it was easier like this in terms of parsing the arguments in the script, but i could make that more robust. Should i make the script more generic to handle multiple .y.in files, or should that bridge be crossed if it's ever needed? (I imagine XPathGrammar.y doesn't see much action?)

> > Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:50
> > +inputFile = sys.argv[1]
> > +outputDir = sys.argv[2]
> 
> Nit: python style variable naming is input_file.

All the other action_* and rule_* scripts use this style, should i change it anyway?
Comment 15 Tony Chang 2012-10-09 13:28:41 PDT
(In reply to comment #14)
> (In reply to comment #11)
> > Shouldn't CSSGrammar.y.includes be an input too?
> 
> Yeah, i guess, it was easier like this in terms of parsing the arguments in the script, but i could make that more robust. Should i make the script more generic to handle multiple .y.in files, or should that bridge be crossed if it's ever needed? (I imagine XPathGrammar.y doesn't see much action?)

I wouldn't bother making the script handle multiple .y.in files. Like you said, this stuff doesn't change that often.

> > > Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:50
> > > +inputFile = sys.argv[1]
> > > +outputDir = sys.argv[2]
> > 
> > Nit: python style variable naming is input_file.
> 
> All the other action_* and rule_* scripts use this style, should i change it anyway?

The other scripts were written before we had much python code in WebKit and had converged on PEP8 style.  We should try to use PEP8 style in new files.
Comment 16 Pablo Flouret 2012-10-09 15:20:56 PDT
Created attachment 167860 [details]
Patch
Comment 17 Tony Chang 2012-10-09 16:25:03 PDT
Comment on attachment 167860 [details]
Patch

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

LGTM assuming all the ews bots go green.  The feedback on the python script are minor, so feel free to ignore.

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:62
> +if len(arguments) == 0:
> +    exit_with_usage(1)

Nit: "if not len(arguments):" would be more like WebKit style.

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:76
> +gcc_exe = "gcc"
> +if "CC" in os.environ:
> +    gcc_exe = os.environ["CC"]

Nit: This could just be gcc_exe = os.environ.get("CC", "gcc").

> Source/WebCore/WebCore.gyp/scripts/action_preprocessgrammar.py:86
> +output_file = open(os.path.join(output_dir, os.path.basename(input_root)), "w")

Nit: Using 'with' for files would automatically close it for you.  It's a bit more idiomatic.
Comment 18 Pablo Flouret 2012-10-09 16:41:11 PDT
Looks like they're all green. If you want we can wait overnight to see if anyone from the other ports has any comments. Otherwise cq+ away.
Comment 19 WebKit Review Bot 2012-10-10 11:34:19 PDT
Comment on attachment 167860 [details]
Patch

Clearing flags on attachment: 167860

Committed r130937: <http://trac.webkit.org/changeset/130937>
Comment 20 WebKit Review Bot 2012-10-10 11:34:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Csaba Osztrogonác 2012-10-10 12:01:57 PDT
(In reply to comment #19)
> (From update of attachment 167860 [details])
> Clearing flags on attachment: 167860
> 
> Committed r130937: <http://trac.webkit.org/changeset/130937>

It broke the Qt Win build:
	perl C:/WebKitBuildSlave/szeged-windows-1/qt-windows-32bit-release/build/Source/WebCore/css/makegrammar.pl --grammar C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\css\CSSGrammar.y.in --outputDir generated --extraDefines "UNICODE WIN32 ENABLE_3D_RENDERING=1 ENABLE_BLOB=1 ENABLE_CHANNEL_MESSAGING=1 ENABLE_CSS_BOX_DECORATION_BREAK=1 ENABLE_CSS_COMPOSITING=1 ENABLE_CSS_EXCLUSIONS=1 ENABLE_CSS_FILTERS=1 ENABLE_CSS_IMAGE_SET=1 ENABLE_CSS_REGIONS=1 ENABLE_CSS_STICKY_POSITION=1 ENABLE_DATALIST_ELEMENT=1 ENABLE_DETAILS_ELEMENT=1 ENABLE_FAST_MOBILE_SCROLLING=1 ENABLE_FILTERS=1 ENABLE_FTPDIR=1 ENABLE_FULLSCREEN_API=1 ENABLE_GESTURE_EVENTS=1 ENABLE_ICONDATABASE=1 ENABLE_IFRAME_SEAMLESS=1 ENABLE_INPUT_TYPE_COLOR=1 ENABLE_INSPECTOR=1 ENABLE_INSPECTOR_SERVER=1 ENABLE_JAVASCRIPT_DEBUGGER=1 ENABLE_LEGACY_NOTIFICATIONS=1 ENABLE_LEGACY_VIEWPORT_ADAPTION=1 ENABLE_LEGACY_VENDOR_PREFIXES=1 ENABLE_METER_ELEMENT=1 ENABLE_NOTIFICATIONS=1 ENABLE_PAGE_VISIBILITY_API=1 ENABLE_PROGRESS_ELEMENT=1 ENABLE_REQUEST_ANIMATION_FRAME=1 ENABLE_SHARED_WORKERS=1 ENABLE_SMOOTH_SCROLLING=1 ENABLE_SQL_DATABASE=1 ENABLE_SVG=1 ENABLE_SVG_FONTS=1 ENABLE_TOUCH_ADJUSTMENT=1 ENABLE_TOUCH_EVENTS=1 ENABLE_WEB_SOCKETS=1 ENABLE_WEB_TIMING=1 ENABLE_WORKERS=1 WTF_USE_TILED_BACKING_STORE=1 HAVE_QTQUICK=1 HAVE_QTPRINTSUPPORT=1 HAVE_QSTYLE=1 HAVE_QTTESTLIB=1 ENABLE_NETSCAPE_PLUGIN_API=1 PLUGIN_ARCHITECTURE_UNSUPPORTED=1 WTF_USE_3D_GRAPHICS=1 ENABLE_WEBGL=1 ENABLE_CSS_SHADERS=1 ENABLE_ORIENTATION_EVENTS=1 ENABLE_DEVICE_ORIENTATION=1 ENABLE_TOUCH_SLIDER=1 ENABLE_ACCELERATED_2D_CANVAS=0 ENABLE_ANIMATION_API=0 ENABLE_BATTERY_STATUS=0 ENABLE_CSP_NEXT=0 ENABLE_CSS_GRID_LAYOUT=0 ENABLE_CSS_HIERARCHIES=0 ENABLE_CSS_IMAGE_ORIENTATION=0 ENABLE_CSS_IMAGE_RESOLUTION=0 ENABLE_CSS_VARIABLES=0 ENABLE_CSS3_TEXT_DECORATION=0 ENABLE_DASHBOARD_SUPPORT=0 ENABLE_DATAGRID=0 ENABLE_DATA_TRANSFER_ITEMS=0 ENABLE_DIRECTORY_UPLOAD=0 ENABLE_DOWNLOAD_ATTRIBUTE=0 ENABLE_FILE_SYSTEM=0 ENABLE_GAMEPAD=0 ENABLE_GEOLOCATION=0 ENABLE_HIGH_DPI_CANVAS=0 ENABLE_INDEXED_DATABASE=0 ENABLE_INPUT_SPEECH=0 ENABLE_INPUT_TYPE_DATE=0 ENABLE_INPUT_TYPE_DATETIME=0 ENABLE_INPUT_TYPE_DATETIMELOCAL=0 ENABLE_INPUT_TYPE_MONTH=0 ENABLE_INPUT_TYPE_TIME=0 ENABLE_INPUT_TYPE_WEEK=0 ENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 ENABLE_LINK_PREFETCH=0 ENABLE_LINK_PRERENDER=0 ENABLE_MATHML=0 ENABLE_MEDIA_SOURCE=0 ENABLE_MEDIA_STATISTICS=0 ENABLE_MEDIA_STREAM=0 ENABLE_MHTML=0 ENABLE_MICRODATA=0 ENABLE_MUTATION_OBSERVERS=0 ENABLE_NAVIGATOR_CONTENT_UTILS=0 ENABLE_NETWORK_INFO=0 ENABLE_QUOTA=0 ENABLE_SCRIPTED_SPEECH=0 ENABLE_SHADOW_DOM=0 ENABLE_STYLE_SCOPED=0 ENABLE_SVG_DOM_OBJC_BINDINGS=0 ENABLE_TEXT_AUTOSIZING=0 ENABLE_TEXT_NOTIFICATIONS_ONLY=0 ENABLE_TOUCH_ICON_LOADING=0 ENABLE_UNDO_MANAGER=0 ENABLE_VIBRATION=0 ENABLE_VIDEO=0 ENABLE_VIDEO_TRACK=0 ENABLE_WEB_AUDIO=0 ENABLE_XSLT=0" --symbolsPrefix cssyy
The system cannot find the path specified.
generated\\CSSGrammar.y:86.1: syntax error, unexpected end of file
Died at C:/WebKitBuildSlave/szeged-windows-1/qt-windows-32bit-release/build/Source/WebCore/css/makegrammar.pl line 88.
Comment 22 WebKit Review Bot 2012-10-10 12:35:04 PDT
Re-opened since this is blocked by bug 98946
Comment 23 Tony Chang 2012-10-10 12:40:52 PDT
Actually, I think this is just using the wrong slash for paths.  Let me try to fix.
Comment 24 Pablo Flouret 2012-10-10 12:42:41 PDT
(In reply to comment #23)
> Actually, I think this is just using the wrong slash for paths.  Let me try to fix.

If that's not it then this might do it:

diff --git a/Source/WebCore/css/makegrammar.pl b/Source/WebCore/css/makegrammar.pl
index 32f757e..4b587ea 100644
--- a/Source/WebCore/css/makegrammar.pl
+++ b/Source/WebCore/css/makegrammar.pl
@@ -25,6 +25,7 @@ use Config;
 use File::Basename;
 use File::Spec;
 use Getopt::Long;
+use IO::File;
 
 require Config;
 
@@ -58,21 +59,20 @@ if ($suffix eq ".y.in") {
     my $grammarFileOutPath = File::Spec->join($outputDir, "$filename.y");
     my $featureFlags = "-D " . join(" -D ", split(" ", $extraDefines));
 
-    system($gcc . " -E -P -x c $grammarFilePath $featureFlags > $grammarFileOutPath.processed" );
+    my $processed = new IO::File;
+    open $processed, "$gcc -E -P -x c $grammarFilePath $featureFlags|";
 
     open GRAMMAR, ">$grammarFileOutPath" or die;
     open INCLUDES, "<$grammarFileIncludesPath" or die;
-    open PROCESSED, "<$grammarFileOutPath.processed" or die;
 
     while (<INCLUDES>) {
         print GRAMMAR;
     }
-    while (<PROCESSED>) {
+    while (<$processed>) {
         print GRAMMAR;
     }
 
     close GRAMMAR;
-    unlink("$grammarFileOutPath.processed");
     $grammarFilePath = $grammarFileOutPath;
 }
Comment 25 Zoltan Arvai 2012-10-10 14:18:06 PDT
If it helps here is the content of CSSGrammar.y that read by makegrammar.pl line 88:
https://gist.github.com/3868405
Comment 26 Tony Chang 2012-10-10 14:32:51 PDT
Reverted r130937 for reason:

Breaks Qt build

Committed r130962: <http://trac.webkit.org/changeset/130962>
Comment 27 Tony Chang 2012-10-10 14:36:38 PDT
This is what we learned:

Qt uses --preprocessor \"$${QMAKE_MOC} -E\" in other places that we use the proprocessor.  My last attempt to fix the build (http://trac.webkit.org/changeset/130957) used that, but the output from moc -E is different from gcc (namely, it doesn't maintain leading spaces).

I'm not sure what the right fix for this is.  Is there a way we can get moc to behave more like gcc?  Alternately, maybe we can use cl.exe on Qt Win?
Comment 28 Pablo Flouret 2012-10-10 17:11:57 PDT
(In reply to comment #27)
> This is what we learned:
> 
> Qt uses --preprocessor \"$${QMAKE_MOC} -E\" in other places that we use the proprocessor.  My last attempt to fix the build (http://trac.webkit.org/changeset/130957) used that, but the output from moc -E is different from gcc (namely, it doesn't maintain leading spaces).
> 
> I'm not sure what the right fix for this is.  Is there a way we can get moc to behave more like gcc?  Alternately, maybe we can use cl.exe on Qt Win?

Why did this not fail on chromium win? Is it using cygwin or requiring gcc around?

Alternatively, i could modify the script to do very dumb preprocessing of just #if ENABLE_* directives, since doing anything fancier than that in the .y file would probably break anyway...
Comment 29 Adam Barth 2012-10-10 17:35:23 PDT
This caused trouble for the ChromiumOS build as well.
Comment 30 Tony Chang 2012-10-10 18:30:17 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > This is what we learned:
> > 
> > Qt uses --preprocessor \"$${QMAKE_MOC} -E\" in other places that we use the proprocessor.  My last attempt to fix the build (http://trac.webkit.org/changeset/130957) used that, but the output from moc -E is different from gcc (namely, it doesn't maintain leading spaces).
> > 
> > I'm not sure what the right fix for this is.  Is there a way we can get moc to behave more like gcc?  Alternately, maybe we can use cl.exe on Qt Win?
> 
> Why did this not fail on chromium win? Is it using cygwin or requiring gcc around?

I suspect gcc is installed.

> Alternatively, i could modify the script to do very dumb preprocessing of just #if ENABLE_* directives, since doing anything fancier than that in the .y file would probably break anyway...

I think it would be better to figure out how to use cl.exe on the various Windows platforms, unless the Qt guys have another suggestion.

Adam, do you have a link to the chromiumos failure? I don't see any chromiumos bots on either build.webkit.org or http://build.chromium.org/p/chromium.webkit/
Comment 31 Adam Barth 2012-10-10 19:52:59 PDT
We didn't see the problem until the patch reached the main waterfall:

http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28daisy%29/builds/3657/steps/cbuildbot/logs/stdio

chromeos-chrome-24.0.1292.0_alpha-r1: Traceback (most recent call last):
chromeos-chrome-24.0.1292.0_alpha-r1:   File "scripts/action_preprocessgrammar.py", line 82, in <module>
chromeos-chrome-24.0.1292.0_alpha-r1:     stdout=subprocess.PIPE)
chromeos-chrome-24.0.1292.0_alpha-r1:   File "/usr/lib64/python2.6/subprocess.py", line 623, in __init__
chromeos-chrome-24.0.1292.0_alpha-r1:     errread, errwrite)
chromeos-chrome-24.0.1292.0_alpha-r1:   File "/usr/lib64/python2.6/subprocess.py", line 1141, in _execute_child
chromeos-chrome-24.0.1292.0_alpha-r1:     raise child_exception
chromeos-chrome-24.0.1292.0_alpha-r1: OSError: [Errno 2] No such file or directory
chromeos-chrome-24.0.1292.0_alpha-r1: make: *** [c/Release/obj/gen/webkit/CSSGrammar.y] Error 1
chromeos-chrome-24.0.1292.0_alpha-r1: make: *** Waiting for unfinished jobs....
chromeos-chrome-24.0.1292.0_alpha-r1: emake failed
Comment 32 Tony Chang 2012-10-11 10:37:21 PDT
(In reply to comment #31)
> We didn't see the problem until the patch reached the main waterfall:
> 
> http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28daisy%29/builds/3657/steps/cbuildbot/logs/stdio

Hmm, that error means it didn't find the compiler (it's an arm compile, the lines above use armv7a-cros-linux-gnueabi-g++).  I'm not sure if the CC environment variable was set.  It doesn't look like it, so I'm not sure how make knows to use armv7a-cros-linux-gnueabi-g++.

The other .py scripts used by the chromium build just call the .pl file and the .pl scripts mostly use Source/WebCore/bindings/scripts/preprocessor.pm.  Maybe we can continue to use that method rather than trying to write new code.
Comment 33 Pablo Flouret 2012-10-11 11:24:57 PDT
(In reply to comment #32)
> 
> The other .py scripts used by the chromium build just call the .pl file and the .pl scripts mostly use Source/WebCore/bindings/scripts/preprocessor.pm.  Maybe we can continue to use that method rather than trying to write new code.

applyPreprocessor() essentially does the same that's being done in makegrammar.pl, so it wouldn't solve Qt woes anyway.

I'm wondering if just handling simple #if ENABLE_* directives in makegrammar.pl isn't the right thing to do after all. Anything other than that should probably pass through to the CSSGrammar.y, there would be no need to separate into .y.in and .y.includes, we could get rid of action_preprocessgrammar.py, no fumbling to find a preprocessor in every platform under the sun, etc.

What do you think?
Comment 34 Simon Hausmann 2012-10-12 05:05:24 PDT
(In reply to comment #27)
> This is what we learned:
> 
> Qt uses --preprocessor \"$${QMAKE_MOC} -E\" in other places that we use the proprocessor.  My last attempt to fix the build (http://trac.webkit.org/changeset/130957) used that, but the output from moc -E is different from gcc (namely, it doesn't maintain leading spaces).
> 
> I'm not sure what the right fix for this is.  Is there a way we can get moc to behave more like gcc?  Alternately, maybe we can use cl.exe on Qt Win?

I've looked a bit into this and the issue seems to be the following:

The bison grammar contains things like

  $$ = parser->createFloatingSelector();

Moc is tokenizing the input and due to the way it works it is tokenizing this in a way that it _usually_ works, which is as a C++ parser, because of its actual purpose. In C++ the dollar sign is not a valid character for identifiers, and therefore this causes an error (that is silently discarded).

When moc is used in preprocessor-only mode however it should not do that. I'm working on a fix for that. Locally applied I can build with the original patch that passes --preprocessor to makegrammar.pl.
Comment 35 Simon Hausmann 2012-10-12 05:11:39 PDT
(In reply to comment #34)
[...]
> When moc is used in preprocessor-only mode however it should not do that. I'm working on a fix for that. Locally applied I can build with the original patch that passes --preprocessor to makegrammar.pl.

Fix uploaded into Qt Gerrit:

    https://codereview.qt-project.org/#change,37022
Comment 36 Pablo Flouret 2012-10-12 13:34:09 PDT
Created attachment 168471 [details]
Patch
Comment 37 Pablo Flouret 2012-10-12 13:38:41 PDT
(In reply to comment #36)
> Created an attachment (id=168471) [details]
> Patch

Cleaned up makegrammar.pl a bit and made it take the preprocessor and bison executables as arguments whenever possible, so provided the moc fix is in the bots i think now it should work correctly everywhere (including chromiumOS, since it should've failed the first time because the preprocessor wasn't an argument, i believe; we still need to keep an eye, of course).

Also got rid of action_preprocessgrammar.py, used makegrammar.pl on chromium here as well.
Comment 38 Pablo Flouret 2012-10-12 13:53:16 PDT
Created attachment 168475 [details]
Rebased patch
Comment 39 Gyuyoung Kim 2012-10-12 14:05:41 PDT
Comment on attachment 168475 [details]
Rebased patch

Attachment 168475 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14292008
Comment 40 Pablo Flouret 2012-10-12 14:12:01 PDT
Created attachment 168479 [details]
Patch

Some debugging cruft was in the way.
Comment 41 Tony Chang 2012-10-12 14:36:54 PDT
Comment on attachment 168479 [details]
Patch

Thanks! Re-using preprocess.pm and using the same perl script on Chromium seems more likely to work.

Maybe someone from Qt can try landing this and making sure the bots don't go red.
Comment 42 Adam Barth 2012-10-12 14:39:41 PDT
Can you hold off landing this for a while?  The tree has been a disaster recently and I haven't been able to get a roll in all day.
Comment 43 Tony Chang 2012-10-12 14:43:42 PDT
(In reply to comment #42)
> Can you hold off landing this for a while?  The tree has been a disaster recently and I haven't been able to get a roll in all day.

Yeah, I was hoping that after the ews bubbles all go green, someone from Qt can try landing it during their Monday morning (i.e., not today).
Comment 44 Gyuyoung Kim 2012-10-12 15:03:46 PDT
Comment on attachment 168479 [details]
Patch

Attachment 168479 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14294026
Comment 45 Early Warning System Bot 2012-10-12 15:35:57 PDT
Comment on attachment 168479 [details]
Patch

Attachment 168479 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14291031
Comment 46 Early Warning System Bot 2012-10-12 15:46:46 PDT
Comment on attachment 168479 [details]
Patch

Attachment 168479 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14296041
Comment 47 Simon Hausmann 2012-10-13 09:32:40 PDT
(In reply to comment #43)
> (In reply to comment #42)
> > Can you hold off landing this for a while?  The tree has been a disaster recently and I haven't been able to get a roll in all day.
> 
> Yeah, I was hoping that after the ews bubbles all go green, someone from Qt can try landing it during their Monday morning (i.e., not today).

The Qt fix went in, but it's not through all the stage of the Qt CI system yet. I hope that the change will be available in the final repo by middle of next week. It will require anyone building the Qt port of WebKit to update the Qt version, so could we perhaps aim for landing this end of next week? Or is that too late for you guys?
Comment 48 Simon Hausmann 2012-10-14 03:00:01 PDT
(In reply to comment #47)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > Can you hold off landing this for a while?  The tree has been a disaster recently and I haven't been able to get a roll in all day.
> > 
> > Yeah, I was hoping that after the ews bubbles all go green, someone from Qt can try landing it during their Monday morning (i.e., not today).
> 
> The Qt fix went in, but it's not through all the stage of the Qt CI system yet. I hope that the change will be available in the final repo by middle of next week. It will require anyone building the Qt port of WebKit to update the Qt version, so could we perhaps aim for landing this end of next week? Or is that too late for you guys?

The good news is that unexpectedly the update went through very fast and from the Qt side things are ready. I'll send a heads-up for the upcoming update to the webkit-qt mailing list and leave it as Ossy's discretion when to do the update (whenever it suits you best time wise :)
Comment 49 Csaba Osztrogonác 2012-10-15 03:48:17 PDT
I'm updating the Linux bots today, Zoltán will update the Windows bot today or tomorrow, so we will be able to land it soon.
Comment 50 Tony Chang 2012-10-15 10:00:23 PDT
FWIW, it's fine to wait until later in the week to land this.  I don't think it's blocking anything that time critical.
Comment 51 Pablo Flouret 2012-10-15 12:53:33 PDT
Created attachment 168764 [details]
Rebased patch
Comment 52 Pablo Flouret 2012-10-15 12:55:23 PDT
(In reply to comment #51)
> Created an attachment (id=168764) [details]
> Rebased patch

The bots were pretty wonky on friday, new ews run to be sure everything is still green.
Comment 53 Csaba Osztrogonác 2012-10-15 13:00:40 PDT
I updated the Qt 5 on the Linux bots. We are waiting for the update on Qt-ARM bot and the update on Qt-Windows bot. I think the guys will finish it tomorrow.
Comment 54 Gyuyoung Kim 2012-10-15 14:18:57 PDT
Comment on attachment 168764 [details]
Rebased patch

Attachment 168764 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14288842
Comment 55 Pablo Flouret 2012-10-15 14:56:47 PDT
Comment on attachment 168764 [details]
Rebased patch

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

> Source/cmake/WebKitMacros.cmake:156
> +        COMMAND ${PERL_EXECUTABLE} -I ${WEBCORE_DIR}/bindings/scripts ${WEBCORE_DIR}/css/makegrammar.pl ${_input} --outputDir ${DERIVED_SOURCES_WEBCORE_DIR} --extraDefines "${_features}" --preprocessor "${CODE_GENERATOR_PREPROCESSOR}" --bison "${BISON_EXECUTABLE}" --symbolsPrefix ${_prefix} ${_input}

EFL build fails due to the (erroneous) extra ${_input} here.
Comment 56 Pablo Flouret 2012-10-15 17:45:11 PDT
Created attachment 168823 [details]
Patch for landing

Got rid of the extra ${_input}
Comment 57 Csaba Osztrogonác 2012-10-16 02:30:19 PDT
We updated all Qt bots, so it is safe to land the patch now.
Comment 58 Pablo Flouret 2012-10-16 10:18:55 PDT
Comment on attachment 168823 [details]
Patch for landing

Tony, feel free to cq+ whenever you think is best. I can keep an eye on the bots to see if any problems come up.
Comment 59 Tony Chang 2012-10-16 11:03:57 PDT
Comment on attachment 168823 [details]
Patch for landing

Clearing flags on attachment: 168823

Committed r131477: <http://trac.webkit.org/changeset/131477>
Comment 60 Pablo Flouret 2012-10-16 12:06:27 PDT
Qt MIPS missing the moc update?
Comment 61 Csaba Osztrogonác 2012-10-16 12:12:01 PDT
(In reply to comment #60)
> Qt MIPS missing the moc update?

Hmm ... it seems yes. Gergely, could you update the Qt 5 on the MIPS bot?
Comment 62 Csaba Osztrogonác 2012-10-16 12:13:05 PDT
(In reply to comment #61)
> (In reply to comment #60)
> > Qt MIPS missing the moc update?
> 
> Hmm ... it seems yes. Gergely, could you update the Qt 5 on the MIPS bot?

FYI: http://lists.webkit.org/pipermail/webkit-qt/2012-October/003182.html
Comment 63 Gergely Kis 2012-10-17 01:31:35 PDT
(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > Qt MIPS missing the moc update?
> > 
> > Hmm ... it seems yes. Gergely, could you update the Qt 5 on the MIPS bot?
> 
> FYI: http://lists.webkit.org/pipermail/webkit-qt/2012-October/003182.html

Working on the update right now, thanks for the heads up.