Bug 81028 - nrwt: run with no args on lion, is putting baselines in platform/mac-lion, not platform/mac
Summary: nrwt: run with no args on lion, is putting baselines in platform/mac-lion, no...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-13 13:20 PDT by Dirk Pranke
Modified: 2012-03-14 11:53 PDT (History)
12 users (show)

See Also:


Attachments
Patch (8.26 KB, patch)
2012-03-13 17:17 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
don't hardcode mac-lion (8.48 KB, patch)
2012-03-13 18:59 PDT, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-03-13 13:20:42 PDT
mac-lion shouldn't be automatically used for baselines until Mountain Lion ships and the apple guys decide that 'mac' == 'mountain lion'. platform/mac-lion should be included in the search path for baseline retrieval, but we should only be able to put new baselines there by hand (explicitly moving them from platform/mac by those who know if the baseline is different between Lion and Mountain Lion.
Comment 1 Dirk Pranke 2012-03-13 13:21:56 PDT
Looks like we broke this either in r92882 or r105951.
Comment 2 Alexey Proskuryakov 2012-03-13 13:26:43 PDT
We do not intend to put any results in mac-lion by hand until Mountain Lion ships.
Comment 3 Dirk Pranke 2012-03-13 15:42:49 PDT
(In reply to comment #2)
> We do not intend to put any results in mac-lion by hand until Mountain Lion ships.

Does that mean that you don't need there to be a mac-lion directory at all?
Comment 4 Dirk Pranke 2012-03-13 15:49:58 PDT
The last time I discussed this (w/ Darin, I believe), I thought there was a need for apple employees to be able to distinguish between 'mac-future' and 'mac-lion' for some period of time prior to Mountain Lion actually shipping (technically, it was Lion at the time).

It may have been that the only thing in mac-lion should be a Skipped file?

I think the most recent discussion of this was in https://bugs.webkit.org/show_bug.cgi?id=72748 .

cf. also http://trac.webkit.org/wiki/LayoutTestsSearchPath ... 

If there will not be any baselines in mac-lion until ML ships, that simplifies the code slightly (mac-lion doesn't need to be in the baseline path for reads or writes, not just writes), which is why I am asking ...
Comment 5 Dirk Pranke 2012-03-13 17:17:47 PDT
Created attachment 131757 [details]
Patch
Comment 6 Dirk Pranke 2012-03-13 17:23:14 PDT
ap, darin: note the tests in test_skipped_file_search_paths() and test_baseline_search_path() and make sure that they are what you'd like the behavior to be (hopefully the tests are close enough to english that their intent will be clear). If they aren't, let me know and I'll be happy to change them.

In English, my expectations are:

by default, on lion (platform mac-lion), we will look for baselines in mac-lion and then mac, but write new baselines into mac

'mac-future' will only read from 'mac' and will write to 'mac'.

Nothing should ever be written to 'platform/mac-future'. There is no 'platform/mac-future/Skipped', and there are no baselines.
Comment 7 Alexey Proskuryakov 2012-03-13 17:34:33 PDT
> In English, my expectations are:
> 
> by default, on lion (platform mac-lion), we will look for baselines in mac-lion and then mac, but write new baselines into mac

This looks right to me.

> 'mac-future' will only read from 'mac' and will write to 'mac'.
> 
> Nothing should ever be written to 'platform/mac-future'. There is no 'platform/mac-future/Skipped', and there are no baselines.

I don't know what mac-future platform is needed for, but this behavior sounds safe.
Comment 8 Mark Rowe (bdash) 2012-03-13 17:43:59 PDT
Comment on attachment 131757 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:56
> +        if self.name() == 'mac-lion':
> +            # mac-lion should normally write baselines into platform/mac until Mountain Lion ships.
> +            # It can still read baselines from platform/mac-lion, though.

Can we avoid hard-coding Lion / Mountain Lion in here? This will always be the case for the currently-shipping version of the OS.
Comment 9 Dirk Pranke 2012-03-13 18:15:58 PDT
(In reply to comment #8)
> (From update of attachment 131757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131757&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:56
> > +        if self.name() == 'mac-lion':
> > +            # mac-lion should normally write baselines into platform/mac until Mountain Lion ships.
> > +            # It can still read baselines from platform/mac-lion, though.
> 
> Can we avoid hard-coding Lion / Mountain Lion in here? This will always be the case for the currently-shipping version of the OS.

I can certainly delete the "Mountain Lion" reference, and rework this so that we have a mac.CURRENT_VERSION='lion' or something - is that what you were suggesting?

Or were you suggesting that somehow I attempt to figure out what "the currently shipping version of the OS" is dynamically? I don't know how I'd do that.
Comment 10 Dirk Pranke 2012-03-13 18:18:10 PDT
(In reply to comment #7)
> > In English, my expectations are:
> > 
> > by default, on lion (platform mac-lion), we will look for baselines in mac-lion and then mac, but write new baselines into mac
> 
> This looks right to me.
> 
> > 'mac-future' will only read from 'mac' and will write to 'mac'.
> > 
> > Nothing should ever be written to 'platform/mac-future'. There is no 'platform/mac-future/Skipped', and there are no baselines.
> 
> I don't know what mac-future platform is needed for, but this behavior sounds safe.

'mac-future' as a name/concept is needed for you guys to be able to run on unreleased versions of the operating system and have that get handled properly (distinguishing it from the currently shipping version). There isn't supposed to be a directory -- there is one currently checked in, but that's the result of a bug.

Partially I am looking to confirm that the unreleased version of the OS doesn't need a Skipped file (at least not one that's checked into webkit).
Comment 11 Mark Rowe (bdash) 2012-03-13 18:19:12 PDT
(In reply to comment #10)
> Partially I am looking to confirm that the unreleased version of the OS doesn't need a Skipped file (at least not one that's checked into webkit).

That's correct.
Comment 12 Mark Rowe (bdash) 2012-03-13 18:20:44 PDT
(In reply to comment #9)
> 
> Or were you suggesting that somehow I attempt to figure out what "the currently shipping version of the OS" is dynamically? I don't know how I'd do that.

MacPort has a list of OS X versions in order of release. It seems relatively easy to derive the desired information from there.
Comment 13 Dirk Pranke 2012-03-13 18:22:50 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > 
> > Or were you suggesting that somehow I attempt to figure out what "the currently shipping version of the OS" is dynamically? I don't know how I'd do that.
> 
> MacPort has a list of OS X versions in order of release. It seems relatively easy to derive the desired information from there.

Sure, that works as well.
Comment 14 Dirk Pranke 2012-03-13 18:59:52 PDT
Created attachment 131771 [details]
don't hardcode mac-lion
Comment 15 Dirk Pranke 2012-03-14 11:53:45 PDT
Committed r110722: <http://trac.webkit.org/changeset/110722>