Bug 105498

Summary: chromium nrwt: Pick the newest binary found in DEFAULT_BUILD_DIRECTORIES, not the first
Product: WebKit Reporter: Nico Weber <thakis>
Component: New BugsAssignee: Nico Weber <thakis>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 104434    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Nico Weber 2012-12-19 22:30:51 PST
chromium nrwt: Pick the newest binary found in DEFAULT_BUILD_DIRECTORIES, not the first
Comment 1 Nico Weber 2012-12-19 22:31:42 PST
Created attachment 180280 [details]
Patch
Comment 2 Nico Weber 2012-12-19 22:32:05 PST
webkitdirs.pm gets this right already, but nrwt didn't.
Comment 3 Eric Seidel (no email) 2012-12-19 22:37:40 PST
Comment on attachment 180280 [details]
Patch

Can we test this?
Comment 4 Nico Weber 2012-12-19 23:08:32 PST
Created attachment 180283 [details]
Patch
Comment 5 Nico Weber 2012-12-19 23:09:25 PST
rniwa showed me how to run python tests (Tools/Scripts/test-webkitpy webkitpy.layout_tests.port), so here's a patch that doesn't break the existing tests.

I'll add a test for my new feature tomorrow.
Comment 6 Eric Seidel (no email) 2012-12-19 23:16:43 PST
Comment on attachment 180283 [details]
Patch

