Bug 72748 - NRWT fails on unreleased versions of Mac OS X
Summary: NRWT fails on unreleased versions of Mac OS X
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
: 73927 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-11-18 12:04 PST by Adam Roben (:aroben)
Modified: 2011-12-06 14:37 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.54 KB, patch)
2011-12-06 11:31 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (5.85 KB, patch)
2011-12-06 11:38 PST, Adam Barth
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-11-18 12:04:38 PST
When I run NRWT on an unreleased version of Mac OS X, I see this:

Failed to open Skipped file: /Users/aroben/dev/WebKit/OpenSource/LayoutTests/platform/mac-future/Skipped
Starting 1 worker ...Process _Process-1:
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/multiprocessing/process.py", line 232, in _bootstrap
    self.run()
  File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 259, in run
    port_obj = host.port_factory.get(self._platform_name, options)
  File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/factory.py", line 147, in get
    return self._get_kwargs(**kwargs)
  File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/factory.py", line 127, in _get_kwargs
    return maker(host, **kwargs)
  File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/mac.py", line 78, in __init__
    ApplePort.__init__(self, host, **kwargs)
  File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/apple.py", line 73, in __init__
    assert port_name in self.VERSION_FALLBACK_ORDER, "%s is not in %s" % (port_name, self.VERSION_FALLBACK_ORDER)
AssertionError: mac-future is not in ['mac-leopard', 'mac-snowleopard', 'mac-lion', 'mac']
Starting testing ... 

And then it just hangs there forever.
Comment 1 Dirk Pranke 2011-11-18 12:13:12 PST
bah. Looks like this broke with some recent refactoring and we don't have a test for it?
Comment 2 Eric Seidel (no email) 2011-11-18 12:14:47 PST
Yeah, I'm sure I broke it.
Comment 3 Adam Roben (:aroben) 2011-12-06 09:37:15 PST
*** Bug 73927 has been marked as a duplicate of this bug. ***
Comment 4 Darin Adler 2011-12-06 10:09:27 PST
Someone teach me how to fix this. It’s blocking a lot of work on WebKit at Apple.
Comment 5 Adam Barth 2011-12-06 10:30:57 PST
The proximate fix is to add the string mac-future to self.VERSION_FALLBACK_ORDER in apple.py.  I'm not sure what else needs to be done.  I'm on #webkit and happy to help.
Comment 6 Adam Barth 2011-12-06 10:33:07 PST
From Bug 73927:

> I get this on my newfangled post-Lion computer when I use run-webkit-tests.
> 
> Failed to open Skipped file: /Volumes/Home/darin/Safari/OpenSource/LayoutTests/platform/mac-future/Skipped
> 
> This doesn’t seem right. While there might be “mac-future” as an internal version inside the scripts, we have no intention of actually creating mac-future directories. Is this working as designed?

