Skip to content

Commit

Permalink
Pass AdditionalBuildVariableComputer to CcToolchainProvider
Browse files Browse the repository at this point in the history
    This way, we can compute build variables for a different configuration than the
    one present at cc_toolchain analysis. Do bad things in the process (such as storing BuildOptions in FeatureConfigurationForStarlark or creating AppleConfiguration inline in the AdditionalBuildVariableComputer).

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240997356
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 4adc582 commit 6027c20
Show file tree
Hide file tree
Showing 16 changed files with 52 additions and 143 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ private static CcToolchainVariables computeCcToolchainVariables(
String cpu = buildOptions.get(Options.class).cpu;

Map<String, String> appleEnv = getEnvironmentBuildVariables(xcodeConfig, cpu);
CcToolchainVariables.Builder variables = CcToolchainVariables.builder();
CcToolchainVariables.Builder variables = new CcToolchainVariables.Builder();
variables
.addStringVariable(
XCODE_VERSION_KEY, xcodeConfig.getXcodeVersion().toStringWithMinimumComponents(2))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -917,13 +917,7 @@ public static String computeCcFlags(RuleContext ruleContext, TransitiveInfoColle
String originalCcFlags = toolchainProvider.getLegacyCcFlagsMakeVariable();

// Ensure that Sysroot is set properly.
// TODO(b/129045294): We assume --incompatible_disable_genrule_cc_toolchain_dependency will
// be flipped sooner than --incompatible_enable_cc_toolchain_resolution. Then this method
// will be gone.
String sysrootCcFlags =
computeCcFlagForSysroot(
toolchainProvider.getCppConfigurationEvenThoughItCanBeDifferentThatWhatTargetHas(),
toolchainProvider);
String sysrootCcFlags = computeCcFlagForSysroot(toolchainProvider);

// Fetch additional flags from the FeatureConfiguration.
List<String> featureConfigCcFlags =
Expand All @@ -948,9 +942,8 @@ private static boolean containsSysroot(String ccFlags, List<String> moreCcFlags)
.anyMatch(str -> str.contains(SYSROOT_FLAG));
}

private static String computeCcFlagForSysroot(
CppConfiguration cppConfiguration, CcToolchainProvider toolchainProvider) {
PathFragment sysroot = toolchainProvider.getSysrootPathFragment(cppConfiguration);
private static String computeCcFlagForSysroot(CcToolchainProvider toolchainProvider) {
PathFragment sysroot = toolchainProvider.getSysrootPathFragment();
String sysrootFlag = "";
if (sysroot != null) {
sysrootFlag = SYSROOT_FLAG + sysroot;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ private PublicHeaders computePublicHeaders() {
*/
private CcCompilationContext initializeCcCompilationContext() {
CcCompilationContext.Builder ccCompilationContextBuilder =
CcCompilationContext.builder(actionConstructionContext, configuration, label);
new CcCompilationContext.Builder(actionConstructionContext, configuration, label);

// Setup the include path; local include directories come before those inherited from deps or
// from the toolchain; in case of aliasing (same include file found on different entries),
Expand Down Expand Up @@ -1214,7 +1214,7 @@ private NestedSet<Artifact> getSourceArtifactsByType(
* specified on the current object. This method should only be called once.
*/
private CcCompilationOutputs createCcCompileActions() throws RuleErrorException {
CcCompilationOutputs.Builder result = CcCompilationOutputs.builder();
CcCompilationOutputs.Builder result = new CcCompilationOutputs.Builder();
Preconditions.checkNotNull(ccCompilationContext);

if (shouldProvideHeaderModules()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ public CcCompilationContext createCcCompilationContext(
Object headers, Object systemIncludes, Object includes, Object quoteIncludes, Object defines)
throws EvalException {
CcCompilationContext.Builder ccCompilationContext =
CcCompilationContext.builder(
new CcCompilationContext.Builder(
/* actionConstructionContext= */ null, /* configuration= */ null, /* label= */ null);
ccCompilationContext.addDeclaredIncludeSrcs(
toNestedSetOfArtifacts(headers, "headers").getSet(Artifact.class));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.TemplateVariableInfo;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import java.io.Serializable;
import java.util.HashMap;
import net.starlark.java.syntax.Location;

/**
* Implementation for the cc_toolchain rule.
Expand All @@ -44,27 +42,9 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
/** Default attribute name for the c++ toolchain type */
public static final String CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME = "$cc_toolchain_type";

public static final String ALLOWED_LAYERING_CHECK_FEATURES_ALLOWLIST =
"disabling_parse_headers_and_layering_check_allowed";
public static final String ALLOWED_LAYERING_CHECK_FEATURES_TARGET =
"@bazel_tools//tools/build_defs/cc/whitelists/parse_headers_and_layering_check:"
+ ALLOWED_LAYERING_CHECK_FEATURES_ALLOWLIST;
public static final Label ALLOWED_LAYERING_CHECK_FEATURES_LABEL =
Label.parseAbsoluteUnchecked(ALLOWED_LAYERING_CHECK_FEATURES_TARGET);

public static final String LOOSE_HEADER_CHECK_ALLOWLIST =
"loose_header_check_allowed_in_toolchain";
public static final String LOOSE_HEADER_CHECK_TARGET =
"@bazel_tools//tools/build_defs/cc/whitelists/starlark_hdrs_check:" + LOOSE_HEADER_CHECK_ALLOWLIST;
public static final Label LOOSE_HEADER_CHECK_LABEL =
Label.parseAbsoluteUnchecked(LOOSE_HEADER_CHECK_TARGET);

@Override
public ConfiguredTarget create(RuleContext ruleContext)
throws InterruptedException, RuleErrorException, ActionConflictException {
if (!isAppleToolchain()) {
CcCommon.checkRuleLoadedThroughMacro(ruleContext);
}
validateToolchain(ruleContext);
CcToolchainAttributesProvider attributes =
new CcToolchainAttributesProvider(
Expand All @@ -82,16 +62,15 @@ public ConfiguredTarget create(RuleContext ruleContext)
if (!CppHelper.useToolchainResolution(ruleContext)) {
// This is not a platforms-backed build, let's provide CcToolchainAttributesProvider
// and have cc_toolchain_suite select one of its toolchains and create CcToolchainProvider
// from its attributes. We also need to provide a do-nothing ToolchainInfo.
return ruleConfiguredTargetBuilder
.addNativeDeclaredProvider(new ToolchainInfo(ImmutableMap.of("cc", "dummy cc toolchain")))
.build();
// from its attributes.
return ruleConfiguredTargetBuilder.build();
}

// This is a platforms-backed build, we will not analyze cc_toolchain_suite at all, and we are
// sure current cc_toolchain is the one selected. We can create CcToolchainProvider here.
CcToolchainProvider ccToolchainProvider =
CcToolchainProviderHelper.getCcToolchainProvider(ruleContext, attributes);
CcToolchainProviderHelper.getCcToolchainProvider(
ruleContext, attributes, /* crosstoolFromCcToolchainSuiteProtoAttribute= */ null);

if (ccToolchainProvider == null) {
// Skyframe restart
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ public class CcToolchainAttributesProvider extends ToolchainInfo implements HasC
private final Label libcTopAttribute;
private final NestedSet<Artifact> libc;
private final NestedSet<Artifact> libcMiddleman;
private final TransitiveInfoCollection libcTop;
private final NestedSet<Artifact> targetLibc;
private final NestedSet<Artifact> targetLibcMiddleman;
private final TransitiveInfoCollection targetLibcTop;
private final NestedSet<Artifact> fullInputsForCrosstool;
private final NestedSet<Artifact> fullInputsForLink;
private final NestedSet<Artifact> coverage;
Expand All @@ -82,6 +78,7 @@ public class CcToolchainAttributesProvider extends ToolchainInfo implements HasC
private final TransitiveInfoCollection fdoOptimize;
private final ImmutableList<Artifact> fdoOptimizeArtifacts;
private final FdoPrefetchHintsProvider fdoPrefetch;
private final TransitiveInfoCollection libcTop;
private final TransitiveInfoCollection moduleMap;
private final Artifact moduleMapArtifact;
private final Artifact zipper;
Expand Down Expand Up @@ -133,21 +130,9 @@ public CcToolchainAttributesProvider(
this.arFiles = getOptionalMiddlemanOrFiles(ruleContext, "ar_files");
this.linkerFiles = getMiddlemanOrFiles(ruleContext, "linker_files");
this.dwpFiles = getMiddlemanOrFiles(ruleContext, "dwp_files");

this.libcMiddleman =
getOptionalMiddlemanOrFiles(ruleContext, CcToolchainRule.LIBC_TOP_ATTR, Mode.TARGET);
this.libc = getOptionalFiles(ruleContext, CcToolchainRule.LIBC_TOP_ATTR, Mode.TARGET);
this.libcTop = ruleContext.getPrerequisite(CcToolchainRule.LIBC_TOP_ATTR, Mode.TARGET);

this.targetLibcMiddleman =
getOptionalMiddlemanOrFiles(ruleContext, CcToolchainRule.TARGET_LIBC_TOP_ATTR, Mode.TARGET);
this.targetLibc =
getOptionalFiles(ruleContext, CcToolchainRule.TARGET_LIBC_TOP_ATTR, Mode.TARGET);
this.targetLibcTop =
ruleContext.getPrerequisite(CcToolchainRule.TARGET_LIBC_TOP_ATTR, Mode.TARGET);

this.libcTopAttribute = ruleContext.attributes().get("libc_top", BuildType.LABEL);

this.fullInputsForCrosstool =
NestedSetBuilder.<Artifact>stableOrder()
.addTransitive(allFilesMiddleman)
Expand Down Expand Up @@ -179,6 +164,8 @@ public CcToolchainAttributesProvider(
this.fdoPrefetch =
ruleContext.getPrerequisite(
":fdo_prefetch_hints", Mode.TARGET, FdoPrefetchHintsProvider.PROVIDER);
this.libcTopAttribute = ruleContext.attributes().get("libc_top", BuildType.LABEL);
this.libcTop = ruleContext.getPrerequisite(CcToolchainRule.LIBC_TOP_ATTR, Mode.TARGET);
this.moduleMap = ruleContext.getPrerequisite("module_map", Mode.HOST);
this.moduleMapArtifact = ruleContext.getPrerequisiteArtifact("module_map", Mode.HOST);
this.zipper = ruleContext.getPrerequisiteArtifact(":zipper", Mode.HOST);
Expand Down Expand Up @@ -376,22 +363,6 @@ public NestedSet<Artifact> getLibc() {
return libc;
}

public NestedSet<Artifact> getTargetLibc() {
return targetLibc;
}

public TransitiveInfoCollection getTargetLibcTop() {
return targetLibcTop;
}

public Label getLibcTopLabel() {
return getLibcTop() == null ? null : getLibcTop().getLabel();
}

public Label getTargetLibcTopLabel() {
return getTargetLibcTop() == null ? null : getTargetLibcTop().getLabel();
}

public Label getLibcTopAttribute() {
return libcTopAttribute;
}
Expand Down Expand Up @@ -458,6 +429,10 @@ private static NestedSet<Artifact> fullInputsForLink(
}
return builder.build();
}

public Label getLibcTopLabel() {
return getLibcTop() == null ? null : getLibcTop().getLabel();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ public final class CcToolchainProvider extends ToolchainInfo
/* dwpFiles= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* coverageFiles= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* libcLink= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* targetLibcLink= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* staticRuntimeLinkInputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* staticRuntimeLinkMiddleman= */ null,
/* dynamicRuntimeLinkInputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
Expand All @@ -79,11 +78,9 @@ public final class CcToolchainProvider extends ToolchainInfo
(buildOptions) -> CcToolchainVariables.EMPTY,
CcToolchainVariables.EMPTY,
/* builtinIncludeFiles= */ ImmutableList.of(),
/* targetBuiltinIncludeFiles= */ ImmutableList.of(),
/* linkDynamicLibraryTool= */ null,
/* builtInIncludeDirectories= */ ImmutableList.of(),
/* sysroot= */ null,
/* targetSysroot= */ null,
/* fdoContext= */ null,
/* isHostConfiguration= */ false,
/* licensesProvider= */ null);
Expand All @@ -104,7 +101,6 @@ public final class CcToolchainProvider extends ToolchainInfo
private final NestedSet<Artifact> dwpFiles;
private final NestedSet<Artifact> coverageFiles;
private final NestedSet<Artifact> libcLink;
private final NestedSet<Artifact> targetLibcLink;
@Nullable private final NestedSet<Artifact> staticRuntimeLinkInputs;
@Nullable private final Artifact staticRuntimeLinkMiddleman;
@Nullable private final NestedSet<Artifact> dynamicRuntimeLinkInputs;
Expand All @@ -116,11 +112,9 @@ public final class CcToolchainProvider extends ToolchainInfo
private final AdditionalBuildVariablesComputer additionalBuildVariablesComputer;
private final CcToolchainVariables buildVariables;
private final ImmutableList<Artifact> builtinIncludeFiles;
private final ImmutableList<Artifact> targetBuiltinIncludeFiles;
@Nullable private final Artifact linkDynamicLibraryTool;
private final ImmutableList<PathFragment> builtInIncludeDirectories;
@Nullable private final PathFragment sysroot;
private final PathFragment targetSysroot;
private final boolean isHostConfiguration;
/**
* WARNING: We don't like {@link FdoContext}. Its {@link FdoContext#fdoProfilePath} is pure path
Expand Down Expand Up @@ -149,7 +143,6 @@ public CcToolchainProvider(
NestedSet<Artifact> dwpFiles,
NestedSet<Artifact> coverageFiles,
NestedSet<Artifact> libcLink,
NestedSet<Artifact> targetLibcLink,
NestedSet<Artifact> staticRuntimeLinkInputs,
@Nullable Artifact staticRuntimeLinkMiddleman,
NestedSet<Artifact> dynamicRuntimeLinkInputs,
Expand All @@ -161,11 +154,9 @@ public CcToolchainProvider(
AdditionalBuildVariablesComputer additionalBuildVariablesComputer,
CcToolchainVariables buildVariables,
ImmutableList<Artifact> builtinIncludeFiles,
ImmutableList<Artifact> targetBuiltinIncludeFiles,
Artifact linkDynamicLibraryTool,
ImmutableList<PathFragment> builtInIncludeDirectories,
@Nullable PathFragment sysroot,
@Nullable PathFragment targetSysroot,
FdoContext fdoContext,
boolean isHostConfiguration,
LicensesProvider licensesProvider) {
Expand All @@ -186,7 +177,6 @@ public CcToolchainProvider(
this.dwpFiles = Preconditions.checkNotNull(dwpFiles);
this.coverageFiles = Preconditions.checkNotNull(coverageFiles);
this.libcLink = Preconditions.checkNotNull(libcLink);
this.targetLibcLink = Preconditions.checkNotNull(targetLibcLink);
this.staticRuntimeLinkInputs = staticRuntimeLinkInputs;
this.staticRuntimeLinkMiddleman = staticRuntimeLinkMiddleman;
this.dynamicRuntimeLinkInputs = dynamicRuntimeLinkInputs;
Expand All @@ -202,11 +192,9 @@ public CcToolchainProvider(
this.additionalBuildVariablesComputer = additionalBuildVariablesComputer;
this.buildVariables = buildVariables;
this.builtinIncludeFiles = builtinIncludeFiles;
this.targetBuiltinIncludeFiles = targetBuiltinIncludeFiles;
this.linkDynamicLibraryTool = linkDynamicLibraryTool;
this.builtInIncludeDirectories = builtInIncludeDirectories;
this.sysroot = sysroot;
this.targetSysroot = targetSysroot;
this.fdoContext = fdoContext == null ? FdoContext.getDisabledContext() : fdoContext;
this.isHostConfiguration = isHostConfiguration;
this.licensesProvider = licensesProvider;
Expand Down Expand Up @@ -426,12 +414,8 @@ public NestedSet<Artifact> getCoverageFiles() {
return coverageFiles;
}

public NestedSet<Artifact> getLibcLink(CppConfiguration cppConfiguration) {
if (cppConfiguration.equals(getCppConfigurationEvenThoughItCanBeDifferentThatWhatTargetHas())) {
return libcLink;
} else {
return targetLibcLink;
}
public NestedSet<Artifact> getLibcLink() {
return libcLink;
}

/**
Expand Down Expand Up @@ -611,7 +595,7 @@ public CcToolchainVariables getBuildVariables(
return CcToolchainProviderHelper.getBuildVariables(
buildOptions,
cppConfiguration,
getSysrootPathFragment(cppConfiguration),
getSysrootPathFragment(),
additionalBuildVariablesComputer);
}
return buildVariables;
Expand All @@ -620,15 +604,9 @@ public CcToolchainVariables getBuildVariables(
/**
* Return the set of include files that may be included even if they are not mentioned in the
* source file or any of the headers included by it.
*
* @param cppConfiguration
*/
public ImmutableList<Artifact> getBuiltinIncludeFiles(CppConfiguration cppConfiguration) {
if (cppConfiguration.equals(getCppConfigurationEvenThoughItCanBeDifferentThatWhatTargetHas())) {
return builtinIncludeFiles;
} else {
return targetBuiltinIncludeFiles;
}
public ImmutableList<Artifact> getBuiltinIncludeFiles() {
return builtinIncludeFiles;
}

/**
Expand All @@ -652,12 +630,8 @@ public String getSysroot() {
return sysroot != null ? sysroot.getPathString() : null;
}

public PathFragment getSysrootPathFragment(CppConfiguration cppConfiguration) {
if (cppConfiguration.equals(getCppConfigurationEvenThoughItCanBeDifferentThatWhatTargetHas())) {
return sysroot;
} else {
return targetSysroot;
}
public PathFragment getSysrootPathFragment() {
return sysroot;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,6 @@ public static CcToolchainProvider getCcToolchainProvider(

PathFragment sysroot =
calculateSysroot(attributes.getLibcTopLabel(), toolchainInfo.getDefaultSysroot());
PathFragment targetSysroot =
calculateSysroot(attributes.getTargetLibcTopLabel(), toolchainInfo.getDefaultSysroot());

ImmutableList.Builder<PathFragment> builtInIncludeDirectoriesBuilder = ImmutableList.builder();
for (String s : toolchainInfo.getRawBuiltInIncludeDirectories()) {
Expand Down Expand Up @@ -248,7 +246,6 @@ public static CcToolchainProvider getCcToolchainProvider(
attributes.getDwpFiles(),
attributes.getCoverage(),
attributes.getLibc(),
attributes.getTargetLibc(),
staticRuntimeLinkInputs,
staticRuntimeLinkMiddleman,
dynamicRuntimeLinkSymlinks,
Expand All @@ -264,11 +261,9 @@ public static CcToolchainProvider getCcToolchainProvider(
sysroot,
attributes.getAdditionalBuildVariablesComputer()),
getBuiltinIncludes(attributes.getLibc()),
getBuiltinIncludes(attributes.getTargetLibc()),
attributes.getLinkDynamicLibraryTool(),
builtInIncludeDirectories,
sysroot,
targetSysroot,
fdoContext,
configuration.isHostConfiguration(),
attributes.getLicensesProvider());
Expand Down Expand Up @@ -507,6 +502,20 @@ private static CppModuleMap createCrosstoolModuleMap(CcToolchainAttributesProvid
return new CppModuleMap(moduleMapArtifact, "crosstool");
}

static TransitiveInfoCollection selectDep(
ImmutableList<? extends TransitiveInfoCollection> deps, Label label) {
if (deps.isEmpty()) {
return null;
}
for (TransitiveInfoCollection dep : deps) {
if (dep.getLabel().equals(label)) {
return dep;
}
}

return deps.get(0);
}

/**
* Returns {@link CcToolchainVariables} instance with build variables that only depend on the
* toolchain.
Expand Down
Loading

0 comments on commit 6027c20

Please sign in to comment.