Bug 98140

Summary: check-webkit-style can't handle qt-arm, qt-win, qt-mac, qt-5.0, qt-5.0-wk1 and qt-5.0-wk2 TestExpecatations
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Critical CC: abarth, dpranke, eric, levin, michael.bruning, ojan, ossy, rafael.lobo, tony, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Csaba Osztrogonác
Reported 2012-10-02 03:46:15 PDT
We got the following non-sense: https://bugs.webkit.org/show_bug.cgi?id=98136#c3 ------------------------------------------------------------------------------------------------------------------------------------- Comment #3 From WebKit Review Bot 2012-10-02 03:39:37 PST (-) [reply] Attachment 166654 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 LayoutTests/platform/qt-5.0-wk2/TestExpectations:1: No port uses path LayoutTests/platform/qt-5.0-wk2/TestExpectations for test_expectations [test/expectations] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. ------------------------------------------------------------------------------------------------------------------------------------- Have you got any idea what happened with the check-webkit-style?
Attachments
Patch (4.25 KB, patch)
2012-10-02 14:37 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Michael Brüning
Comment 1 2012-10-02 04:15:56 PDT
Interesting enough, it was still able to handle it a day (a couple of hours) earlier (see https://bugs.webkit.org/show_bug.cgi?id=98037).
Csaba Osztrogonác
Comment 2 2012-10-02 04:17:20 PDT
(In reply to comment #1) > Interesting enough, it was still able to handle it a day (a couple of hours) earlier (see https://bugs.webkit.org/show_bug.cgi?id=98037). Hmmm ... let me check the history and try bisecting.
Csaba Osztrogonác
Comment 3 2012-10-02 04:25:06 PDT
bisecting finished - https://trac.webkit.org/changeset/127910 Hmmm ... it seems it can be a very old bug ... But I don't have time to find who caused this annoying bug. :-/
Csaba Osztrogonác
Comment 4 2012-10-02 04:30:29 PDT
It seems it handles only qt and qt-linux ... :-/
Tony Chang
Comment 5 2012-10-02 14:37:56 PDT
Created attachment 166752 [details] Patch
Tony Chang
Comment 6 2012-10-02 14:40:10 PDT
I fixed one bug and suppressed another. The problem is that when we use port_factory.all_port_names(), it doesn't know to return both qt-4.8 and qt-5.0 versions. I'm not sure what the right way to fix this is, so I'm skipping style check (the check is doing more harm than good).
Eric Seidel (no email)
Comment 7 2012-10-02 14:41:09 PDT
Comment on attachment 166752 [details] Patch Seems OK. I found qt's layout test fallback confusing (and different from all the other ports) so I didn't implement support for it in the original NRWT conversion. I (perhaps naively) hoped that they'd move to a naming system/fallback more similar to the other ports. But it's also possible I simply mis-understood. In any case, someone just needs to implement proper Qt port fallback for NRWT and then we can remove qt-arm's ORWT usage, and fix the bugs like this one. :)
Eric Seidel (no email)
Comment 8 2012-10-02 14:42:32 PDT
(In reply to comment #6) > I fixed one bug and suppressed another. The problem is that when we use port_factory.all_port_names(), it doesn't know to return both qt-4.8 and qt-5.0 versions. I'm not sure what the right way to fix this is, so I'm skipping style check (the check is doing more harm than good). Presumably those are a lot like mac-leopard/mac-lion, etc. If Qt uses the same style of fallback logic then we can just copy the logic from ApplePort/MacPort/WinPort or move it down to WebKitPort/Port for sharing.
Rafael Brandao
Comment 9 2012-10-02 14:52:56 PDT
I believe the only reliable solution to figure out which version to useis to pass executor around and use "qmake --version" whenever you need to figure out the version, like we do on Tools/Scripts/webkitpy/layout_tests/port/qt.py (check qt_version()), but when I did it back there I felt like it was the wrong approach. Should we do this here as well?
Tony Chang
Comment 10 2012-10-02 15:09:50 PDT
(In reply to comment #8) > Presumably those are a lot like mac-leopard/mac-lion, etc. If Qt uses the same style of fallback logic then we can just copy the logic from ApplePort/MacPort/WinPort or move it down to WebKitPort/Port for sharing. Yeah, there are 2 things to fix: 1) Make qt.py's __init__ know how to parse something like qt-5.0-mac (it currently doesn't know how). 2) Update builders.py to specify qt versions (currently it just has qt-linux, qt-mac, qt-win and qt-wk2). (In reply to comment #9) > I believe the only reliable solution to figure out which version to useis to pass executor around and use "qmake --version" whenever you need to figure out the version, like we do on Tools/Scripts/webkitpy/layout_tests/port/qt.py (check qt_version()), but when I did it back there I felt like it was the wrong approach. Should we do this here as well? That's what it's doing, but the EWS bots have Qt 4.8 installed (actually, probably no Qt, but that falls back to 4.8), but wants to check the TestExpectations file for Qt 5.0. There's currently no way to override the Qt version when constructing a QtPort().
WebKit Review Bot
Comment 11 2012-10-02 15:29:29 PDT
Comment on attachment 166752 [details] Patch Clearing flags on attachment: 166752 Committed r130220: <http://trac.webkit.org/changeset/130220>
WebKit Review Bot
Comment 12 2012-10-02 15:29:33 PDT
All reviewed patches have been landed. Closing bug.
Dirk Pranke
Comment 13 2012-10-02 15:58:17 PDT
Note that there are several open Qt/webkitpy/nrwt-related bugs still open that relate to this, tracked here: https://bugs.webkit.org/show_bug.cgi?id=89880.
Note You need to log in before you can comment on or make changes to this bug.