Skip to content

Commit

Permalink
Remove unused arguments
Browse files Browse the repository at this point in the history
    This falls nicely into our quest to not use configuration from cc_toolchain :)

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240338037
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent b36956f commit b5ed979
Show file tree
Hide file tree
Showing 7 changed files with 131 additions and 86 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -418,35 +418,6 @@ public List<Pair<Artifact, Label>> getHeaders() {
return getHeaders(ruleContext);
}

public void reportInvalidOptions(RuleContext ruleContext) {
reportInvalidOptions(ruleContext, cppConfiguration, ccToolchain);
}

public static void reportInvalidOptions(
RuleContext ruleContext, CppConfiguration cppConfiguration, CcToolchainProvider ccToolchain) {
if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
&& !ccToolchain.supportsFission()) {
ruleContext.ruleWarning(
"Fission is not supported by this crosstool. Please use a "
+ "supporting crosstool to enable fission");
}
if (cppConfiguration.buildTestDwpIsActivated()
&& !(ccToolchain.supportsFission()
&& cppConfiguration.fissionIsActiveForCurrentCompilationMode())) {
ruleContext.ruleWarning(
"Test dwp file requested, but Fission is not enabled. To generate a "
+ "dwp for the test executable, use '--fission=yes' with a toolchain that supports "
+ "Fission to build statically.");
}

if (cppConfiguration.getLibcTopLabel() != null && ccToolchain.getDefaultSysroot() == null) {
ruleContext.ruleError(
"The selected toolchain "
+ ccToolchain.getToolchainIdentifier()
+ " does not support setting --grte_top (it doesn't specify builtin_sysroot).");
}
}

/**
* Supply CC_FLAGS Make variable value computed from FeatureConfiguration. Appends them to
* original CC_FLAGS, so FeatureConfiguration can override legacy values.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
}

final CcCommon common = new CcCommon(ruleContext);
common.reportInvalidOptions(ruleContext);
CompilationInfo compilationInfo =
new CcCompilationHelper(
ruleContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.analysis.skylark.SymbolGenerator;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
Expand Down Expand Up @@ -86,6 +85,7 @@ public CcLinkingOutputs getCcLinkingOutputs() {
}
}

private final RuleContext ruleContext;
private final CppSemantics semantics;
private final BuildConfiguration configuration;
private final CppConfiguration cppConfiguration;
Expand Down Expand Up @@ -125,52 +125,40 @@ public CcLinkingOutputs getCcLinkingOutputs() {
private final Label label;
private final ActionRegistry actionRegistry;
private final RuleErrorConsumer ruleErrorConsumer;
private final SymbolGenerator<?> symbolGenerator;

private Artifact grepIncludes;
private boolean isStampingEnabled;
private boolean isTestOrTestOnlyTarget;

/**
* Creates a CcLinkingHelper that outputs artifacts in a given configuration.
*
* @param ruleErrorConsumer the RuleErrorConsumer
* @param label the Label of the rule being built
* @param actionRegistry the ActionRegistry of the rule being built
* @param actionConstructionContext the ActionConstructionContext of the rule being built
* @param ruleContext the RuleContext for the rule being built
* @param semantics CppSemantics for the build
* @param featureConfiguration activated features and action configs for the build
* @param ccToolchain the C++ toolchain provider for the build
* @param fdoContext the C++ FDO optimization support provider for the build
* @param configuration the configuration that gives the directory of output artifacts
*/
public CcLinkingHelper(
RuleErrorConsumer ruleErrorConsumer,
Label label,
ActionRegistry actionRegistry,
ActionConstructionContext actionConstructionContext,
RuleContext ruleContext,
CppSemantics semantics,
FeatureConfiguration featureConfiguration,
CcToolchainProvider ccToolchain,
FdoContext fdoContext,
BuildConfiguration configuration,
CppConfiguration cppConfiguration,
SymbolGenerator<?> symbolGenerator) {
BuildConfiguration configuration) {
this.ruleContext = Preconditions.checkNotNull(ruleContext);
this.semantics = Preconditions.checkNotNull(semantics);
this.featureConfiguration = Preconditions.checkNotNull(featureConfiguration);
this.ccToolchain = Preconditions.checkNotNull(ccToolchain);
this.fdoContext = Preconditions.checkNotNull(fdoContext);
this.configuration = Preconditions.checkNotNull(configuration);
this.cppConfiguration = cppConfiguration;
this.ruleErrorConsumer = ruleErrorConsumer;
this.label = label;
this.actionRegistry = actionRegistry;
this.actionConstructionContext = actionConstructionContext;
this.symbolGenerator = symbolGenerator;
this.cppConfiguration =
Preconditions.checkNotNull(ruleContext.getFragment(CppConfiguration.class));
this.ruleErrorConsumer = ruleContext;
this.label = ruleContext.getLabel();
this.actionRegistry = ruleContext;
this.actionConstructionContext = ruleContext;
}