It sounds like we need to teach the tool to look for the mac-future skipped list somewhere else, presumably in LayoutTests/platform/mac/Skipped?
Comment 7 Darin Adler 2011-12-06 10:40:15 PST
(In reply to comment #6)
> It sounds like we need to teach the tool to look for the mac-future skipped list somewhere else, presumably in LayoutTests/platform/mac/Skipped?

That sounds right. Here is the design concept:

The directory platform/mac represents Mac in the future and the other directories such as platform/mac-lion mean “things that are different in Lion than the future” and platform/mac-snowleopard means “things that are different in SnowLeopard than Lion”.

Skipped files are an anomaly here; they are independent rather than built on top of each other as deltas, but that is a design mistake. The Skipped file in each directory should be a delta from the Skipped file from below. There should be a master Skipped file that is not platform specific, and then each directory should modify that one.

I’d expect something similar for the Chrome-style expected results file too. And I’d also like to merge the Skipped concept and the expected results concept into a single file.
Comment 8 Adam Barth 2011-12-06 10:45:44 PST
I'm trying to figure out how I can reproduce this issue on a Lion machine by haxoring the version detection logic.
Comment 9 Adam Barth 2011-12-06 10:52:44 PST
I'm developing a theory.  That message is just a warning, by the way.
Comment 10 Adam Barth 2011-12-06 10:54:33 PST
Ok.  I see the bug.  Should be pretty easy to fix.  I need to do an Apple-Mac build to be sure.  That's going to take about 22 minutes.  I hope to have a patch to most here in about 45 minutes.
Comment 11 Adam Barth 2011-12-06 11:31:49 PST
Created attachment 118078 [details]
Patch
Comment 12 Adam Roben (:aroben) 2011-12-06 11:33:47 PST
Comment on attachment 118078 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:79
> -        ApplePort.__init__(self, host, **kwargs)
>          self._operating_system = 'mac'
> +        ApplePort.__init__(self, host, **kwargs)

This change didn't make it into your ChangeLog.

> Tools/Scripts/webkitpy/layout_tests/port/win.py:79
> -        ApplePort.__init__(self, host, **kwargs)
>          self._operating_system = 'win'
> +        ApplePort.__init__(self, host, **kwargs)

Nor did this one.
Comment 13 Adam Barth 2011-12-06 11:34:54 PST
> This change didn't make it into your ChangeLog.

Oops.  I revised the patch after writing the ChangeLog.  Will fix.
Comment 14 Adam Barth 2011-12-06 11:38:09 PST
Created attachment 118080 [details]
Patch
Comment 15 Dirk Pranke 2011-12-06 11:48:03 PST
Comment on attachment 118080 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:354
> +                _log.debug("Skipped does not exist: %s" % filename)

It seems like skipped_file_search_paths() shouldn't be returning a directory that doesn't exist for mac-future. Can you add a fixme or file a separate bug for that? 

As an aside, I'm not sure what the difference between self.port_name and self.name() is supposed to be; I can't remember if I added that or someone else did, but it seems like the two names are confusing so we should probably clean that up at some point as well.
Comment 16 Adam Barth 2011-12-06 11:57:53 PST
> It seems like skipped_file_search_paths() shouldn't be returning a directory that doesn't exist for mac-future. Can you add a fixme or file a separate bug for that? 

Will do.

> As an aside, I'm not sure what the difference between self.port_name and self.name() is supposed to be; I can't remember if I added that or someone else did, but it seems like the two names are confusing so we should probably clean that up at some point as well.

That puzzled me as well.  It seems like a good longer term direction for this code is to separate out the parsing of port names, the state representing which port we're talking about, and the port-specific behaviors.  At the moment, they're fairly tightly bound together.
Comment 17 Dirk Pranke 2011-12-06 11:59:32 PST
(In reply to comment #16)
> That puzzled me as well.  It seems like a good longer term direction for this code is to separate out the parsing of port names, the state representing which port we're talking about, and the port-specific behaviors.  At the moment, they're fairly tightly bound together.

Agreed. Every time I've tried to do this thus far it's devolved into a rewrite-the-world-at-once patch for everything under layout_tests/port/*, but I'm due to try it again.
Comment 18 Adam Barth 2011-12-06 12:00:10 PST
Committed r102161: <http://trac.webkit.org/changeset/102161>
Comment 19 Eric Seidel (no email) 2011-12-06 14:14:05 PST
Comment on attachment 118080 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:354
>> +                _log.debug("Skipped does not exist: %s" % filename)
> 
> It seems like skipped_file_search_paths() shouldn't be returning a directory that doesn't exist for mac-future. Can you add a fixme or file a separate bug for that? 
> 
> As an aside, I'm not sure what the difference between self.port_name and self.name() is supposed to be; I can't remember if I added that or someone else did, but it seems like the two names are confusing so we should probably clean that up at some point as well.

name() is the random, port-specific string which gets passed in and is parsed with different rules per-port.
port_name is supposed to be the root name of the port, like "mac", "win", "chromium", "qt", "gtk", etc.
It was a somewhat failed attempt at preparing the Port clases for moving "name" parsing out of the Port constructors (into Factory or elsewhere).
Comment 20 Adam Barth 2011-12-06 14:37:31 PST
At least in some places port_name seems to include the version.  For example, the port_name argument to __init__ can include the version suffix.