Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2120233002:
Enable gold threaded link (Closed)
DescriptionEnable gold multi-threaded link
BUG=161942
Committed: https://crrev.com/ce25823016da06a8f947efba77eaba23c7cec7a1
Cr-Commit-Position: refs/heads/master@{#404208}
Patch Set 1 #
Total comments: 2
Patch Set 2 : +linux_use_bundled_binutils condition #
Total comments: 3
Messages
Total messages: 19 (7 generated)
Description was changed from ========== Enable gold threaded link BUG=161942 ========== to ========== Enable gold multi-threaded link BUG=161942 ==========
tzik@chromium.org changed reviewers: + thakis@chromium.org
PTAL
Thanks! https://codereview.chromium.org/2120233002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2120233002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:329: "-Wl,--thread-count=4", I think we only want to enable this if we're using bundled gold (ie usually not for builds with the cros tool chain). Is this checked for all already?
https://codereview.chromium.org/2120233002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2120233002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:329: "-Wl,--thread-count=4", On 2016/07/04 21:24:58, Nico wrote: > I think we only want to enable this if we're using bundled gold (ie usually not > for builds with the cros tool chain). Is this checked for all already? I think it wasn't. Added a condition to check linux_use_bundled_binutils here.
lgtm, let's give this another try.
The CQ bit was checked by tzik@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 ========== Enable gold multi-threaded link BUG=161942 ========== to ========== Enable gold multi-threaded link BUG=161942 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Enable gold multi-threaded link BUG=161942 ========== to ========== Enable gold multi-threaded link BUG=161942 Committed: https://crrev.com/ce25823016da06a8f947efba77eaba23c7cec7a1 Cr-Commit-Position: refs/heads/master@{#404208} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ce25823016da06a8f947efba77eaba23c7cec7a1 Cr-Commit-Position: refs/heads/master@{#404208}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:40: linux_use_bundled_binutils = I'm happy this CL landed, but now I'm trying to roll DEPS for PDFium, and I'm stuck because this evaluates to true, even though $binutils_path below does not exist. Any suggestions how if I should add some kind of a check for $binutils_path, or just add binutils to the PDFium build on Linux?
Message was sent while issue was closed.
https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:40: linux_use_bundled_binutils = On 2016/08/29 17:55:43, Lei Zhang wrote: > I'm happy this CL landed, but now I'm trying to roll DEPS for PDFium, and I'm > stuck because this evaluates to true, even though $binutils_path below does not > exist. Any suggestions how if I should add some kind of a check for > $binutils_path, or just add binutils to the PDFium build on Linux? I'd ask on gn-dev what best practice is supposed to be.
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:40: linux_use_bundled_binutils = On 2016/08/29 17:56:50, Nico wrote: > On 2016/08/29 17:55:43, Lei Zhang wrote: > > I'm happy this CL landed, but now I'm trying to roll DEPS for PDFium, and I'm > > stuck because this evaluates to true, even though $binutils_path below does > not > > exist. Any suggestions how if I should add some kind of a check for > > $binutils_path, or just add binutils to the PDFium build on Linux? > > I'd ask on gn-dev what best practice is supposed to be. There's a number of different possible options, but in this particular case I'd be inclined to change this so that linux_use_bundled_binutils wasn't true if that directory didn't exist. There's a //build/dir_exists.py script that can be used for that purpose. Of course, we'd probably want to do a pass over things and figure out if there are other places where we assume that //third_party/binutils exists; hopefully they're all gated on this flag.
Message was sent while issue was closed.
On 2016/08/29 18:11:06, Dirk Pranke wrote: > https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2120233002/diff/20001/build/config/compiler/B... > build/config/compiler/BUILD.gn:40: linux_use_bundled_binutils = > On 2016/08/29 17:56:50, Nico wrote: > > On 2016/08/29 17:55:43, Lei Zhang wrote: > > > I'm happy this CL landed, but now I'm trying to roll DEPS for PDFium, and > I'm > > > stuck because this evaluates to true, even though $binutils_path below does > > not > > > exist. Any suggestions how if I should add some kind of a check for > > > $binutils_path, or just add binutils to the PDFium build on Linux? > > > > I'd ask on gn-dev what best practice is supposed to be. > > There's a number of different possible options, but in this particular case > I'd be inclined to change this so that linux_use_bundled_binutils wasn't > true if that directory didn't exist. > > There's a //build/dir_exists.py script that can be used for that purpose. > > Of course, we'd probably want to do a pass over things and figure out > if there are other places where we assume that //third_party/binutils exists; > hopefully they're all gated on this flag. Another alternative would be to add a flag to //build_overrides/build.gni indicating whether the repo should use binutils or not (that would avoid the exec_script() call, so it'd be slightly faster).
Message was sent while issue was closed.
Thinking about this a bit more, I'd actually prefer the latter, so that we don't need the exec_script call. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
