Bug 64071 – new-run-webkit-tests does not support qt-4.8 results

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

new-run-webkit-tests does not support qt-arm or qt-4.8 results

We've not added support yet.  It's not clear if these results are even in use.  I don't see any bots on build.webkit.org

It's unclear if "arm" is both the architecture and the OS here?  Should that really be qt-linux-arm?

And what about qt-4.8?  Is that qt-4.8-linux?  Or do those results match on all platforms with 4.8?

[Eric Seidel (no email)]

I believe the fallback orders in ORWT are:

qt-arm, qt
qt-4.8, qt

But I suspect those aren't really right.

From my investigation it appears that the qt-mac results aren't really used either.  They're certainly very out of date.

[Csaba Osztrogonác]

(In reply to comment #1)
> I believe the fallback orders in ORWT are:
> 
> qt-arm, qt
> qt-4.8, qt
> 
> But I suspect those aren't really right.
> 
> From my investigation it appears that the qt-mac results aren't really used either.  They're certainly very out of date.

Now qt, qt-arm, qt-4.8 and qt-mac platforms are used by buildbots run 
on http://build.webkit.sed.hu/waterfall . qt-linux and qt-win platforms
aren't really used.

The reference OS for Qt port is Linux, so all qt-* platforms should
fallback to qt platform. qt-4.8 is for development, result must be
same as on qt (4.7) platform when Qt-4.8 is released. AFAIK theses
bugs are fixed in Qt trunk, but I'm going to check it in 1-2 days.

On qt-mac platform we only use Skipped list now, because we haven't
human resource to maintain expected files day by day. We always add
failing tests to this skipped list with changeset number.

I think making NRWT work with these platforms isn't priority for us now,
because there are major problems with NRWT for Qt port now. But Szeged team 
will fix this bug later.

[Balazs Kelemen]

> I think making NRWT work with these platforms isn't priority for us now,
> because there are major problems with NRWT for Qt port now. But Szeged team 
> will fix this bug later.

What are those major problems exactly? I think it is a priority for us to use the test infrastructure that has become the official one for the WebKit project.

[Csaba Osztrogonác]

I disabled NRWT for qt-arm and qt-4.8 platforms until fix:
http://trac.webkit.org/changeset/90746

Gábor started working on this bug.

[Eric Seidel (no email)]

It's easy to support these two platforms.  But it would make the logic for generating these names cleaner if the platform directories were renamed to include the OS, so:

qt-4.8-linux
qt-linux-arm

Which fit a more easy generated pattern of:

PORT-VERSION-OS
and
PORT-OS-ARCH

Where both OS and ARCH are optional.

In either case it doesn't matter much, but we'll need to special case the fallback.

chromium-linux-x86 definitely follows the PORT-OS-ARCH pattern, I don't really see any other ports which have a VERSION for their port, but not their OS.  (mac-leopard, win-xp are 2 of many examples of using OSVERSION in the names).

This is all part of our continuing effort to simplify our fallback rules.  qt-4.8 and qt-arm didn't easily fit any existing fallback rules, so I didn't add support for them in the first-pass implementation of the QtPort for NRWT.

[Eric Seidel (no email)]

(In reply to comment #2)
> Now qt, qt-arm, qt-4.8 and qt-mac platforms are used by buildbots run 
> on http://build.webkit.sed.hu/waterfall . qt-linux and qt-win platforms
> aren't really used.

Can we delete qt-linux and qt-win then?

[Eric Seidel (no email)]

(In reply to comment #2)
> The reference OS for Qt port is Linux, so all qt-* platforms should
> fallback to qt platform. qt-4.8 is for development, result must be
> same as on qt (4.7) platform when Qt-4.8 is released. AFAIK theses
> bugs are fixed in Qt trunk, but I'm going to check it in 1-2 days.

The Apple folks have historically used "mac" to mean the "Development version" and specific numbered versions for older versions.

I think Qt should consider doing similar (which means that some of the 4.8 results should move into qt/ and some of the 4.7-specific results in qt/ should move up into a qt-4.7.

[Csaba Osztrogonác]

Now the baseline search path on qt-wk platform: qt-wk2 -> qt-linux -> qt -> generic

It should be: qt-wk2 -> ((qt-linux ->)) qt-4.8 -> qt -> generic

If it is fixed, we can enable ~400 tests on qt-wk2 platform.

[Csaba Osztrogonác]

(In reply to comment #6)
> Can we delete qt-linux and qt-win then?

Not yet, they might be used in the future.

[Csaba Osztrogonác]

(In reply to comment #7)
> (In reply to comment #2)
> > The reference OS for Qt port is Linux, so all qt-* platforms should
> > fallback to qt platform. qt-4.8 is for development, result must be
> > same as on qt (4.7) platform when Qt-4.8 is released. AFAIK theses
> > bugs are fixed in Qt trunk, but I'm going to check it in 1-2 days.
> 
> The Apple folks have historically used "mac" to mean the "Development version" and specific numbered versions for older versions.
> 
> I think Qt should consider doing similar (which means that some of the 4.8 results should move into qt/ and some of the 4.7-specific results in qt/ should move up into a qt-4.7.

Now the Qt 4.7 is the default, and 4.8 exceptions are stored in qt-4.8. But after 4.8 is released we will merge qt-4.8 to qt and after then we can remove qt-4.8 platform. (Because qt will means Qt 4.8 and 4.7 won't be supported anymore.)

[Csaba Osztrogonác]

(In reply to comment #8)
> Now the baseline search path on qt-wk platform: qt-wk2 -> qt-linux -> qt -> generic
> 
> It should be: qt-wk2 -> ((qt-linux ->)) qt-4.8 -> qt -> generic
> 
> If it is fixed, we can enable ~400 tests on qt-wk2 platform.

I forgot to mention why we need this search path: Because to build WebKit2, the minimum Qt version is 5.0 (which based on 4.8).

[Eric Seidel (no email)]

It sounds like your troubles could very easily be fixed by moving qt-4.8 to qt and qt to qt-4.7, right?

[Eric Seidel (no email)]

The reason why I propose moving instead of hacking the tool is that there are long-term benefits to every project using similar fallback systems.

We would still need to add 4.7 as an explicit version number (I think), but otherwise NRWT should just work "out of the box" including getting the wk2 fallback path correctly then, right?

[Csaba Osztrogonác]

(In reply to comment #13)
> We would still need to add 4.7 as an explicit version number (I think), but otherwise NRWT should just work "out of the box" including getting the wk2 fallback path correctly then, right?

Do you mean we should add a qt-4.7 platform too?
And then the search paths would be in general: 
qt-wk2|none -> qt-(linux|win|mac) -> qt-(4.7|4.8) -> generic

examples:
qt-wk2 -> qt-linux -> qt-4.8 -> qt -> generic
          qt-linux -> qt-4.8 -> qt -> generic
          qt-linux -> qt-4.7 -> qt -> generic

It sounds good to me. Now wk2 won't depends on Qt version. (You can't build it with Qt 4.7, only Qt 5.0, so version will be qt-48 always, because qt-4.7 == Qt-4.7.x and qt-4.8 == Qt-4.8.x or newer)

We only need to add a Qt version checker function similar to in webkitdirs.pm:
sub getQtVersion()
{
    my $qtVersion = `$qmakebin --version`;
    $qtVersion =~ s/^(.*)Qt version (\d\.\d)(.*)/$2/s ;
    return $qtVersion;
}

[Csaba Osztrogonác]

Removing qt-arm, and filed a new bug report about it: https://bugs.webkit.org/show_bug.cgi?id=67777

[Eric Seidel (no email)]

(In reply to comment #14)
> (In reply to comment #13)
> > We would still need to add 4.7 as an explicit version number (I think), but otherwise NRWT should just work "out of the box" including getting the wk2 fallback path correctly then, right?
> 
> Do you mean we should add a qt-4.7 platform too?
> And then the search paths would be in general: 
> qt-wk2|none -> qt-(linux|win|mac) -> qt-(4.7|4.8) -> generic
> 
> examples:
> qt-wk2 -> qt-linux -> qt-4.8 -> qt -> generic
>           qt-linux -> qt-4.8 -> qt -> generic
>           qt-linux -> qt-4.7 -> qt -> generic

No, in order to match the other WebKit ports, the search paths would need to be:

qt-wk2 -> qt -> qt-4.8 -> qt-4.7 -> generic

That matches mac/win:
mac-wk2 -> mac -> mac-lion -> mac-snow-leopard -> mac-leopard -> generic
win-wk2 -> win -> win-win7sp0 -> win-xp -> generic

I'm not sure where qt-linux vs. qt-mac, etc. falls in.  We should look at how Chromium does their fallback.

[Eric Seidel (no email)]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > We would still need to add 4.7 as an explicit version number (I think), but otherwise NRWT should just work "out of the box" including getting the wk2 fallback path correctly then, right?
> > 
> > Do you mean we should add a qt-4.7 platform too?
> > And then the search paths would be in general: 
> > qt-wk2|none -> qt-(linux|win|mac) -> qt-(4.7|4.8) -> generic
> > 
> > examples:
> > qt-wk2 -> qt-linux -> qt-4.8 -> qt -> generic
> >           qt-linux -> qt-4.8 -> qt -> generic
> >           qt-linux -> qt-4.7 -> qt -> generic
> 
> No, in order to match the other WebKit ports, the search paths would need to be:
> 
> qt-wk2 -> qt -> qt-4.8 -> qt-4.7 -> generic
> 
> That matches mac/win:
> mac-wk2 -> mac -> mac-lion -> mac-snow-leopard -> mac-leopard -> generic
> win-wk2 -> win -> win-win7sp0 -> win-xp -> generic
> 
> I'm not sure where qt-linux vs. qt-mac, etc. falls in.  We should look at how Chromium does their fallback.

Sorry, my comment was just wrong.  I guess I should wake up more before commenting on bugs.

Mac/Win work like this:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/mac_unittest.py#L128
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py#L78

aka:

mac-wk2 -> mac-lion -> mac-snow-leopard ... -> mac

which is closer to what you said than what I said, with the exception of where you placed qt-4.7

[Kristóf Kosztyó]

Created attachment 108797 [details]
proposed fix

[Eric Seidel (no email)]

Comment on attachment 108797 [details]
proposed fix

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

It's not immediately clear what the fallback path looks like here.  But it seems that 4.8 results should fallback on 4.7 results, or vice versa, similar to how Mac/Win work.

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:93
> +    def get_qt_version(self):

I might instead make this qt_version(self) and have it be @memoized.  We don't usually use "get" in function names in WebKit.

[Csaba Osztrogonác]

Comment on attachment 108797 [details]
proposed fix

r- now, because you missed to add the unit test change.

[Csaba Osztrogonác]

(In reply to comment #19)
> (From update of attachment 108797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108797&action=review
> 
> It's not immediately clear what the fallback path looks like here.  But it seems that 4.8 results should fallback on 4.7 results, or vice versa, similar to how Mac/Win work.

Absolutely no. qt-4.8 shouldn't fallback to qt-4.7 or vice versa. qt-4.7 and qt-4.8 should fallback to qt and then generic. qt-4.7 and qt-4.8 doesn't depends on each other. There are failing tests on qt-4.7 which pass on qt-4.8 and vice-versa.

 
> > Tools/Scripts/webkitpy/layout_tests/port/qt.py:93
> > +    def get_qt_version(self):
> 
> I might instead make this qt_version(self) and have it be @memoized.  We don't usually use "get" in function names in WebKit.

Good point.

[Adam Barth]

You're welcome to have whatever fallback path you like, but the common way we do fallbacks in WebKit is to have older versions fall back to newer versions.  For example, Leopard falls back to Snow Leopard, which falls back to Lion.  Here's a diagram I drew a while back:

https://docs.google.com/drawings/d/1z65SKkWrD4Slm6jobIphHwwRADyUtjOAxwGBVKBY8Kc/edit?hl=en_US

I agree that it sounds goofy, but it seems to work pretty well.

[Kristóf Kosztyó]

Created attachment 109006 [details]
proposed fix

[Csaba Osztrogonác]

Comment on attachment 109006 [details]
proposed fix

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

The development of Qt was faster than this patch's. :) So please update the patch. 

Now qt-wk2 should fallback to a new qt-5.0 platform. ( After Qt 4.8 will be releases,
we are going to stop supporting qt-4.7 test results and then we're going to merge
qt-4.8 results to general qt platform. But it is a mid-term plan.) Otherwise the patch LGTM.

Is there any objection against landing the fixed patch?

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:33
>  import logging
>  import sys
> +import re

style: alphabetic order

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:111
> +        if '4.7' in version:
> +            search_paths.append('qt-4.7')
> +        elif version:
> +            search_paths.append('qt-4.8')

if '4.7' in version:
    search_paths.append('qt-4.7')
elif '4.8' in version:
    search_paths.append('qt-4.8')
elif version:
    search_paths.append('qt-5.0')

> Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:36
>  from webkitpy.layout_tests.port.qt import QtPort
>  from webkitpy.layout_tests.port import port_testcase
>  from webkitpy.tool.mocktool import MockOptions, MockUser, MockExecutive
> +from webkitpy.common.system.executive_mock import MockExecutive2

style: alphabetic order

> Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:59
> +            return 'QMake version 2.01a\nUsing Qt version 4.8.2 in /usr/local/Trolltech/Qt-4.8.2/lib'

nit: s/4.8.2/4.8.0/g

> Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:78
> +        self._assert_search_path(['qt-wk2', 'qt-mac', 'qt-4.8', 'qt'], 'darwin', use_webkit2=True, qt_version='5.0')
> +        self._assert_search_path(['qt-wk2', 'qt-win', 'qt-4.8', 'qt'], 'cygwin', use_webkit2=True, qt_version='5.0')
> +        self._assert_search_path(['qt-wk2', 'qt-linux', 'qt-4.8', 'qt'], 'linux2', use_webkit2=True, qt_version='5.0')

s/qt-4.8/qt-5.0/g

[Kristóf Kosztyó]

Created attachment 109951 [details]
updated fallback path

[Csaba Osztrogonác]

Comment on attachment 109951 [details]
updated fallback path

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

In general LGTM, but please fix these little things.

> Tools/ChangeLog:9
> +        * Scripts/webkitpy/layout_tests/port/qt_unittest.py: Removed.

Removed ????

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:93
> +    def qt_version(self):

@memoized is still missing

> Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:59
> +            return 'QMake version 2.01a\nUsing Qt version 4.8.0 in /usr/local/Trolltech/Qt-4.8.2/lib'

one more s/4.8.2/4.8.0 :)

[Kristóf Kosztyó]

(In reply to comment #26)
> (From update of attachment 109951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109951&action=review
> 
> In general LGTM, but please fix these little things.
> 
> > Tools/ChangeLog:9
> > +        * Scripts/webkitpy/layout_tests/port/qt_unittest.py: Removed.
> 
> Removed ????
> 

Uups, I had a little problem with the git.
One moment, and I will fix it.

[Kristóf Kosztyó]

Created attachment 109960 [details]
proposed fix

[Csaba Osztrogonác]

Comment on attachment 109960 [details]
proposed fix

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

I tested your patch, but I found one more thing. Skipped 
file in qt-<version> directory is ignored, but we need it.

Could you override _skipped_file_search_paths too?

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:93
> +    def qt_version(self):

@memoized is still missing :-/

[Kristóf Kosztyó]

Created attachment 110135 [details]
proposed fix

I created also the Skipped files for qt-4.7 and qt-5.0

[Csaba Osztrogonác]

Comment on attachment 110135 [details]
proposed fix

r=me

[Csaba Osztrogonác]

Comment on attachment 110135 [details]
proposed fix

Landed in http://trac.webkit.org/changeset/97252

[Adam Barth]

http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/2541/steps/webkitpy-test/logs/stdio

You can't call out to qmake when determining the baseline paths:

    for line in self._executive.run_command(['qmake', '-v']).split('\n'):

We need to be able to determine the baseline paths on all systems, even those without qmake.  Mac and Win also need to solve this problem.  It's probably a good idea to follow their approach.

[Csaba Osztrogonác]

We can't determine baseline path without calling qmake ... :((

[Adam Barth]

> We can't determine baseline path without calling qmake ... :((

Can you explain why in more detail?  I understand that you might not be able to determine which baseline path you'd like to use without calling qmake (or otherwise inspecting the environment), but you should be able to determine the universe of possible baseline paths without reading any information from the environment.

Put another way, we have a requirement that you can enumerate the universe of baseline paths for all ports when running on any platform.  For example, on a Mac without qmake installed, you should be able to know where the Qt port stores its baselines and what the fallback behavior is between them.

[Csaba Osztrogonác]

Fixed patch landed in http://trac.webkit.org/changeset/97461.
It wasn't the best solution, but this patch was very important for us.

I filed a new bug to fix Qt port for baselineoptimizer: https://bugs.webkit.org/show_bug.cgi?id=70108