Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2963183003:
Simplify condition to use libc++ (Closed)
|
Created:
3 years, 5 months ago by Tom Anderson Modified:
3 years, 5 months ago Reviewers:
Nico CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify condition to use libc++
This CL simplifies use_custom_libcxx. Most of the sanitizers are only
used on Linux so those cases can be removed.
BUG=593874
R=thakis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng
Review-Url: https://codereview.chromium.org/2963183003
Cr-Commit-Position: refs/heads/master@{#483787}
Committed: https://chromium.googlesource.com/chromium/src/+/3b5152538d133f59aa6709a0811ff6c745973cc6
Patch Set 1 #Patch Set 2 : Also remove default toolchain check #Messages
Total messages: 22 (15 generated)
Description was changed from ========== Simplify condition to use libc++ BUG=593874 R=thakis@chromium.org ========== to ========== Simplify condition to use libc++ BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng ==========
The CQ bit was checked by thomasanderson@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...
Description was changed from ========== Simplify condition to use libc++ BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng ========== to ========== Simplify condition to use libc++ This CL simplifies use_custom_libcxx. Most of the sanitizers are only used on Linux so those cases can be removed. The only configuration this might affect is libfuzzer on mac, however there's no trybot for that configuration, and the builder has been broken for at least several days [1]. [1] https://build.chromium.org/p/chromium.fyi/builders/Libfuzzer%20Upload%20Mac%2... BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng ==========
Description was changed from ========== Simplify condition to use libc++ This CL simplifies use_custom_libcxx. Most of the sanitizers are only used on Linux so those cases can be removed. The only configuration this might affect is libfuzzer on mac, however there's no trybot for that configuration, and the builder has been broken for at least several days [1]. [1] https://build.chromium.org/p/chromium.fyi/builders/Libfuzzer%20Upload%20Mac%2... BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng ========== to ========== Simplify condition to use libc++ This CL simplifies use_custom_libcxx. Most of the sanitizers are only used on Linux so those cases can be removed. BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng ==========
thakis ptal It took myself some convincing, but I think this is correct. * is_tsan and is_msan have asserts() to make sure they're only used on Linux. * libfuzzer is only used on mac and linux. This CL doesn't actually change when libc++ is used: libfuzzer will sill be used on Linux, and still *not* be used on mac * For ubsan and afl, we only have Linux builders, so things also shouldn't change
I think it would also be relatively easy to extend this to chromecast and even chromeos too
Lgtm Chromeos is special: they have this "build in fairly regular Linux build" mode that their UI folks use for development, but in "real" builds they use a completely custom toolchain. Look at the "simplechrome" bots on build.chromium.org (generate_build_files stdout). Also, CrOS has a whole dedicated toolchain team, so we usually let them do CrOS stuff. They're working on switching to libc++, but they'll likely do it outside of our build files.
On 2017/06/29 23:18:03, Nico (vacation Jun 30-Jul 11) wrote: > Lgtm > > Chromeos is special: they have this "build in fairly regular Linux build" mode > that their UI folks use for development, but in "real" builds they use a > completely custom toolchain. Look at the "simplechrome" bots on > http://build.chromium.org (generate_build_files stdout). Also, CrOS has a whole > dedicated toolchain team, so we usually let them do CrOS stuff. They're working > on switching to libc++, but they'll likely do it outside of our build files. Interesting! That's the longest args.gn I've ever seen! I think we can still enable libc++ for "desktop CrOs" builds. FWIW, I did a local build with the same args.gn as linux_chromium_chromeos_rel_ng and the target 'all' built just fine.
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 thomasanderson@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.
The CQ bit was checked by thomasanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2963183003/#ps20001 (title: "Also remove default toolchain check")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498850215063610,
"parent_rev": "bcdb7dda930d365288aadfe4449a0c972b77d5a3", "commit_rev":
"3b5152538d133f59aa6709a0811ff6c745973cc6"}
Message was sent while issue was closed.
Description was changed from ========== Simplify condition to use libc++ This CL simplifies use_custom_libcxx. Most of the sanitizers are only used on Linux so those cases can be removed. BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng ========== to ========== Simplify condition to use libc++ This CL simplifies use_custom_libcxx. Most of the sanitizers are only used on Linux so those cases can be removed. BUG=593874 R=thakis@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng;master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng Review-Url: https://codereview.chromium.org/2963183003 Cr-Commit-Position: refs/heads/master@{#483787} Committed: https://chromium.googlesource.com/chromium/src/+/3b5152538d133f59aa6709a0811f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3b5152538d133f59aa6709a0811f...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2966893002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 483787 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb.... |