I don't understand why this is "like webkitdir".  Does webkitdir support multiple build directories?  The /out vs. WebKitBuild thing is a chromium oddity, no?
Comment 7 Nico Weber 2012-12-20 08:26:31 PST
(In reply to comment #6)
> (From update of attachment 180283 [details])
> I don't understand why this is "like webkitdir".  Does webkitdir support multiple build directories?  The /out vs. WebKitBuild thing is a chromium oddity, no?

webkitdir.pm supports mutiple build directories in the chromium port (which uses webkitdir.pm for things like build-webkit and update-webkit).
Comment 8 Nico Weber 2012-12-20 08:42:46 PST
Created attachment 180346 [details]
Patch
Comment 9 Dirk Pranke 2012-12-20 09:44:51 PST
Comment on attachment 180346 [details]
Patch

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

> Tools/ChangeLog:9
> +        instead of an older xcodebuild binary.

I would probably rephrase this as something like "use the newest binary available rather than always picking one build directory over another".

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:95
> +            return hits[0][1]  # Return the newest file found.

Do you really want to always pick the file under chromium_base before the file under webkit_base, even if the file under webkit_base is newer? That seems kinda wrong.

It seems like you can probably delete lines 93-95 and just let lines 103-105 do the work?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:108
> +        self.assertEqual(port._build_path(), '/mock-checkout/xcodebuild/Release')

You should put this test in chromium_port_testcase.py, and then it'll be shared across all of the chromium port implementations by default; if you don't want one of the ports to implement this, then override the test in that port's _unittest.py file.
Comment 10 Nico Weber 2012-12-20 09:51:35 PST
Comment on attachment 180346 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:95
>> +            return hits[0][1]  # Return the newest file found.
> 
> Do you really want to always pick the file under chromium_base before the file under webkit_base, even if the file under webkit_base is newer? That seems kinda wrong.
> 
> It seems like you can probably delete lines 93-95 and just let lines 103-105 do the work?

A test explicitly tests for chromium_base always winning over webkit_base.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:108
>> +        self.assertEqual(port._build_path(), '/mock-checkout/xcodebuild/Release')
> 
> You should put this test in chromium_port_testcase.py, and then it'll be shared across all of the chromium port implementations by default; if you don't want one of the ports to implement this, then override the test in that port's _unittest.py file.

The DEFAULT_BUILD_DIRECTORIES are port-specific.
Comment 11 Dirk Pranke 2012-12-20 10:01:17 PST
Comment on attachment 180346 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:95
>>> +            return hits[0][1]  # Return the newest file found.
>> 
>> Do you really want to always pick the file under chromium_base before the file under webkit_base, even if the file under webkit_base is newer? That seems kinda wrong.
>> 
>> It seems like you can probably delete lines 93-95 and just let lines 103-105 do the work?
> 
> A test explicitly tests for chromium_base always winning over webkit_base.

I think we should probably update that test since we're changing the logic; I can't think of a reason to prefer older chromium binaries over newer webkit binaries.

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:108
>>> +        self.assertEqual(port._build_path(), '/mock-checkout/xcodebuild/Release')
>> 
>> You should put this test in chromium_port_testcase.py, and then it'll be shared across all of the chromium port implementations by default; if you don't want one of the ports to implement this, then override the test in that port's _unittest.py file.
> 
> The DEFAULT_BUILD_DIRECTORIES are port-specific.

Ah, good point. I didn't read the individual tests closely enough.
Comment 12 Nico Weber 2012-12-20 10:05:35 PST
(In reply to comment #11)
> (From update of attachment 180346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180346&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:95
> >>> +            return hits[0][1]  # Return the newest file found.
> >> 
> >> Do you really want to always pick the file under chromium_base before the file under webkit_base, even if the file under webkit_base is newer? That seems kinda wrong.
> >> 
> >> It seems like you can probably delete lines 93-95 and just let lines 103-105 do the work?
> > 
> > A test explicitly tests for chromium_base always winning over webkit_base.
> 
> I think we should probably update that test since we're changing the logic; I can't think of a reason to prefer older chromium binaries over newer webkit binaries.

I would think that the existing test is there for a reason.
Comment 13 Dirk Pranke 2012-12-20 10:15:02 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 180346 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=180346&action=review
> > 
> > >>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:95
> > >>> +            return hits[0][1]  # Return the newest file found.
> > >> 
> > >> Do you really want to always pick the file under chromium_base before the file under webkit_base, even if the file under webkit_base is newer? That seems kinda wrong.
> > >> 
> > >> It seems like you can probably delete lines 93-95 and just let lines 103-105 do the work?
> > > 
> > > A test explicitly tests for chromium_base always winning over webkit_base.
> > 
> > I think we should probably update that test since we're changing the logic; I can't think of a reason to prefer older chromium binaries over newer webkit binaries.
> 
> I would think that the existing test is there for a reason.

Yeah, it was. I wrote it :). I also wrote the code that intentionally always picked xcodebuild over out. I'm okay with changing it.
Comment 14 Nico Weber 2012-12-20 12:47:06 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 180346 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=180346&action=review
> > > 
> > > >>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:95
> > > >>> +            return hits[0][1]  # Return the newest file found.
> > > >> 
> > > >> Do you really want to always pick the file under chromium_base before the file under webkit_base, even if the file under webkit_base is newer? That seems kinda wrong.
> > > >> 
> > > >> It seems like you can probably delete lines 93-95 and just let lines 103-105 do the work?
> > > > 
> > > > A test explicitly tests for chromium_base always winning over webkit_base.
> > > 
> > > I think we should probably update that test since we're changing the logic; I can't think of a reason to prefer older chromium binaries over newer webkit binaries.
> > 
> > I would think that the existing test is there for a reason.
> 
> Yeah, it was. I wrote it :). I also wrote the code that intentionally always picked xcodebuild over out. I'm okay with changing it.

Ok. This seems like an unrelated change though, mind if I do that in a different patch?
Comment 15 Dirk Pranke 2012-12-20 13:00:18 PST
Comment on attachment 180346 [details]
Patch

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

> Ok. This seems like an unrelated change though, mind if I do that in a different patch?

Seems pretty related to me, but whatever is easiest for you (or you prefer) is fine.
Comment 16 Nico Weber 2012-12-20 13:06:02 PST
(In reply to comment #15)
> (From update of attachment 180346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180346&action=review
> 
> > Ok. This seems like an unrelated change though, mind if I do that in a different patch?
> 
> Seems pretty related to me, but whatever is easiest for you (or you prefer) is fine.

Cool, thanks. I'll send out the patch for that later today.
Comment 17 Nico Weber 2012-12-20 13:07:57 PST
Created attachment 180395 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-12-20 13:38:48 PST
Comment on attachment 180395 [details]
Patch for landing

Clearing flags on attachment: 180395

Committed r138294: <http://trac.webkit.org/changeset/138294>
Comment 19 WebKit Review Bot 2012-12-20 13:38:52 PST
All reviewed patches have been landed.  Closing bug.