Bug 52671 - [chromium] [linux] if check-sys-deps fails, output the failure reason
Summary: [chromium] [linux] if check-sys-deps fails, output the failure reason
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-18 15:02 PST by Tony Chang
Modified: 2011-01-19 10:14 PST (History)
2 users (show)

See Also:


Attachments
Patch (4.48 KB, patch)
2011-01-18 15:03 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2011-01-18 16:15 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (6.62 KB, patch)
2011-01-19 09:57 PST, Tony Chang
mihai: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-01-18 15:02:38 PST
[chromium] [linux] if check-sys-deps fails, output the failure reason
Comment 1 Tony Chang 2011-01-18 15:03:34 PST
Created attachment 79339 [details]
Patch
Comment 2 Tony Chang 2011-01-18 15:05:00 PST
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
Comment 3 Mihai Parparita 2011-01-18 15:20:32 PST
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.
Comment 4 Tony Chang 2011-01-18 16:15:41 PST
Created attachment 79351 [details]
Patch
Comment 5 Tony Chang 2011-01-18 16:25:03 PST
(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.
Comment 6 Mihai Parparita 2011-01-19 07:58:09 PST
(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).
Comment 7 Tony Chang 2011-01-19 09:57:46 PST
Created attachment 79441 [details]
Patch
Comment 8 Tony Chang 2011-01-19 09:58:41 PST
(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.
Comment 9 Tony Chang 2011-01-19 10:14:40 PST
Committed r76134: <http://trac.webkit.org/changeset/76134>