Bug 105498 – chromium nrwt: Pick the newest binary found in DEFAULT_BUILD_DIRECTORIES, not the first

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

chromium nrwt: Pick the newest binary found in DEFAULT_BUILD_DIRECTORIES, not the first

[Nico Weber]

Created attachment 180280 [details]
Patch

[Nico Weber]

webkitdirs.pm gets this right already, but nrwt didn't.

[Eric Seidel (no email)]

Comment on attachment 180280 [details]
Patch

Can we test this?

[Nico Weber]

Created attachment 180283 [details]
Patch

[Nico Weber]

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.

[Eric Seidel (no email)]

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?

[Nico Weber]

(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).

[Nico Weber]

Created attachment 180346 [details]
Patch

[Dirk Pranke]

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.

[Nico Weber]

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.

[Dirk Pranke]

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.

[Nico Weber]

(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.

[Dirk Pranke]

(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.

[Nico Weber]

(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?

[Dirk Pranke]

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.

[Nico Weber]

(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.

[Nico Weber]

Created attachment 180395 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180395 [details]
Patch for landing

Clearing flags on attachment: 180395

Committed r138294: <http://trac.webkit.org/changeset/138294>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.