bazel: incompatible_remove_legacy_whole_archive: Remove --legacy_whole_archive while setting default to false
Flag: --incompatible_remove_legacy_whole_archive
Available since: 0.25
Will be flipped: 0.26, unless we hear from Bazel users it’s not enough. Bazel 1.0 is the latest flipping possibility, but we will gladly postpone the flip until then, if Bazel users need more time. Please speak up.
Motivation
The docs of this flag say:
–[no]legacy_whole_archive default: “true” When on, use --whole-archive for cc_binary rules that have linkshared=1 and either linkstatic=1 or ‘-static’ in linkopts. This is for backwards compatibility only. A better alternative is to use alwayslink=1 where required.
What this means is that all library archives (and library object groups, as defined by --start-lib/–end-lib) are added to the link command wrapped in -whole-archive, -no-whole-archive, which instructs the linker to always include the entire library. Even with dead code elimination, this can be pernicious if the library defines some global initializer, as that will cause all of the code dependencies of those initializers to be kept.
History
--(legacy_)whole-archive
was introduced on 2004-04-19, with the commit description saying:
I think --whole-archive was just the magical incantation that got swigdeps.so working.
… You’re right though that the real question is what would break if we removed this, and I don’t really have a good idea.
Problem
“Legacy” whole-archive behavior is a poor default, but it is difficult to change.
--[no]legacy_whole_archive
is a whole build option. It is impossible to “tag” individual shared, statically linked, dynamic libraries which can be built without “whole-archive” semantics.
Care must be taken to flip the default–libraries which export symbols must be tagged as alwayslink=1
unless some other mechanism exists in the dylib to preserve the symbols which are being exported.
It is a difficult thing to test: unexported symbols are only noticed at runtime, which means that 100% test coverage would have to exist for a depot-wide mass cleanup.
Migration
--legacy_whole_archive
will be superseded by --incompatible_remove_legacy_whole_archive
. --incompatible_remove_legacy_whole_archive
, when true, removes both the old flag, and the behavior enabled by features (explained below).
Bazel now has a mechanism to configure individual cc_binary
targets which can be built without --legacy_whole_archive
. This mechanism still honors the --incompatible_remove_legacy_whole_archive
. This mechanism is based on feature named “legacy_whole_archive”. When added to the features
attribute of cc_binary
, Bazel will keep the legacy whole archive behavior, but only for the cc_binary
, not for the whole build.
This is the migration plan we will follow at Google:
- Add --features=legacy_whole_archive to the global .bazelrc.
- Users can start adding
features = "-legacy_whole_archive"
(note, minus here) to targets that are safe to build without the legacy behavior. - Add a presubmit check telling users they should add
features = [ "-legacy_whole_archive" ]
. - Have a presubmit check comparing the binary size with and without the feature, to motivate users to migrate.
- Once everything is migrated, remove
legacy_whole_archive
from global .bazelrc. - Celebrate
- Remove
-legacy_whole_archive
features from targets.
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Comments: 20 (17 by maintainers)
Commits related to this issue
- Introduce --incompatible_remove_legacy_whole_archive This flag disables legacy whole archive behavior, as well as makes the --legacy_whole_archive option a noop. See details in https://github.com/baz... — committed to bazelbuild/bazel by hlopko 5 years ago
- Automated rollback of commit 64a966dd8cd5dc564d179d2fe9ecb1c3c7b56b14. *** Reason for rollback *** Breaks a target in the canary's nightly, thus preventing us from releasing tomorrow: [] b/1246567... — committed to bazelbuild/bazel by fweikert 5 years ago
- Don't use the soon-to-be deprecated --legacy_whole_archive flag. See https://github.com/bazelbuild/bazel/issues/7362 — committed to pmuetschard/gapid by pmuetschard 5 years ago
- Don't use the soon-to-be deprecated --legacy_whole_archive flag. See https://github.com/bazelbuild/bazel/issues/7362 — committed to pmuetschard/gapid by pmuetschard 5 years ago
- This PR updates Tensorflow to change protobuf's cc_library //third_party/protobuf:protobuf to include alwayslink = 1. This is needed because tensorflow_framework.so currently defines the symbols prov... — committed to tensorflow/tensorflow by tensorflower-gardener 5 years ago
- PR #32040: Update tensorflow for upcoming incompatible change: https://github.com/bazelbuild/bazel/issues/7362 Imported from GitHub PR #32040 Update tensorflow for upcoming incompatible change: http... — committed to tensorflow/tensorflow by tensorflower-gardener 5 years ago
- Add more alwayslink = 1 so we can build Tensorflow on Windows after --incompatible_remove_legacy_whole_archive (https://github.com/bazelbuild/bazel/issues/7362) is flipped Currently, library archives... — committed to tensorflow/tensorflow by tensorflower-gardener 5 years ago
- Flip --incompatible_remove_legacy_whole_archive Fixes #7362 RELNOTES: --incompatible_remove_legacy_whole_archive has been flipped. See https://github.com/bazelbuild/bazel/issues/7362 for more inform... — committed to gnarled-cipher/bazel by scentini 5 years ago
- Add alwayslink flag to enclave cc_library targets New behavior in an upcoming version of Bazel (bazelbuild/bazel#7362) requires that cc_library targets specify alwayslink=1 if they want to export sym... — committed to google/asylo by KeithMoyer 5 years ago
- Release 1.0.0 (2019-10-10) Baseline: 97a82646dadd93bf52d47828bda42e3383b657c6 Cherry picks: + a0e3bb207fe2044120a2555a37162ee1f2b17500: Remove support for authentication and .netrc + ada... — committed to bazelbuild/bazel by a-googler 5 years ago
- Release 1.0.0 (2019-10-10) Baseline: 97a82646dadd93bf52d47828bda42e3383b657c6 Cherry picks: + a0e3bb207fe2044120a2555a37162ee1f2b17500: Remove support for authentication and .netrc + ada... — committed to bazelbuild/bazel by a-googler 5 years ago
- Update TF for Bazel 1.0's --incompatible_remove_legacy_whole_archive https://github.com/bazelbuild/bazel/issues/7362 This PR updates `cc_library`s to use `alwayslink=1` when needed. Currently, librar... — committed to tensorflow/tensorflow by tensorflower-gardener 5 years ago
- Update TF for Bazel 1.0's --incompatible_remove_legacy_whole_archive https://github.com/bazelbuild/bazel/issues/7362 This PR updates `cc_library`s to use `alwayslink=1` when needed. Currently, librar... — committed to ROCmSoftwarePlatform/tensorflow-upstream by tensorflower-gardener 5 years ago
- Update TF for Bazel 1.0's --incompatible_remove_legacy_whole_archive https://github.com/bazelbuild/bazel/issues/7362 Barring last minute regressions, this is the last commit of this type. This PR up... — committed to tensorflow/tensorflow by tensorflower-gardener 5 years ago
- PR #32040: Update tensorflow for upcoming incompatible change: https://github.com/bazelbuild/bazel/issues/7362 Imported from GitHub PR #32040 Update tensorflow for upcoming incompatible change: http... — committed to timokau/tensorflow by tensorflower-gardener 5 years ago
- Update TF for Bazel 1.0's --incompatible_remove_legacy_whole_archive https://github.com/bazelbuild/bazel/issues/7362 Barring last minute regressions, this is the last commit of this type. This PR up... — committed to timokau/tensorflow by tensorflower-gardener 5 years ago
- Update TF for Bazel 1.0's --incompatible_remove_legacy_whole_archive https://github.com/bazelbuild/bazel/issues/7362 This PR updates `cc_library`s to use `alwayslink=1` when needed. Currently, librar... — committed to timokau/tensorflow by tensorflower-gardener 5 years ago
- Add more `alwayslink=1` to cc_library as a follow up on flipping the default of --incompatible_remove_legacy_whole_archive in cl/277339372 This should fix the latest issue reported in https://github.... — committed to tensorflow/tensorflow by tensorflower-gardener 5 years ago
- This PR updates Tensorflow to change protobuf's cc_library //third_party/protobuf:protobuf to include alwayslink = 1. This is needed because tensorflow_framework.so currently defines the symbols prov... — committed to timokau/tensorflow by tensorflower-gardener 5 years ago
- Add more alwayslink = 1 so we can build Tensorflow on Windows after --incompatible_remove_legacy_whole_archive (https://github.com/bazelbuild/bazel/issues/7362) is flipped Currently, library archives... — committed to timokau/tensorflow by tensorflower-gardener 5 years ago
Update: in https://github.com/RobotLocomotion/drake/pull/12262, I’ve written a
cc_whole_archive_library(deps = ...)
which is basicallycc_library(alwayslink_deps = ...)
. It just rewriteslibraries_to_link
to addalwayslink = True
to every edge, instead of forcing it to appear upstream. Anyone is welcome to reuse it (Apache-2.0) for yourselves if it helps.@hlopko thanks for the advice. It’s a lot uglier than I’d like but it should work.
I realise my use case isn’t super common, but it doesn’t seem unreasonable either (“this
.so
target exists to be shipped standalone without dependencies, but the rest of the codebase wants to behave normally”). It seems odd to me thatalwayslink
has been put onto the depended-on targets rather than the final target? Why should mycc_library
care whether you decide to link in the whole archive or not (meanwhile, the downstream target might be quite opinionated about how linking happens)?I have the exact problem as @johnmarkwayve.
I have hundreds of small targets
cc_library(linkstatic = 1)
that are used both (1) by C++ applications with a main() routine (where we definitely do not wantalwayslink = True
for bloat reasons), and (2) by Python as a loaded module (as a single, large shared library composed of the smallcc_library
targets rolled into one).Because the
alwayslink
is at the point of declaration instead of point of use (aka not on the edges), I can’t tag my smallcc_library
targets with it. But there seems to be no built-in way to roll upcc_library
targets into a whole-archive shared library, which for my use case is a major regression and loss of capability.It seems like at minimum an attribute like the
alwayslink_deps
proposed above should exist, so that I can write …… at which point I could convert
all.a
into acc_binary(all.so)
shared library by writing one more simple rule.The work-around of using a macro to declare two
cc_library
targets (one with and withoutalwayslink
) is a non-starter. For one thing, it compiles all of the sources twice (even if I mark them as “manual” and only induce builds on the flavors that are actually used). For another, it gets us into a “red object” vs “blue object” world, where thedeps
lines need to be respelled inside the macro to have the correct color depending on whether the ultimatecc_binary
is a program or a shared library.For now my workaround is to implement a
cc_whole_archive_library
rule (see https://github.com/RobotLocomotion/drake/pull/12262) that repacks all objects into a single static archive, which I can then label withalwayslink = True
. This is a bit gross from a scale perspective (build footprint and I/O) but appears to get the job done.But in general I’d like if cc_library and/or cc_binary had the capability to deliver shared objects out of the box, instead of having to roll my own starlark for it.
I know I’m very late to the game here, but what’s the suggested mitigation? I had been using
cc_binary
withlinkshared
andlinkstatic
in a few places to force a build of a.so
which contains all symbols of all dependencies so it could be shipped on its own. I only care about linking in these symbols in the few cases where I want to ship a dependency-free.so
, so I definitely don’t want to pollute all of my othercc_library
rules withalwayslink = True
just to support this fairly rare use case?