Automated rollback of commit 64a966dd8cd5dc564d179d2fe9ecb1c3c7b56b14.
*** 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
https://github.com/bazelbuild/bazel/issues/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
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 804e5cb..7dd60c9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -396,10 +396,6 @@
return cppOptions.legacyWholeArchive;
}
- public boolean removeLegacyWholeArchive() {
- return cppOptions.removeLegacyWholeArchive;
- }
-
public boolean getInmemoryDotdFiles() {
return cppOptions.inmemoryDotdFiles;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index fe51e2b..5745048 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -646,7 +646,7 @@
boolean needWholeArchive =
wholeArchive
- || needWholeArchive(featureConfiguration, linkType, linkopts, cppConfiguration);
+ || needWholeArchive(linkingMode, linkType, linkopts, isNativeDeps, cppConfiguration);
// Disallow LTO indexing for test targets that link statically, and optionally for any
// linkstatic target (which can be used to disable LTO indexing for non-testonly cc_binary
// built due to data dependences for a blaze test invocation). Otherwise this will provoke
@@ -1117,37 +1117,15 @@
/** The default heuristic on whether we need to use whole-archive for the link. */
private static boolean needWholeArchive(
- FeatureConfiguration featureConfiguration,
+ Link.LinkingMode staticness,
LinkTargetType type,
Collection<String> linkopts,
+ boolean isNativeDeps,
CppConfiguration cppConfig) {
+ boolean mostlyStatic = (staticness == Link.LinkingMode.STATIC);
boolean sharedLinkopts =
type.isDynamicLibrary() || linkopts.contains("-shared") || cppConfig.hasSharedLinkOption();
- // Fasten your seat belt, the logic below doesn't make perfect sense and it's full of obviously
- // missed corner cases. The world still stands and depends on this behavior, so ¯\_(ツ)_/¯.
- if (!sharedLinkopts) {
- // We are not producing shared library, there is no reason to use --whole-archive with
- // executable (if the executable doesn't use the symbols, nobody else will, so --whole-archive
- // is not needed).
- return false;
- }
- if (cppConfig.removeLegacyWholeArchive()) {
- // --incompatible_remove_legacy_whole_archive has been flipped, no --whole-archive for the
- // entire build.
- return false;
- }
- if (featureConfiguration.getRequestedFeatures().contains(CppRuleClasses.LEGACY_WHOLE_ARCHIVE)) {
- // --incompatible_remove_legacy_whole_archive has not been flipped, and this target requested
- // --whole-archive using features.
- return true;
- }
- if (cppConfig.legacyWholeArchive()) {
- // --incompatible_remove_legacy_whole_archive has not been flipped, so whether to
- // use --whole-archive depends on --legacy_whole_archive.
- return true;
- }
- // Hopefully future default.
- return false;
+ return (isNativeDeps || cppConfig.legacyWholeArchive()) && mostlyStatic && sharedLinkopts;
}
private static ImmutableSet<Artifact> constructOutputs(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index 78fe766..526e9b0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -360,18 +360,16 @@
public Label customMalloc;
@Option(
- name = "legacy_whole_archive",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
- effectTags = {OptionEffectTag.ACTION_COMMAND_LINES, OptionEffectTag.AFFECTS_OUTPUTS},
- metadataTags = {OptionMetadataTag.DEPRECATED},
- help =
- "Deprecated, superseded by --incompatible_remove_legacy_whole_archive "
- + "(see https://github.com/bazelbuild/bazel/issues/7362 for details). "
- + "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.")
+ name = "legacy_whole_archive",
+ defaultValue = "true",
+ documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
+ effectTags = {OptionEffectTag.ACTION_COMMAND_LINES, OptionEffectTag.AFFECTS_OUTPUTS},
+ help =
+ "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."
+ )
public boolean legacyWholeArchive;
@Option(
@@ -715,20 +713,6 @@
public boolean disableLegacyCrosstoolFields;
@Option(
- name = "incompatible_remove_legacy_whole_archive",
- defaultValue = "false",
- documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
- effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
- metadataTags = {
- OptionMetadataTag.INCOMPATIBLE_CHANGE,
- OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
- },
- help =
- "If true, Bazel will not link library dependencies as whole archive by default "
- + "(see https://github.com/bazelbuild/bazel/issues/7362 for migration instructions).")
- public boolean removeLegacyWholeArchive;
-
- @Option(
name = "incompatible_remove_cpu_and_compiler_attributes_from_cc_toolchain",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
@@ -884,7 +868,6 @@
host.disableLegacyCrosstoolFields = disableLegacyCrosstoolFields;
host.disableCrosstool = disableCrosstool;
host.enableCcToolchainResolution = enableCcToolchainResolution;
- host.removeLegacyWholeArchive = removeLegacyWholeArchive;
host.dontEnableHostNonhost = dontEnableHostNonhost;
return host;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
index 0399910..ef836e7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
@@ -412,9 +412,6 @@
*/
public static final String SUPPORTS_DYNAMIC_LINKER = "supports_dynamic_linker";
- /** A feature marking that the target needs to link its deps in --whole-archive block. */
- public static final String LEGACY_WHOLE_ARCHIVE = "legacy_whole_archive";
-
/** Ancestor for all rules that do include scanning. */
public static final class CcIncludeScanningRule implements RuleDefinition {
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java
index ce5d87d..c40af6c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java
@@ -230,15 +230,13 @@
sharedLibrary = nativeDeps;
}
FdoContext fdoContext = toolchain.getFdoContext();
- ImmutableSet.Builder<String> requestedFeatures =
- ImmutableSet.<String>builder().addAll(ruleContext.getFeatures()).add(STATIC_LINKING_MODE);
- if (!ruleContext.getDisabledFeatures().contains(CppRuleClasses.LEGACY_WHOLE_ARCHIVE)) {
- requestedFeatures.add(CppRuleClasses.LEGACY_WHOLE_ARCHIVE);
- }
FeatureConfiguration featureConfiguration =
CcCommon.configureFeaturesOrReportRuleError(
ruleContext,
- /* requestedFeatures= */ requestedFeatures.build(),
+ /* requestedFeatures= */ ImmutableSet.<String>builder()
+ .addAll(ruleContext.getFeatures())
+ .add(STATIC_LINKING_MODE)
+ .build(),
/* unsupportedFeatures= */ ruleContext.getDisabledFeatures(),
toolchain);
CppLinkActionBuilder builder =
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
index 24dc831..3392bfb 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -41,7 +41,6 @@
import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -213,53 +212,6 @@
}
@Test
- public void testLegacyWholeArchive() throws Exception {
- scratch.file(
- "x/BUILD",
- "cc_binary(",
- " name = 'libfoo.so',",
- " srcs = ['foo.cc'],",
- " linkshared = 1,",
- ")");
- // --incompatible_remove_legacy_whole_archive not flipped, --legacy_whole_archive wins.
- useConfiguration(
- "--cpu=k8", "--legacy_whole_archive", "--noincompatible_remove_legacy_whole_archive");
- assertThat(getLibfooArguments()).contains("-Wl,-whole-archive");
- useConfiguration(
- "--cpu=k8", "--nolegacy_whole_archive", "--noincompatible_remove_legacy_whole_archive");
- assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive");
-
- // --incompatible_remove_legacy_whole_archive flipped, --legacy_whole_archive ignored.
- useConfiguration(
- "--cpu=k8", "--legacy_whole_archive", "--incompatible_remove_legacy_whole_archive");
- assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive");
- useConfiguration(
- "--cpu=k8", "--nolegacy_whole_archive", "--incompatible_remove_legacy_whole_archive");
- assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive");
-
- // Even when --nolegacy_whole_archive, features can still add the behavior back.
- useConfiguration(
- "--cpu=k8",
- "--nolegacy_whole_archive",
- "--noincompatible_remove_legacy_whole_archive",
- "--features=legacy_whole_archive");
- assertThat(getLibfooArguments()).contains("-Wl,-whole-archive");
- // Even when --nolegacy_whole_archive, features can still add the behavior, but not when
- // --incompatible_remove_legacy_whole_archive is flipped.
- useConfiguration(
- "--cpu=k8",
- "--incompatible_remove_legacy_whole_archive",
- "--features=legacy_whole_archive");
- assertThat(getLibfooArguments()).doesNotContain("-Wl,-whole-archive");
- }
-
- private List<String> getLibfooArguments() throws LabelSyntaxException {
- ConfiguredTarget configuredTarget = getConfiguredTarget("//x:libfoo.so");
- CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(configuredTarget, "x/libfoo.so");
- return linkAction.getArguments();
- }
-
- @Test
public void testExposesRuntimeLibrarySearchDirectoriesVariable() throws Exception {
scratch.file(
"x/BUILD",
