Issue metadata
Sign in to add a comment
|
Add default dependencies by overriding shared_library. |
||||||||||||||||||||||||
Issue descriptionRecently, Chrome on Linux was switched to use libc++ hosted in//buildtools/third_party/libc++: https://codereview.chromium.org/2963223003 Since this change, the Chromecast component build (which is not exercised on the trybots) has been broken, because our shared libraries and exes do not explicitly depend on the GN target linked above. Other targets in Chrome are not broken in the component build, apparently because of a dependency on //build/config:exe_and_shlib_deps, which seems to be added everywhere. This pattern is not favorable because it places a burden on every developer to remember to add this dependency into such targets in the future. To fix this, we can override the shared_library target type using a template. This is a new feature in GN, and has not yet been picked up in the latest binary: https://chromium-review.googlesource.com/c/546924/4/tools/gn/docs/reference.md Once this is picked up, we can add the following template to BUILDCONFIG.gn to add the libc++ dependency to all shared libraries by default: template("shared_library") { shared_library(invoker.name) { forward_variables_from(invoker, [ "*" ]) deps += [ "//build/config:exe_and_shlib_deps" ] } } In the short-term, the Cast build will use a very similar template to workaround this issue.
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd378629b3d2bbc848071a816394662d0cdf8fd5 commit bd378629b3d2bbc848071a816394662d0cdf8fd5 Author: Stephen Lanham <slan@google.com> Date: Thu Jul 20 14:29:39 2017 [Chromecast] Introduce cast_{shared_library|executable}. This is a temporary workaround to allow the component build to work until crbug.com/746091 is resolved. This introduces a dependency on libc++ when use_custom_libcxx is true. BUG= 746091 Change-Id: I14ae65c0ac7d986649f5f70b47b0ad6f37ceff89 Reviewed-on: https://chromium-review.googlesource.com/578554 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Commit-Queue: Stephen Lanham <slan@chromium.org> Cr-Commit-Position: refs/heads/master@{#488237} [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/android/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/base/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/chromecast.gni [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/crash/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/graphics/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/media/cma/backend/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/media/cma/backend/alsa/BUILD.gn [modify] https://crrev.com/bd378629b3d2bbc848071a816394662d0cdf8fd5/chromecast/system/reboot/BUILD.gn
,
Jul 20 2017
One disadvantage of this design is that shared libraries with configs will need to update their configs declaration from this: configs += [ "//foo" ] to this: configs = [ "//foo" ] unless there is special code which accommodates this in the updated GN binary. This is only bad because it makes it inconsistent with every other target type, which happens to have default configs set.
,
Jul 20 2017
Expanding on #3, this also means that targets cannot manipulate the default configs from the target declaration. For example, the following statement would no longer be valid: configs -= [ "//foo:foo_config" ] This prevents a pretty serious problem, because targets won't be able to access these configs, which we currently rely on for suppressing formatting and such. This problem won't occur if we use set_defaults(), but that comes with costs of its own. Thoughts?
,
Jul 20 2017
I think the things you're hitting are partially why we hadn't done something before. Maybe the thing to do was just have a default deps from the beginning so that we already had deps += instead of deps =. Certainly the inconsistency has never been great.
,
Jul 20 2017
It looks like if we call set_defaults after defining the template override, that should resolve both of those issues. It will cause the defaults to be applied to the template scope, which will then be forwarded to the actual underlying target in the right order. It turns out I needed to do this for the Cast templates. I'll link this bug in the fix.
,
Jul 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ad698a813ca8f46842a7cf186bdb24789d81114 commit 4ad698a813ca8f46842a7cf186bdb24789d81114 Author: Stephen Lanham <slan@google.com> Date: Tue Jul 25 21:24:51 2017 [Chromecast] set_defaults() for cast binary target wrappers. Before this CL, the configs declared in cast_executable and cast_shared_library were clobbering the default configs set in BUILDCONFIG.gn. This is because the forward_variables_from() function silently overwrites the defaults. Call set_defaults for the templates themselves, so targets can modify the default configs as needed, and all the configs are added in the right order. Also add the cast_loadable_module wrapper. BUG= 746091 Change-Id: Ic8bc2ebfbb9a36c03d7273fff002ccdf668abfb4 Reviewed-on: https://chromium-review.googlesource.com/580249 Reviewed-by: Stephen Lanham <slan@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#489438} [modify] https://crrev.com/4ad698a813ca8f46842a7cf186bdb24789d81114/chromecast/chromecast.gni
,
Sep 2 2017
I don't actually remember where we ended up on this. Did you figure out some way to get things working cleanly? Do we still want to make the change listed in the description?
,
May 25 2018
Discovered this bug referenced in a comment while writing https://chromium-review.googlesource.com/c/chromium/src/+/1073613 Currently making progress on this over at bug 845700 . |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by dpranke@chromium.org
, Jul 19 2017Components: Build