/** Sets fields that overlap for cc_library and cc_binary rules. */
public CcLinkingHelper fromCommon(RuleContext ruleContext, CcCommon common) {
public CcLinkingHelper fromCommon(CcCommon common) {
addCcLinkingContexts(
CppHelper.getLinkingContextsFromDeps(
ImmutableList.copyOf(ruleContext.getPrerequisites("deps", Mode.TARGET))));
Expand Down Expand Up @@ -365,7 +353,7 @@ public CcLinkingContext buildCcLinkingContextFromLibrariesToLink(
new Linkstamp(
linkstamp,
ccCompilationContext.getDeclaredIncludeSrcs(),
actionConstructionContext.getActionKeyContext()));
ruleContext.getActionKeyContext()));
}
CcLinkingContext ccLinkingContext = CcLinkingContext.EMPTY;
if (!neverlink) {
Expand All @@ -378,7 +366,8 @@ public CcLinkingContext buildCcLinkingContextFromLibrariesToLink(
? NestedSetBuilder.emptySet(Order.LINK_ORDER)
: NestedSetBuilder.create(
Order.LINK_ORDER,
CcLinkingContext.LinkOptions.of(linkopts, symbolGenerator)))
CcLinkingContext.LinkOptions.of(
linkopts, ruleContext.getSymbolGenerator())))
.addLibraries(
NestedSetBuilder.<LibraryToLink>linkOrder().addAll(libraryToLinks).build())
.addNonCodeInputs(
Expand Down Expand Up @@ -469,26 +458,6 @@ public CcLinkingHelper setUseTestOnlyFlags(boolean useTestOnlyFlags) {
return this;
}

/** Used to set the runfiles path for test rules' dynamic libraries. */
public CcLinkingHelper setTestOrTestOnlyTarget(boolean isTestOrTestOnlyTarget) {
this.isTestOrTestOnlyTarget = isTestOrTestOnlyTarget;
return this;
}

/**
* Used to test the grep-includes tool. This is needing during linking because of linkstamping.
*/
public CcLinkingHelper setGrepIncludes(Artifact grepIncludes) {
this.grepIncludes = grepIncludes;
return this;
}

/** Whether linkstamping is enabled. */
public CcLinkingHelper setIsStampingEnabled(boolean isStampingEnabled) {
this.isStampingEnabled = isStampingEnabled;
return this;
}

public CcLinkingHelper setLinkingMode(LinkingMode linkingMode) {
this.linkingMode = linkingMode;
return this;
Expand Down Expand Up @@ -799,18 +768,13 @@ private boolean createDynamicLibrary(

private CppLinkActionBuilder newLinkActionBuilder(Artifact outputArtifact) {
return new CppLinkActionBuilder(
ruleErrorConsumer,
actionConstructionContext,
label,
ruleContext,
outputArtifact,
configuration,
ccToolchain,
fdoContext,
featureConfiguration,
semantics)
.setGrepIncludes(grepIncludes)
.setIsStampingEnabled(isStampingEnabled)
.setTestOrTestOnlyTarget(isTestOrTestOnlyTarget)
.setLinkerFiles(ccToolchain.getLinkerFiles())
.setLinkArtifactFactory(linkArtifactFactory)
.setUseTestOnlyFlags(useTestOnlyFlags);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ public CcToolchainVariables getCompileBuildVariables(
asStringNestedSet(includeDirs),
asStringNestedSet(quoteIncludeDirs),
asStringNestedSet(systemIncludeDirs),
asStringNestedSet(defines));
asStringNestedSet(defines),
addLegacyCxxOptions);
}

@Override
Expand Down Expand Up @@ -280,6 +281,8 @@ public CcToolchainVariables getLinkBuildVariables(
asStringNestedSet(runtimeLibrarySearchDirectories),
/* librariesToLink= */ null,
asStringNestedSet(librarySearchDirectories),
/* isLegacyFullyStaticLinkingMode= */ false,
isStaticLinkingMode,
/* addIfsoRelatedVariables= */ false);
}

Expand Down Expand Up @@ -989,10 +992,34 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark(
compiler,
abiVersion,
abiLibcVersion,
/* supportsStartEndLib= */ false,
/* supportsInterfaceSharedLibraries= */ false,
/* supportsEmbeddedRuntimes= */ false,
/* staticRuntimesFilegroup= */ "",
/* dynamicRuntimesFilegroup= */ "",
/* supportsFission */ false,
/* needsPic= */ false,
toolPathList,
/* compilerFlags= */ ImmutableList.of(),
/* cxxFlags= */ ImmutableList.of(),
/* unfilteredCxxFlags= */ ImmutableList.of(),
/* linkerFlags= */ ImmutableList.of(),
/* dynamicLibraryLinkerFlags= */ ImmutableList.of(),
/* testOnlyLinkerFlags= */ ImmutableList.of(),
/* objcopyEmbedFlags= */ ImmutableList.of(),
/* ldEmbedFlags= */ ImmutableList.of(),
/* compilationModeCompilerFlags= */ ImmutableMap.of(),
/* compilationModeCxxFlags= */ ImmutableMap.of(),
/* compilationModeLinkerFlags= */ ImmutableMap.of(),
/* mostlyStaticLinkingModeFlags= */ ImmutableList.of(),
/* dynamicLinkingModeFlags= */ ImmutableList.of(),
/* fullyStaticLinkingModeFlags= */ ImmutableList.of(),
/* mostlyStaticLibrariesLinkingModeFlags= */ ImmutableList.of(),
makeVariablePairs.build(),
convertFromNoneable(builtinSysroot, /* defaultValue= */ ""),
/* defaultLibcTop= */ "",
convertFromNoneable(ccTargetOs, /* defaultValue= */ ""),
/* hasDynamicLinkingModeFlags= */ false,
cToolchain.build().toString());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ static CcToolchainProvider getCcToolchainProvider(
fdoInputFile =
FdoInputFile.fromAbsolutePath(ccSkyframeSupportValue.getFdoZipPath().asFragment());
}

CppToolchainInfo toolchainInfo =
getCppToolchainInfo(
ruleContext,
Expand Down Expand Up @@ -610,6 +610,7 @@ static CcToolchainProvider getCcToolchainProvider(

Artifact prefetchHintsArtifact = getPrefetchHintsArtifact(prefetchHints, ruleContext);

reportInvalidOptions(ruleContext, toolchainInfo);
return new CcToolchainProvider(
getToolchainForSkylark(toolchainInfo),
cppConfiguration,
Expand Down Expand Up @@ -756,6 +757,31 @@ private static CToolchain parseToolchainFromAttributes(
}
}

private static void reportInvalidOptions(RuleContext ruleContext, CppToolchainInfo toolchain) {
CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
&& !toolchain.supportsFission()) {
ruleContext.ruleWarning(
"Fission is not supported by this crosstool. Please use a "
+ "supporting crosstool to enable fission");
}
if (cppConfiguration.buildTestDwpIsActivated()
&& !(toolchain.supportsFission()
&& cppConfiguration.fissionIsActiveForCurrentCompilationMode())) {
ruleContext.ruleWarning(
"Test dwp file requested, but Fission is not enabled. To generate a "
+ "dwp for the test executable, use '--fission=yes' with a toolchain that supports "
+ "Fission to build statically.");
}

if (cppConfiguration.getLibcTopLabel() != null && toolchain.getDefaultSysroot() == null) {
ruleContext.ruleError(
"The selected toolchain "
+ toolchain.getToolchainIdentifier()
+ " does not support setting --grte_top (it doesn't specify builtin_sysroot).");
}
}

@Nullable
private static CToolchain getToolchainFromAttributes(
RuleContext ruleContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,65 @@ public static List<String> expandLinkopts(
return result;
}

/**
* Returns the immutable list of linker options for fully statically linked outputs. Does not
* include command-line options passed via --linkopt or --linkopts.
*
* @param config the CppConfiguration for this build
* @param toolchain the c++ toolchain
* @param sharedLib true if the output is a shared lib, false if it's an executable
*/
public static ImmutableList<String> getFullyStaticLinkOptions(
CppConfiguration config, CcToolchainProvider toolchain, boolean sharedLib) {
if (sharedLib) {
return toolchain.getSharedLibraryLinkOptions(
toolchain.getLegacyMostlyStaticLinkFlags(config.getCompilationMode()));
} else {
return toolchain.getLegacyFullyStaticLinkFlags(config.getCompilationMode());
}
}

/**
* Returns the immutable list of linker options for mostly statically linked outputs. Does not
* include command-line options passed via --linkopt or --linkopts.
*
* @param config the CppConfiguration for this build
* @param toolchain the c++ toolchain
* @param sharedLib true if the output is a shared lib, false if it's an executable
*/
public static ImmutableList<String> getMostlyStaticLinkOptions(
CppConfiguration config,
CcToolchainProvider toolchain,
boolean sharedLib,
boolean shouldStaticallyLinkCppRuntimes) {
if (sharedLib) {
return toolchain.getSharedLibraryLinkOptions(
shouldStaticallyLinkCppRuntimes
? toolchain.getLegacyMostlyStaticSharedLinkFlags(config.getCompilationMode())
: toolchain.getLegacyDynamicLinkFlags(config.getCompilationMode()));
} else {
return toolchain.getLegacyMostlyStaticLinkFlags(config.getCompilationMode());
}
}

/**
* Returns the immutable list of linker options for artifacts that are not fully or mostly
* statically linked. Does not include command-line options passed via --linkopt or --linkopts.
*
* @param config the CppConfiguration for this build
* @param toolchain the c++ toolchain
* @param sharedLib true if the output is a shared lib, false if it's an executable
*/
public static ImmutableList<String> getDynamicLinkOptions(
CppConfiguration config, CcToolchainProvider toolchain, Boolean sharedLib) {
if (sharedLib) {
return toolchain.getSharedLibraryLinkOptions(
toolchain.getLegacyDynamicLinkFlags(config.getCompilationMode()));
} else {
return toolchain.getLegacyDynamicLinkFlags(config.getCompilationMode());
}
}

public static NestedSet<Pair<String, String>> getCoverageEnvironmentIfNeeded(
CppConfiguration cppConfiguration, CcToolchainProvider toolchain) {
if (cppConfiguration.collectCodeCoverage()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
+ symlink.prettyPrint()
+ "' to target '"
+ getPrimaryInput()
+ "': "
+ e.getMessage(),
+ "'",
e,
this,
false);
Expand Down

0 comments on commit b5ed979

Please sign in to comment.