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 804e5cb495d2d5..7dd60c96768c9a 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 @@ public boolean legacyWholeArchive() { 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 fe51e2b54683e5..574504828b4d98 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 @@ public CppLinkAction build() throws InterruptedException { 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 @@ private boolean shouldUseLinkDynamicLibraryTool() { /** 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 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 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 78fe766eb929ea..526e9b0ec7d078 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 String getTypeDescription() { 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( @@ -714,20 +712,6 @@ public Label getFdoPrefetchHintsLabel() { + "(see https://github.com/bazelbuild/bazel/issues/6861 for migration instructions).") 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", @@ -884,7 +868,6 @@ public FragmentOptions getHost() { 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 03999104b63c46..ef836e76054b75 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 Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) { */ 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 ce5d87d879dd70..c40af6c7d4e940 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 @@ public static NativeDepsRunfiles createNativeDepsAction( sharedLibrary = nativeDeps; } FdoContext fdoContext = toolchain.getFdoContext(); - ImmutableSet.Builder requestedFeatures = - ImmutableSet.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.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 24dc83165bd5c2..3392bfba4b2222 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; @@ -212,53 +211,6 @@ public void testLibOptsAndLibSrcsAreInCorrectOrder() throws Exception { .matches(".* -Wl,-rpath[^ ]*some-dir(?= ).* -Wl,-rpath[^ ]*some-other-dir .*"); } - @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 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(