Skip to content

Commit

Permalink
Automated rollback of commit 64a966dd8cd5dc564d179d2fe9ecb1c3c7b56b14.
Browse files Browse the repository at this point in the history
    *** 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/bazel#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
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 40386ca commit 18a7701
Show file tree
Hide file tree
Showing 6 changed files with 101 additions and 205 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,6 @@ public boolean legacyWholeArchive() {
return cppOptions.legacyWholeArchive;
}

public boolean removeLegacyWholeArchive() {
return cppOptions.removeLegacyWholeArchive;
}

public boolean getInmemoryDotdFiles() {
return cppOptions.inmemoryDotdFiles;
}
Expand Down Expand Up @@ -548,10 +544,6 @@ public boolean disableLegacyCrosstoolFields() {
return cppOptions.disableLegacyCrosstoolFields;
}

public boolean disableExpandIfAllAvailableInFlagSet() {
return cppOptions.disableExpandIfAllAvailableInFlagSet;
}

public static String getLegacyCrosstoolFieldErrorMessage(String field) {
Preconditions.checkNotNull(field);
return field
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public Artifact create(
private boolean isLtoIndexing = false;
private boolean usePicForLtoBackendActions = false;
private Iterable<LtoBackendArtifacts> allLtoArtifacts = null;

private final List<VariablesExtension> variablesExtensions = new ArrayList<>();
private final NestedSetBuilder<Artifact> linkActionInputs = NestedSetBuilder.stableOrder();
private final ImmutableList.Builder<Artifact> linkActionOutputs = ImmutableList.builder();
Expand Down Expand Up @@ -646,8 +646,7 @@ public CppLinkAction build() throws InterruptedException {

boolean needWholeArchive =
wholeArchive
|| needWholeArchive(
featureConfiguration, linkingMode, 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
Expand Down Expand Up @@ -1118,42 +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,
LinkingMode linkingMode,
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 (linkingMode != LinkingMode.STATIC) {
// legacy whole archive only applies to static linking mode.
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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",
Expand All @@ -743,20 +727,6 @@ public Label getFdoPrefetchHintsLabel() {
+ "(see https://github.com/bazelbuild/bazel/issues/7075 for migration instructions).")
public boolean removeCpuCompilerCcToolchainAttributes;

@Option(
name = "incompatible_disable_expand_if_all_available_in_flag_set",
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 allow specifying expand_if_all_available in flag_sets"
+ "(see https://github.com/bazelbuild/bazel/issues/7008 for migration instructions).")
public boolean disableExpandIfAllAvailableInFlagSet;

@Option(
name = "incompatible_disable_crosstool_file",
defaultValue = "false",
Expand Down Expand Up @@ -893,13 +863,11 @@ public FragmentOptions getHost() {
host.doNotUseCpuTransformer = doNotUseCpuTransformer;
host.disableGenruleCcToolchainDependency = disableGenruleCcToolchainDependency;
host.disableDepsetInUserFlags = disableDepsetInUserFlags;
host.disableExpandIfAllAvailableInFlagSet = disableExpandIfAllAvailableInFlagSet;
host.disableLegacyCcProvider = disableLegacyCcProvider;
host.removeCpuCompilerCcToolchainAttributes = removeCpuCompilerCcToolchainAttributes;
host.disableLegacyCrosstoolFields = disableLegacyCrosstoolFields;
host.disableCrosstool = disableCrosstool;
host.enableCcToolchainResolution = enableCcToolchainResolution;
host.removeLegacyWholeArchive = removeLegacyWholeArchive;
host.dontEnableHostNonhost = dontEnableHostNonhost;
return host;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,15 +230,13 @@ public static NativeDepsRunfiles createNativeDepsAction(
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 =
Expand Down
Loading

0 comments on commit 18a7701

Please sign in to comment.