| Summary: | Report error when #else is used in message receiver generator's input. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gergő Balogh <gbalogh.u-szeged> | ||||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ap, bfulgham, commit-queue, darin, ossy | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 121877 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Gergő Balogh
2013-11-11 07:23:45 PST
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 |