Bug 124147

Created attachment 216573 [details] patch

I don't think if we will need #else in the message files, but warning the user is better than simple ignoring the #else. The patch looks good to me, but we need a WK2 owner review.

Comment on attachment 216573 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=216573&action=review > Source/WebKit2/ChangeLog:8 > + * Scripts/webkit2/parser.py: Can this be tested with a webkitpy regression test?

Created attachment 217986 [details] patch the style check is crash on Source/WebKit2/Scripts/webkit2/messages_unittest.py even it is well formated (my part anyway) and working

LGTM with the new unittest. WK2 owners?

Comment on attachment 217986 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=217986&action=review This change is good, please feel free to review and land as appropriate. > Source/WebKit2/Scripts/webkit2/messages_unittest.py:1130 > + with self.assertRaisesRegexp(Exception, r"ERROR: '#else.*' are not supported in the \*\.in files"): Where does this ".*" come from? How is it helpful? > Source/WebKit2/Scripts/webkit2/parser.py:68 > + raise Exception("ERROR: '%s' are not supported in the *.in files" % trimmed) Shouldn't it be "... is not supported in .in files" (no "the", and singular)?

(In reply to comment #6) > (From update of attachment 217986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217986&action=review > > This change is good, please feel free to review and land as appropriate. > > > Source/WebKit2/Scripts/webkit2/messages_unittest.py:1130 > > + with self.assertRaisesRegexp(Exception, r"ERROR: '#else.*' are not supported in the \*\.in files"): > > Where does this ".*" come from? How is it helpful? I use to follow the practise, that I give as much information as possible in an error message. Maybe it is irrelevant in the case of 'else', but I think if somebody make a mistake (for example 'else if' instead of 'elif') it will be nice to show the whole line, to help identify and correct it. > > > Source/WebKit2/Scripts/webkit2/parser.py:68 > > + raise Exception("ERROR: '%s' are not supported in the *.in files" % trimmed) > > Shouldn't it be "... is not supported in .in files" (no "the", and singular)?

(In reply to comment #6) > (From update of attachment 217986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217986&action=review > > This change is good, please feel free to review and land as appropriate. Could you please set r+ and c+ for it as a WebKit2 owner? > > > Source/WebKit2/Scripts/webkit2/messages_unittest.py:1130 > > + with self.assertRaisesRegexp(Exception, r"ERROR: '#else.*' are not supported in the \*\.in files"): > > Where does this ".*" come from? How is it helpful? > > > Source/WebKit2/Scripts/webkit2/parser.py:68 > > + raise Exception("ERROR: '%s' are not supported in the *.in files" % trimmed) > > Shouldn't it be "... is not supported in .in files" (no "the", and singular)?

> Could you please set r+ and c+ for it as a WebKit2 owner? You don't need an owner for that, any reviewer can do final review and set r+ with approval from an owner. Are you planning to address the comments though?

Created attachment 218390 [details] patch fix > Are you planning to address the comments though? Yes. are -> is The answer for the other one see above.

Comment on attachment 218390 [details] patch fix View in context: https://bugs.webkit.org/attachment.cgi?id=218390&action=review > Source/WebKit2/Scripts/webkit2/parser.py:68 > + raise Exception("ERROR: '%s' are not supported in the *.in files" % trimmed) are -> is here too

Created attachment 218488 [details] patch

Comment on attachment 218488 [details] patch Clearing flags on attachment: 218488 Committed r160161: <http://trac.webkit.org/changeset/160161>

All reviewed patches have been landed. Closing bug.

This change breaks the Windows build: Running the tests ... [4/1455] webkit2.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_elif erred: Traceback (most recent call last): File "/home/buildbot/slave/win-debug-tests/build/Source/WebKit2/Scripts/webkit2/messages_unittest.py", line 1135, in test_error_at_elif with self.assertRaisesRegexp(Exception, r"ERROR: '#elif.*' is not supported in the \*\.in files"): AttributeError: 'UnsupportedPrecompilerDirectiveTest' object has no attribute 'assertRaisesRegexp' [5/1455] webkit2.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_else erred: Traceback (most recent call last): File "/home/buildbot/slave/win-debug-tests/build/Source/WebKit2/Scripts/webkit2/messages_unittest.py", line 1131, in test_error_at_else with self.assertRaisesRegexp(Exception, r"ERROR: '#else.*' is not supported in the \*\.in files"): AttributeError: 'UnsupportedPrecompilerDirectiveTest' object has no attribute 'assertRaisesRegexp'

(In reply to comment #15) > This change breaks the Windows build: > > Running the tests ... > [4/1455] webkit2.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_elif erred: > Traceback (most recent call last): > File "/home/buildbot/slave/win-debug-tests/build/Source/WebKit2/Scripts/webkit2/messages_unittest.py", line 1135, in test_error_at_elif > with self.assertRaisesRegexp(Exception, r"ERROR: '#elif.*' is not supported in the \*\.in files"): > AttributeError: 'UnsupportedPrecompilerDirectiveTest' object has no attribute 'assertRaisesRegexp' > > [5/1455] webkit2.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_else erred: > Traceback (most recent call last): > File "/home/buildbot/slave/win-debug-tests/build/Source/WebKit2/Scripts/webkit2/messages_unittest.py", line 1131, in test_error_at_else > with self.assertRaisesRegexp(Exception, r"ERROR: '#else.*' is not supported in the \*\.in files"): > AttributeError: 'UnsupportedPrecompilerDirectiveTest' object has no attribute 'assertRaisesRegexp' Thanks for noticing this failure. But I think it isn't a serious problem, because not the build is broken, only two webkitpy unittest fail. And they fail because the Windows bots have older than 2.7 python. (which was released ~3.5 years before) Additionally it was a WebKit2 patch and there is no WebKit2 on Windows long time ago. I think we can fix this problem in 3 different way: - install newer python to Windows buildbots - disable this WebKit2 unittest, because there is no WebKit2 on Windows - use similar workaround to Tools/Scripts/webkitpy/common/webkitunittest.py

new bug report to disable wk2-webkitpy unittests on Windows: https://bugs.webkit.org/show_bug.cgi?id=125318