Bug 52671 – [chromium] [linux] if check-sys-deps fails, output the failure reason

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] [linux] if check-sys-deps fails, output the failure reason

[Tony Chang]

Created attachment 79339 [details]
Patch

[Tony Chang]

Previously, if you didn't have all the right fonts installed on Linux, NRWT would fail with the following output:

2011-01-18 12:25:45,331 13914 metered_stream.py:126 INFO Starting helper ...
2011-01-18 12:25:45,331 13914 metered_stream.py:126 INFO Checking system dependencies ...
2011-01-18 12:25:45,542 13914 executive.py:365 DEBUG "/src/chrome/src/out/Release/DumpRenderTree --check-layout-test-sys-deps" took 0.21s
2011-01-18 12:25:45,543 13914 chromium.py:131 ERROR System dependencies check failed.
2011-01-18 12:25:45,543 13914 chromium.py:132 ERROR To override, invoke with --nocheck-sys-deps
2011-01-18 12:25:45,543 13914 chromium.py:133 ERROR 

With this change, we include the actual error text. E.g.,

2011-01-18 12:25:45,543 13914 chromium.py:134 ERROR You are missing /usr/share/fonts/truetype/ttf-dejavu/DejaVuSans.ttf. Try installing msttcorefonts. Also see http://code.google.com/p/chromium/wiki/LinuxBuildInstructions

[Mihai Parparita]

Comment on attachment 79339 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:127
> +        def error_handler(script_error, error=local_error):

Is the error argument necessary (vs. referencing local_error inside directly)? You never actually use it.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:197
> +        class MockExecute:

Call this MockExecutive? Also, we already have webkitpy.common.system.executive_mock.MockExecutive2 and webkitpy.tool.mocktool.MockExecutive, albeit neither one does things with the error_handler.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:215
> +        needs_http = False

Not sure why this necessary (vs. using a named argument)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:220
> +        class MockLog:

webkitpy.common.system.logtesting.LoggingTestCase should let you check that things get logged without having to do your own MockLog class or monkey-patching chromium._log.

[Tony Chang]

Created attachment 79351 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> (From update of attachment 79339 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79339&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:127
> > +        def error_handler(script_error, error=local_error):
> 
> Is the error argument necessary (vs. referencing local_error inside directly)? You never actually use it.

It is necessary because python scoping is weird.  See http://www.python.org/dev/peps/pep-3104/.  There's only global scope and local scope so this allows us to access local_error without making it global.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:197
> > +        class MockExecute:
> 
> Call this MockExecutive? Also, we already have webkitpy.common.system.executive_mock.MockExecutive2 and webkitpy.tool.mocktool.MockExecutive, albeit neither one does things with the error_handler.

I didn't know about those.  Switched to using webkitpy.common.system.executive_mock.MockExecutive2 for this test and test_diff_image.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:215
> > +        needs_http = False
> 
> Not sure why this necessary (vs. using a named argument)

Switched to a named argument.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:220
> > +        class MockLog:
> 
> webkitpy.common.system.logtesting.LoggingTestCase should let you check that things get logged without having to do your own MockLog class or monkey-patching chromium._log.

I didn't know about this either!  Switched to using LoggingTestCase.

[Mihai Parparita]

(In reply to comment #5)
> It is necessary because python scoping is weird.  See http://www.python.org/dev/peps/pep-3104/.  There's only global scope and local scope so this allows us to access local_error without making it global.

That says that Python 2.1 moved closer to static nested scoping. The second make_scoreboard example (with the Namespace class) is basically what you're doing, as far as modifying an outer variable (but not assigning to it, which still doesn't work).

[Tony Chang]

Created attachment 79441 [details]
Patch

[Tony Chang]

(In reply to comment #6)
> (In reply to comment #5)
> > It is necessary because python scoping is weird.  See http://www.python.org/dev/peps/pep-3104/.  There's only global scope and local scope so this allows us to access local_error without making it global.
> 
> That says that Python 2.1 moved closer to static nested scoping. The second make_scoreboard example (with the Namespace class) is basically what you're doing, as far as modifying an outer variable (but not assigning to it, which still doesn't work).

You are absolutely correct!  Sorry for not trying it.  Updated to just access local_error directly.

[Tony Chang]

Committed r76134: <http://trac.webkit.org/changeset/76134>