Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incompatible_remove_legacy_whole_archive: Remove --legacy_whole_archive while setting default to false #7362

Open
hlopko opened this issue Feb 6, 2019 · 9 comments

Comments

@hlopko
Copy link
Contributor

commented Feb 6, 2019

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:

  1. Add --features=legacy_whole_archive to the global .bazelrc.
  2. Users can start adding features = "-legacy_whole_archive" (note, minus here) to targets that are safe to build without the legacy behavior.
  3. Add a presubmit check telling users they should add features = [ "-legacy_whole_archive" ].
  4. Have a presubmit check comparing the binary size with and without the feature, to motivate users to migrate.
  5. Once everything is migrated, remove legacy_whole_archive from global .bazelrc.
  6. Celebrate
  7. Remove -legacy_whole_archive features from targets.
@hlopko

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

CC @trybka

bazel-io pushed a commit that referenced this issue Feb 13, 2019

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
#7362.

It also introduces a possibility to enable legacy behavior per target by `--features=legacy_whole_archive` option, or `features = [ "legacy_whole_archive" ]` attribute.

RELNOTES: None.
PiperOrigin-RevId: 233691404

bazel-io pushed a commit that referenced this issue Feb 18, 2019

Automated rollback of commit 64a966d.
*** Reason for rollback ***

Breaks a target in the canary's nightly, thus preventing us from releasing tomorrow:

[]

b/124656723

*** Original change description ***

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
#7362.

It also introduces a possibility to enable legacy behavior per target by `--features=legacy_whole_archive` option, or `features = [ "legacy_whole_archive" ]` attribute.

RELNOTES: None.
PiperOrigin-RevId: 234481950
@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@hlopko I added the breaking-change-0.26 label since no one seems to have spoken up about increasing the window. TBH, I'm not sure what the significance of the label is if you don't actually flip the flag, but letting you know jic :)

@hlopko

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@dkelmer Thank you! Because the flag is only available in 0.25, people actually didn't have a chance to try it out yet. I'm not in hurry to flip this flag, so I added migration-0.26 and breaking-change-0.27.

@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Sounds good :)

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

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
bazelbuild#7362.

It also introduces a possibility to enable legacy behavior per target by `--features=legacy_whole_archive` option, or `features = [ "legacy_whole_archive" ]` attribute.

RELNOTES: None.
PiperOrigin-RevId: 233691404

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Automated rollback of commit 64a966d.
*** Reason for rollback ***

Breaks a target in the canary's nightly, thus preventing us from releasing tomorrow:

[]

b/124656723

*** Original change description ***

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
bazelbuild#7362.

It also introduces a possibility to enable legacy behavior per target by `--features=legacy_whole_archive` option, or `features = [ "legacy_whole_archive" ]` attribute.

RELNOTES: None.
PiperOrigin-RevId: 234481950

pmuetschard added a commit to pmuetschard/gapid that referenced this issue May 10, 2019

@pmuetschard

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I think this breaks Android. On Android, to build an .so, one has to create a cc_library rule and depend on it from the android_binary target. This will cause bazel to create an .a for the cc_library and then attempt to create the .so from it. Without wrapping the .a in --whole-archive, well, no symbols make it into the .so.

Example commands I'm seeing:

clang ... -fPIC -c foo.cpp -o foo.o
ar rcsD ...libfoo.a ...foo.o
clang -shared -o ..._nativedeps/...so ...libfoo.a

Adding features = ["legacy_whole_archive"] to the cc_library or the android_binary rule had no effect.

@hlopko

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Did you try adding alwayslink = 1 to cc libraries symbols of which you need in the final .so?

@pmuetschard

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Adding alwayslink = 1 did indeed bring back the --whole-archive flag for the Android libs.

@laurentlb

This comment has been minimized.

Copy link
Member

commented May 31, 2019

This flag was not flipped in 0.27, targeting 1.0?

@hlopko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Yes, this didn't make it into 0.27, we're postponing the flip until 1.0. Could you create the labels so I can update our flags?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.