Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2173343002:
Attempt #4 to 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 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. |
DescriptionAttempt #4 to land "Fix double-building of v8 in GN builds."
The third attempt never landed :). This attempt completely reworks
the logic to attempt to be clearer and more obviously correct. This
attempt also actually had unit tests written for it (see bug 625353).
R=machenbach@chromium.org
BUG=625353, 629825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_arm64_dbg_recipe
Committed: https://crrev.com/682a41db33afada6052997f8d2ee5f20c3069787
Cr-Commit-Position: refs/heads/master@{#37999}
Patch Set 1 : patch for review w/ tests included for illustration #
Total comments: 2
Patch Set 2 : patch for landing, tests removed #Patch Set 3 : for review (w/ tests), s/mips/mipsel #Patch Set 4 : patchset #3 w/ tests removed, for landing #
Total comments: 2
Messages
Total messages: 30 (22 generated)
add tests, fix bugs
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. R=machenbach@chromium.org BUG=629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_compile_arm64_dbg_recipe ========== to ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_compile_arm64_dbg_recipe ==========
dpranke@chromium.org changed reviewers: + jochen@chromium.org, michaelbai@chromium.org
Patchset #1 (id:40001) has been deleted
dpranke@chromium.org changed reviewers: + brettw@chromium.org
Okay, please take another look, as the code is quite different this time. I decided that after getting this wrong three times, it was probably worth starting over to make sure things were clear, and actually writing tests. So, one full day of software engineering later ... The new code should be clearer about the reason for each branch. In addition, you can see that we only look at the current_* variables and the host_* variables, which means that this should work equally well for when v8 is being compiled in the primary toolchain (as normal) or in a secondary toolchain as part of a fat-binary-build like in webview (i.e., we never look at the target_* variables, which would confuse things). Make sure you take a look at the tests I wrote in patchset #1; they should be clear enough to follow. We don't have a good way to check in tests for build files at this point, so I've just posted the tests to the bug and will think about ways we might want to have tests in the future.
The CQ bit was checked by dpranke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, no build URL)
Description was changed from ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_compile_arm64_dbg_recipe ========== to ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_arm64_dbg_recipe ==========
The CQ bit was checked by dpranke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by dpranke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Separate comments on the tests. Thanks for working on those, we should really bake them in somehow. https://codereview.chromium.org/2173343002/diff/60001/v8_snapshot_test.py File v8_snapshot_test.py (right): https://codereview.chromium.org/2173343002/diff/60001/v8_snapshot_test.py#new... v8_snapshot_test.py:32: 'build/toolchain/win/setup_toolchain.py': ''' Is there no way to use the original files as they are? I find it a bit brittle that the test runner moves original files around. Could not at least BUILD.gn be a new file that lives in a test work dir? https://codereview.chromium.org/2173343002/diff/60001/v8_snapshot_test.py#new... v8_snapshot_test.py:87: self.assertEqual(set(got_lines), set(exp_lines)) Just a thought: The approach is maybe a bit sensitive to other output that might show up in the future.
LGTM! Thanks for this patch. It is very clear now and using the current_* values should also ease the orthogonal goal of compiling multiple V8s at once. I'll land this soon, so that the v8 roll containing this lands when MTV day starts. https://codereview.chromium.org/2173343002/diff/120001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2173343002/diff/120001/snapshot_toolchain.gni... snapshot_toolchain.gni:75: if (is_clang || is_android) { I assume there is no explicit host_clang flag like we had in gyp?
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 ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_arm64_dbg_recipe ========== to ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_arm64_dbg_recipe ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_arm64_dbg_recipe ========== to ========== Attempt #4 to land "Fix double-building of v8 in GN builds." The third attempt never landed :). This attempt completely reworks the logic to attempt to be clearer and more obviously correct. This attempt also actually had unit tests written for it (see bug 625353). R=machenbach@chromium.org BUG=625353, 629825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_x86_dbg,android_compile_mips_dbg,android_arm64_dbg_recipe Committed: https://crrev.com/682a41db33afada6052997f8d2ee5f20c3069787 Cr-Commit-Position: refs/heads/master@{#37999} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/682a41db33afada6052997f8d2ee5f20c3069787 Cr-Commit-Position: refs/heads/master@{#37999}
Message was sent while issue was closed.
https://codereview.chromium.org/2173343002/diff/120001/snapshot_toolchain.gni File snapshot_toolchain.gni (right): https://codereview.chromium.org/2173343002/diff/120001/snapshot_toolchain.gni... snapshot_toolchain.gni:75: if (is_clang || is_android) { On 2016/07/25 09:04:02, Michael Achenbach (slow) wrote: > I assume there is no explicit host_clang flag like we had in gyp? Correct, there isn't a generic flag for this yet (there is a cros-specific one). I expect we'll add a generic one sooner or later, though. |
