Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2116913002:
Land v8-side changes to switch to v8_current_cpu in the GN build. (Closed)
|
Created:
4 years, 5 months ago by Dirk Pranke Modified:
4 years, 5 months ago CC:
v8-reviews_googlegroups.com, Nico Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionLand v8-side changes to switch to v8_current_cpu in the GN build.
This change makes the architecture that we target generated
v8 code for a property of the current toolchain, rather than a
global setting that applies to every toolchain.
This will allow us to properly build two snapshots for two different
architectures in a single build, which is needed for android
webview/monochrome builds.
R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org
BUG=625383
Committed: https://crrev.com/6c3aaae96964f2605562a211e52fc9e0928fd6c7
Cr-Commit-Position: refs/heads/master@{#37805}
Patch Set 1 #Patch Set 2 : for review #
Total comments: 5
Patch Set 3 : rework to use custom_toolchain, not buildconfig #
Total comments: 2
Patch Set 4 : remove print() calls #Patch Set 5 : merge #
Total comments: 1
Patch Set 6 : remove incorrect assert #
Total comments: 3
Messages
Total messages: 46 (19 generated)
Description was changed from ========== v8-side work in progress to handle v8_target_cpu properly. BUG=621581 ========== to ========== v8-side changes to switch to v8_current_cpu This change makes deciding which architecture to target code for a property of the current toolchain, rather than a global setting. This will allow us to properly build two snapshots in a single invocation, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=621581 ==========
dpranke@chromium.org changed reviewers: + brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org
Please take a look. This CL needs https://codereview.chromium.org/2116183002/ to land in Chromium first.
I have the feeling the first sentence of the CL description might miss a word. Or I'm reading it somehow wrong...
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
lgtm ~ comment: https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:55: if ((host_os == "linux" && current_os == "android") || is_clang) { Why did target_os change to current_os? Where's the difference? https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:60: } else if (host_os == "mac" && current_os == "win") { Do we really use this build configuration? Compiling for windows on mac?
Description was changed from ========== v8-side changes to switch to v8_current_cpu This change makes deciding which architecture to target code for a property of the current toolchain, rather than a global setting. This will allow us to properly build two snapshots in a single invocation, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=621581 ========== to ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes deciding which architecture to target code for a property of the current toolchain, rather than a global setting. This will allow us to properly build two snapshots in a single invocation, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=621581 ==========
I updated the description subject, hopefully it's clearer now. https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:55: if ((host_os == "linux" && current_os == "android") || is_clang) { On 2016/07/07 07:40:23, Michael Achenbach wrote: > Why did target_os change to current_os? Where's the difference? Theoretically, if this file got loaded from some target being parsed in the host_toolchain, it might still pick up the as if this was for android, and not linux. In practice, this is almost certainly not an issue one way or another. I just changed it to try and reduce uses of target_*, as it seems to be turning out that most such cases are wrong. https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:60: } else if (host_os == "mac" && current_os == "win") { On 2016/07/07 07:40:23, Michael Achenbach wrote: > Do we really use this build configuration? Compiling for windows on mac? thakis@ has been working off-and-on on this w/ clang.
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2116913002/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:60: } else if (host_os == "mac" && current_os == "win") { On 2016/07/07 17:19:16, Dirk Pranke wrote: > On 2016/07/07 07:40:23, Michael Achenbach wrote: > > Do we really use this build configuration? Compiling for windows on mac? > > thakis@ has been working off-and-on on this w/ clang. Yes, see https://codereview.chromium.org/1183633003/ It's in (slow) bring-up. The idea isn't "on mac" but "on non-windows" in general.
lgtm
Description was changed from ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes deciding which architecture to target code for a property of the current toolchain, rather than a global setting. This will allow us to properly build two snapshots in a single invocation, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=621581 ========== to ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes deciding which architecture to target code for a property of the current toolchain, rather than a global setting. This will allow us to properly build two snapshots in a single invocation, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ==========
> I updated the description subject, hopefully it's clearer now. Sorry for the misunderstanding, I meant the next sentence, e.g. the second line. The title was clear.
On 2016/07/08 07:03:12, Michael Achenbach wrote: > > I updated the description subject, hopefully it's clearer now. > > Sorry for the misunderstanding, I meant the next sentence, e.g. the second line. > The title was clear. Ah, okay. I'll see if I can reword that, too :).
Description was changed from ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes deciding which architecture to target code for a property of the current toolchain, rather than a global setting. This will allow us to properly build two snapshots in a single invocation, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ========== to ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes which architecture to target v8 code generator for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ==========
Description was changed from ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes which architecture to target v8 code generator for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ========== to ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes the architecture that we target generated v8 code for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ==========
Updated to match https://codereview.chromium.org/2116183002/#ps60001 . Please take another look?
lgtm LGTM https://codereview.chromium.org/2116913002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2116913002/diff/40001/BUILD.gn#newcode2082 BUILD.gn:2082: print(current_toolchain) Remove them?
https://codereview.chromium.org/2116913002/diff/40001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2116913002/diff/40001/BUILD.gn#newcode2082 BUILD.gn:2082: print(current_toolchain) On 2016/07/12 01:23:58, michaelbai wrote: > Remove them? Whoops, yes, will do.
lgtm ~ remove prints
lgtm
Can this land? Guess the chromium-side sticked. It would enable us to migrate our stand-alone simulators to gn as well.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, brettw@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2116913002/#ps60001 (title: "remove print() calls")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/15 13:28:26, Michael Achenbach wrote: > Can this land? Guess the chromium-side sticked. It would enable us to migrate > our stand-alone simulators to gn as well. Yes, this can (and should) land, so I'm landing it now. However, the changes won't really work right until I can delete https://cs.chromium.org/chromium/src/build/toolchain/gcc_toolchain.gni?rcl=0&... in a follow-up chromium-side CL that will also need to be merged into V8. This multi-step process was necessary to keep things working across the rolls :(.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9182) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9152)
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, brettw@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2116913002/#ps80001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2116913002/diff/80001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2116913002/diff/80001/snapshot_toolchain.gni#... snapshot_toolchain.gni:68: "v8 target must match the regular target on this platform") Turns out this assertion is wrong and doesn't hold on windows, where we might be using an x64 nacl toolchain even when targeting x86.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, brettw@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2116913002/#ps100001 (title: "remove incorrect assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes the architecture that we target generated v8 code for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ========== to ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes the architecture that we target generated v8 code for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes the architecture that we target generated v8 code for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 ========== to ========== Land v8-side changes to switch to v8_current_cpu in the GN build. This change makes the architecture that we target generated v8 code for a property of the current toolchain, rather than a global setting that applies to every toolchain. This will allow us to properly build two snapshots for two different architectures in a single build, which is needed for android webview/monochrome builds. R=brettw@chromium.org, jochen@chromium.org, michaelbai@chromium.org BUG=625383 Committed: https://crrev.com/6c3aaae96964f2605562a211e52fc9e0928fd6c7 Cr-Commit-Position: refs/heads/master@{#37805} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6c3aaae96964f2605562a211e52fc9e0928fd6c7 Cr-Commit-Position: refs/heads/master@{#37805}
Message was sent while issue was closed.
https://codereview.chromium.org/2116913002/diff/100001/build_overrides/v8.gni File build_overrides/v8.gni (left): https://codereview.chromium.org/2116913002/diff/100001/build_overrides/v8.gni... build_overrides/v8.gni:19: v8_enable_gdbjit_default = true v8_enable_gdbjit_default got removed here by rebase, will send fix CL.
Message was sent while issue was closed.
https://codereview.chromium.org/2116913002/diff/100001/build_overrides/v8.gni File build_overrides/v8.gni (left): https://codereview.chromium.org/2116913002/diff/100001/build_overrides/v8.gni... build_overrides/v8.gni:19: v8_enable_gdbjit_default = true On 2016/07/18 14:47:06, Michael Achenbach wrote: > v8_enable_gdbjit_default got removed here by rebase, will send fix CL. Argh, I thought I had fixed that, sorry :(.
Message was sent while issue was closed.
milko.leporis@imgtec.com changed reviewers: + milko.leporis@imgtec.com
Message was sent while issue was closed.
https://codereview.chromium.org/2116913002/diff/100001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2116913002/diff/100001/snapshot_toolchain.gni... snapshot_toolchain.gni:46: current_cpu == "mipsel64", current_cpu should be mips64el not mipsel64 Chromium Android mips64el GN gen is failing: http://www.rt-rk.com/mips-buildbot/builders/Release_build_mips64/builds/698/s...
Message was sent while issue was closed.
On 2016/07/20 12:00:00, lmilko wrote: > https://codereview.chromium.org/2116913002/diff/100001/snapshot_toolchain.gni > File snapshot_toolchain.gni (right): > > https://codereview.chromium.org/2116913002/diff/100001/snapshot_toolchain.gni... > snapshot_toolchain.gni:46: current_cpu == "mipsel64", > current_cpu should be mips64el not mipsel64 > > Chromium Android mips64el GN gen is failing: > http://www.rt-rk.com/mips-buildbot/builders/Release_build_mips64/builds/698/s... Thanks, fix posted to https://codereview.chromium.org/2167873002/# . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
