Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011853002:
[gn] Add unittests (Closed)
|
Created:
4 years, 7 months ago by Michael Achenbach Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gn] Add unittests
BUG=chromium:474921
NOTRY=true
Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e
Cr-Commit-Position: refs/heads/master@{#36510}
Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c
Cr-Commit-Position: refs/heads/master@{#36582}
Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177
Cr-Commit-Position: refs/heads/master@{#36606}
Committed: https://crrev.com/d87fb10fe78a8b28e17486a8d4287a441b2d8440
Cr-Commit-Position: refs/heads/master@{#36642}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review #Patch Set 3 : Fixing v8_shell dependency #
Total comments: 3
Patch Set 4 : Ran gn format to sort everything. #Patch Set 5 : Fixes #Patch Set 6 : Clean up #
Total comments: 2
Patch Set 7 : Fix #Patch Set 8 : Fix and rebase #Patch Set 9 : Rebase gyp changes + comment #
Messages
Total messages: 53 (21 generated)
Description was changed from ========== [gn] Add unittests BUG= ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true ==========
machenbach@chromium.org changed reviewers: + bmeurer@chromium.org, vogelheim@chromium.org
PTAL https://codereview.chromium.org/2011853002/diff/1/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/1/BUILD.gn#newcode2024 BUILD.gn:2024: group("gn_all") { This ports parts of https://code.google.com/p/chromium/codesearch#chromium/src/v8/gypfiles/all.gyp Calling it like chromium does: https://code.google.com/p/chromium/codesearch#chromium/src/BUILD.gn&l=34 https://codereview.chromium.org/2011853002/diff/1/build_overrides/gtest.gni File build_overrides/gtest.gni (right): https://codereview.chromium.org/2011853002/diff/1/build_overrides/gtest.gni#n... build_overrides/gtest.gni:6: gtest_include_multiprocess = false Copied this guy: https://code.google.com/p/chromium/codesearch#chromium/src/build_overrides/gt... Flipped the gtest_include_multiprocess to false as it otherwise led to difference with the current gyp config. https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:4: This ports https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/unittests/... with two TODOs for follow ups. This https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/unittests/... is off by default in gn. So no action required.
lgtm https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:95: "heap/bitmap-unittest.cc", This looks like mostly-alphabetically sorted, in which case heap/* should go before interpreter/*. https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:119: if (v8_target_arch == "arm64") { [here & below] else if ? (Readability nitpick, really; and am not actually sure that's a generally accepted readability idea. But those /should/ be mutually exclusive, right?) https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:163: "//build/win:default_exe_manifest", "win:default_exe_manifest" _sounds_ like something that should be conditional on the OS...
Will also look into the test failure. https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:95: "heap/bitmap-unittest.cc", On 2016/05/25 09:30:58, vogelheim wrote: > This looks like mostly-alphabetically sorted, in which case heap/* should go > before interpreter/*. Done. Was c/p from gyp. https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:119: if (v8_target_arch == "arm64") { On 2016/05/25 09:30:58, vogelheim wrote: > [here & below] else if ? > > (Readability nitpick, really; and am not actually sure that's a generally > accepted readability idea. But those /should/ be mutually exclusive, right?) Done. Was c/p from the gyp structure. Not sure if more readable, but definitely more logical :) https://codereview.chromium.org/2011853002/diff/1/test/unittests/BUILD.gn#new... test/unittests/BUILD.gn:163: "//build/win:default_exe_manifest", On 2016/05/25 09:30:58, vogelheim wrote: > "win:default_exe_manifest" _sounds_ like something that should be conditional on > the OS... That's included unconditionally for all exes. The condition is somewhere in the config. Was added here: https://codereview.chromium.org/1941433002
PTAL at patch 3. Attempt to fix the cross-compile trybot by conditionally depending on v8_shell only.
lgtm https://codereview.chromium.org/2011853002/diff/40001/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/40001/test/unittests/BUILD.gn... test/unittests/BUILD.gn:85: "heap/heap-unittest.cc", nitpick: more non-standard alphabet use... (heap < gc < memory) https://codereview.chromium.org/2011853002/diff/40001/test/unittests/BUILD.gn... test/unittests/BUILD.gn:93: "interpreter/bytecode-register-allocator-unittest.cc", nitpick: also unsorted...
https://codereview.chromium.org/2011853002/diff/40001/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/40001/test/unittests/BUILD.gn... test/unittests/BUILD.gn:85: "heap/heap-unittest.cc", On 2016/05/25 10:37:38, vogelheim wrote: > nitpick: more non-standard alphabet use... (heap < gc < memory) Ran gn format now ;)
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2011853002/#ps60001 (title: "Ran gn format to sort everything.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011853002/60001
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2004373005/ by hablich@chromium.org. The reason for reverting is: Speculative revert because of roll block: https://codereview.chromium.org/2004203004/.
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} ==========
machenbach@chromium.org changed reviewers: + jochen@chromium.org
PTAL. Patches 5 and 6 fix the component build problem.
On 2016/05/30 08:57:34, Michael Achenbach (slow) wrote: > PTAL. Patches 5 and 6 fix the component build problem. Patch 5 also contains a rebase. Sorry for that...
lgtm (Please have a look at that comment. I'm not particularly sure about the issue, though.) https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.g... test/unittests/BUILD.gn:162: deps += [ "../..:v8_maybe_snapshot" ] Wouldn't this also require v8_base ?
https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.gn File test/unittests/BUILD.gn (right): https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.g... test/unittests/BUILD.gn:162: deps += [ "../..:v8_maybe_snapshot" ] On 2016/05/30 11:11:34, vogelheim wrote: > Wouldn't this also require v8_base ? It should be transitively in there. E.g. v8_external_snapshot depends on v8_base.
On 2016/05/30 11:31:37, Michael Achenbach (slow) wrote: > https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.gn > File test/unittests/BUILD.gn (right): > > https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.g... > test/unittests/BUILD.gn:162: deps += [ "../..:v8_maybe_snapshot" ] > On 2016/05/30 11:11:34, vogelheim wrote: > > Wouldn't this also require v8_base ? > > It should be transitively in there. E.g. v8_external_snapshot depends on > v8_base. + the bots that failed before, now compile
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011853002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011853002/100001
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
On 2016/05/30 11:31:37, Michael Achenbach (slow) wrote: > https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.gn > File test/unittests/BUILD.gn (right): > > https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.g... > test/unittests/BUILD.gn:162: deps += [ "../..:v8_maybe_snapshot" ] > On 2016/05/30 11:11:34, vogelheim wrote: > > Wouldn't this also require v8_base ? > > It should be transitively in there. E.g. v8_external_snapshot depends on > v8_base. Oh. But I guess then group/component "v8" don't need to depend on both v8_base and v8_maybe_snapshot?
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582}
Message was sent while issue was closed.
On 2016/05/30 11:34:55, vogelheim wrote: > On 2016/05/30 11:31:37, Michael Achenbach (slow) wrote: > > https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.gn > > File test/unittests/BUILD.gn (right): > > > > > https://codereview.chromium.org/2011853002/diff/100001/test/unittests/BUILD.g... > > test/unittests/BUILD.gn:162: deps += [ "../..:v8_maybe_snapshot" ] > > On 2016/05/30 11:11:34, vogelheim wrote: > > > Wouldn't this also require v8_base ? > > > > It should be transitively in there. E.g. v8_external_snapshot depends on > > v8_base. > > Oh. But I guess then group/component "v8" don't need to depend on both v8_base > and v8_maybe_snapshot? No, guess not. We could clean that up separately.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2022893002/ by machenbach@chromium.org. The reason for reverting is: http://crbug.com/615890.
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} ==========
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2011853002/#ps120001 (title: "Fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011853002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011853002/120001
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2026713002/ by machenbach@chromium.org. The reason for reverting is: Still http://crbug.com/615890.
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} ==========
Awesome. After https://codereview.chromium.org/2022093002/ landed we can check the remaining problem on the chromium android trybots. I just did that for patch 7 with fails. Will work on a fix.
PTAL at patch 8. Passes now on all relevant bots.
lgtm
On 2016/06/01 11:02:15, vogelheim wrote: > lgtm Rebased also latest gyp changes and added a "please keep in sync" comment.
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2011853002/#ps160001 (title: "Rebase gyp changes + comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011853002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011853002/160001
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} ========== to ========== [gn] Add unittests BUG=chromium:474921 NOTRY=true Committed: https://crrev.com/67e549ec5b34ad02506abb2720c6a0851ae31f5e Cr-Commit-Position: refs/heads/master@{#36510} Committed: https://crrev.com/143b3d167d7014525d049d3ab53c73cf2592243c Cr-Commit-Position: refs/heads/master@{#36582} Committed: https://crrev.com/378a26c03efc74bda401daa5accda223cb266177 Cr-Commit-Position: refs/heads/master@{#36606} Committed: https://crrev.com/d87fb10fe78a8b28e17486a8d4287a441b2d8440 Cr-Commit-Position: refs/heads/master@{#36642} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d87fb10fe78a8b28e17486a8d4287a441b2d8440 Cr-Commit-Position: refs/heads/master@{#36642} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
