Summary: | chromium nrwt: Pick the newest binary found in DEFAULT_BUILD_DIRECTORIES, not the first | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nico Weber <thakis> | ||||||||||
Component: | New Bugs | Assignee: | 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
Nico Weber
2012-12-19 22:30:51 PST
Created attachment 180280 [details]
Patch
webkitdirs.pm gets this right already, but nrwt didn't. Comment on attachment 180280 [details]
Patch
Can we test this?
Created attachment 180283 [details]
Patch
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 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?
(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). Created attachment 180346 [details]
Patch
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 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 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. (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. (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. (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 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. (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. Created attachment 180395 [details]
Patch for landing
Comment on attachment 180395 [details] Patch for landing Clearing flags on attachment: 180395 Committed r138294: <http://trac.webkit.org/changeset/138294> All reviewed patches have been landed. Closing bug. |