Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171083003:
Re-land "Fix double-building of v8 in GN builds" (Closed)
|
Created:
4 years, 5 months ago by Dirk Pranke Modified:
4 years, 5 months ago Reviewers:
Michael Achenbach 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. |
DescriptionRe-land "Fix double-building of v8 in GN builds"
This re-lands r37926 w/ the needed fix for cross-compiles; we
can only re-use the default toolchain when the host can actually
run it.
R=machenbach@chromium.org
BUG=629825
Committed: https://crrev.com/5b762044b53f988fa9a534fe1a84f9938b3abd75
Cr-Commit-Position: refs/heads/master@{#37970}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 1
Messages
Total messages: 18 (8 generated)
Description was changed from ========== Re-land "Fix double-building of v8 in GN builds" This re-lands r37926 w/ the needed fix for cross-compiles. R=machenbach@chromium.org BUG=629825 Review-Url: https://codereview.chromium.org/2166173002 Cr-Commit-Position: refs/heads/master@{#37926} ========== to ========== Re-land "Fix double-building of v8 in GN builds" This re-lands r37926 w/ the needed fix for cross-compiles; we can only re-use the default toolchain when the host can actually run it. R=machenbach@chromium.org BUG=629825 ==========
Please take a look. I don't think this is probably the best way to express this logic, but my brain isn't coming up w/ anything better at the moment and it at least seems to work. Feel free to land this and see, or submit something similar that has the same effect but is expressed more cleanly.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm - seems to be reasonable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by machenbach@chromium.org
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 ========== Re-land "Fix double-building of v8 in GN builds" This re-lands r37926 w/ the needed fix for cross-compiles; we can only re-use the default toolchain when the host can actually run it. R=machenbach@chromium.org BUG=629825 ========== to ========== Re-land "Fix double-building of v8 in GN builds" This re-lands r37926 w/ the needed fix for cross-compiles; we can only re-use the default toolchain when the host can actually run it. R=machenbach@chromium.org BUG=629825 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Re-land "Fix double-building of v8 in GN builds" This re-lands r37926 w/ the needed fix for cross-compiles; we can only re-use the default toolchain when the host can actually run it. R=machenbach@chromium.org BUG=629825 ========== to ========== Re-land "Fix double-building of v8 in GN builds" This re-lands r37926 w/ the needed fix for cross-compiles; we can only re-use the default toolchain when the host can actually run it. R=machenbach@chromium.org BUG=629825 Committed: https://crrev.com/5b762044b53f988fa9a534fe1a84f9938b3abd75 Cr-Commit-Position: refs/heads/master@{#37970} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5b762044b53f988fa9a534fe1a84f9938b3abd75 Cr-Commit-Position: refs/heads/master@{#37970}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2175773002/ by timvolodine@chromium.org. The reason for reverting is: breaks compile on Android x86 and x64 Builders (dbg) https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20x8... also see crbug.com/630603.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2175693003/ by vogelheim@chromium.org. The reason for reverting is: Reverted, because it breaks some funky android build. Can reproduce breakage locally, w/ args.gn as follows: disable_brotli_filter = true disable_file_support = true disable_ftp_support = true enable_websockets = false ffmpeg_branding = "Chrome" is_component_build = false is_debug = true proprietary_codecs = true symbol_level = 1 target_cpu = "x86" target_os = "android" use_goma = true use_platform_icu_alternatives = true This ends up building the mkpeephole tool w/ an architecture that won't run on the build machine. .
Message was sent while issue was closed.
https://codereview.chromium.org/2171083003/diff/20001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2171083003/diff/20001/snapshot_toolchain.gni#... snapshot_toolchain.gni:42: (target_cpu == "x86" || target_cpu == "x64")) { So the revert was due to the case where target cpu is x86, but Android, and therefore still can't run on the host. Maybe limit this to linux targets?
Message was sent while issue was closed.
On 2016/07/22 20:55:15, Michael Achenbach (slow) wrote: > https://codereview.chromium.org/2171083003/diff/20001/snapshot_toolchain.gni > File snapshot_toolchain.gni (right): > > https://codereview.chromium.org/2171083003/diff/20001/snapshot_toolchain.gni#... > snapshot_toolchain.gni:42: (target_cpu == "x86" || target_cpu == "x64")) { > So the revert was due to the case where target cpu is x86, but Android, and > therefore still can't run on the host. Maybe limit this to linux targets? Yup, I think it needs to be "target_os == host_os". I'll upload a patch soon, and see if I can make this a bit clearer as well now that it's not the end of the day.
Message was sent while issue was closed.
Guess the android_compile_x86_dbg trybot would have caught it. Let's add it on a reland. |
