Bug 124147 – Report error when #else is used in message receiver generator's input.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Gergő Balogh]

Report error when #else is used in message receiver generator's input, because it is ignored by the generator script.

[Gergő Balogh]

Created attachment 216573 [details]
patch

[Csaba Osztrogonác]

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.

[Alexey Proskuryakov]

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?

[Gergő Balogh]

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

[Csaba Osztrogonác]

LGTM with the new unittest. WK2 owners?

[Alexey Proskuryakov]

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)?

[Gergő Balogh]

(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)?

[Gergő Balogh]

(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)?

[Alexey Proskuryakov]

> 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?

[Gergő Balogh]

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.

[Csaba Osztrogonác]

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

[Gergő Balogh]

Created attachment 218488 [details]
patch

[WebKit Commit Bot]

Comment on attachment 218488 [details]
patch

Clearing flags on attachment: 218488

Committed r160161: <http://trac.webkit.org/changeset/160161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

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'

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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