Skip to content

Commit

Permalink
Check the toolchain transition migration status and use the new
Browse files Browse the repository at this point in the history
transition if appropriate.

Adds a test for the new toolchain transition functionality.

Adds the --incompatible_use_toolchain_transition flag to force all rules to depend on toolchains via the toolchain transition.
Adds the incompatible_use_toolchain_transition rule attribute to force a
specific rule to depend on toolchains via the toolchain transition.

RELNOTES: Rule authors should use the
incompatible_use_toolchain_transition rule attribute to migrate to using
the toolchain transition. jcater to udpate notes further.

Closes bazelbuild#10523.
  • Loading branch information
katre committed Jun 11, 2020
1 parent cb798a4 commit a01fe35
Show file tree
Hide file tree
Showing 10 changed files with 476 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,17 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
return OrderedSetMultimap.create();
}

// TODO(#10523): Remove this when the migration period for toolchain transitions has ended.
boolean useToolchainTransition =
shouldUseToolchainTransition(node.getConfiguration(), fromRule);
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps =
partiallyResolveDependencies(
outgoingLabels, fromRule, attributeMap, toolchainContexts, aspects);
outgoingLabels,
fromRule,
attributeMap,
toolchainContexts,
useToolchainTransition,
aspects);

OrderedSetMultimap<DependencyKind, DependencyKey> outgoingEdges =
fullyResolveDependencies(
Expand All @@ -250,6 +258,29 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
return outgoingEdges;
}

/**
* Returns whether or not to use the new toolchain transition. Checks the global incompatible
* change flag and the rule's toolchain transition readiness attribute.
*/
private boolean shouldUseToolchainTransition(
@Nullable BuildConfiguration configuration, @Nullable Rule fromRule) {
// Check whether the global incompatible change flag is set.
if (configuration != null) {
PlatformOptions platformOptions = configuration.getOptions().get(PlatformOptions.class);
if (platformOptions != null && platformOptions.overrideToolchainTransition) {
return true;
}
}

// Check the rule definition to see if it is ready.
if (fromRule != null && fromRule.getRuleClassObject().useToolchainTransition()) {
return true;
}

// Default to false.
return false;
}

/**
* Factor in the properties of the current rule into the dependency edge calculation.
*
Expand All @@ -264,6 +295,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
@Nullable Rule fromRule,
ConfiguredAttributeMapper attributeMap,
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
boolean useToolchainTransition,
Iterable<Aspect> aspects)
throws EvalException {
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps =
Expand All @@ -278,15 +310,29 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
// use sensible defaults. Not depending on their package makes the error message reporting
// a missing toolchain a bit better.
// TODO(lberki): This special-casing is weird. Find a better way to depend on toolchains.
partiallyResolvedDeps.put(
TOOLCHAIN_DEPENDENCY,
PartiallyResolvedDependency.builder()
.setLabel(toLabel)
// TODO(#10523): Replace this with a proper transition for the execution platform.
.setTransition(HostTransition.INSTANCE)
// TODO(#10523): Set the toolchainContextKey from ToolchainCollection.
.setPropagatingAspects(ImmutableList.of())
.build());
// TODO(#10523): Remove check when this is fully released.
if (useToolchainTransition) {
// We need to create an individual PRD for each distinct toolchain context that contains
// this toolchain, because each has a different ToolchainContextKey.
for (ToolchainContext toolchainContext :
toolchainContexts.getContextsForResolvedToolchain(toLabel)) {
partiallyResolvedDeps.put(
TOOLCHAIN_DEPENDENCY,
PartiallyResolvedDependency.builder()
.setLabel(toLabel)
.setTransition(NoTransition.INSTANCE)
.setToolchainContextKey(toolchainContext.key())
.build());
}
} else {
// Legacy approach: use a HostTransition.
partiallyResolvedDeps.put(
TOOLCHAIN_DEPENDENCY,
PartiallyResolvedDependency.builder()
.setLabel(toLabel)
.setTransition(HostTransition.INSTANCE)
.build());
}
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,19 @@ public class PlatformOptions extends FragmentOptions {
+ " 'test'.")
public List<Map.Entry<RegexFilter, List<Label>>> targetFilterToAdditionalExecConstraints;

@Option(
name = "incompatible_override_toolchain_transition",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = OptionEffectTag.LOADING_AND_ANALYSIS,
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"If set to true, all rules will use the toolchain transition for toolchain dependencies.")
public boolean overrideToolchainTransition;

@Override
public PlatformOptions getHost() {
PlatformOptions host = (PlatformOptions) getDefault();
Expand All @@ -232,6 +245,7 @@ public PlatformOptions getHost() {
host.autoConfigureHostPlatform = this.autoConfigureHostPlatform;
host.useToolchainResolutionForJavaRules = this.useToolchainResolutionForJavaRules;
host.targetPlatformFallback = this.targetPlatformFallback;
host.overrideToolchainTransition = this.overrideToolchainTransition;
return host;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.Label;
Expand Down Expand Up @@ -72,6 +73,16 @@ public static <T extends ToolchainContext> Builder<T> builder() {
return new Builder<T>();
}

/**
* Returns every instance of {@link ToolchainContext} that uses {@code resolvedToolchainLabel} as
* a resolved toolchain.
*/
public ImmutableList<T> getContextsForResolvedToolchain(Label resolvedToolchainLabel) {
return getContextMap().values().stream()
.filter(tc -> tc.resolvedToolchainLabels().contains(resolvedToolchainLabel))
.collect(ImmutableList.toImmutableList());
}

/** Builder for ToolchainCollection. */
public static final class Builder<T extends ToolchainContext> {
// This is not immutable so that we can check for duplicate keys easily.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ public StarlarkCallable rule(
Sequence<?> hostFragments,
Boolean starlarkTestable,
Sequence<?> toolchains,
boolean useToolchainTransition,
String doc,
Sequence<?> providesArg,
Sequence<?> execCompatibleWith,
Expand Down Expand Up @@ -353,6 +354,7 @@ public StarlarkCallable rule(
bzlModule.label(), bzlModule.bzlTransitiveDigest());

builder.addRequiredToolchains(parseToolchains(toolchains, thread));
builder.useToolchainTransition(useToolchainTransition);

if (execGroups != Starlark.NONE) {
Map<String, ExecGroup> execGroupDict =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ public enum ThirdPartyLicenseExistencePolicy {
private final Map<String, Attribute> attributes = new LinkedHashMap<>();
private final Set<Label> requiredToolchains = new HashSet<>();
private boolean useToolchainResolution = true;
private boolean useToolchainTransition = false;
private Set<Label> executionPlatformConstraints = new HashSet<>();
private OutputFile.Kind outputFileKind = OutputFile.Kind.FILE;
private final Map<String, ExecGroup> execGroups = new HashMap<>();
Expand Down Expand Up @@ -750,6 +751,7 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p

addRequiredToolchains(parent.getRequiredToolchains());
useToolchainResolution = parent.useToolchainResolution;
useToolchainTransition = parent.useToolchainTransition;
addExecutionPlatformConstraints(parent.getExecutionPlatformConstraints());
try {
addExecGroups(parent.getExecGroups());
Expand Down Expand Up @@ -837,6 +839,7 @@ public RuleClass build(String name, String key) {
// Build setting rules should opt out of toolchain resolution, since they form part of the
// configuration.
this.useToolchainResolution(false);
this.useToolchainTransition(false);
}

return new RuleClass(
Expand Down Expand Up @@ -871,6 +874,7 @@ public RuleClass build(String name, String key) {
thirdPartyLicenseExistencePolicy,
requiredToolchains,
useToolchainResolution,
useToolchainTransition,
executionPlatformConstraints,
execGroups,
outputFileKind,
Expand Down Expand Up @@ -1453,6 +1457,11 @@ public Builder useToolchainResolution(boolean flag) {
return this;
}

public Builder useToolchainTransition(boolean flag) {
this.useToolchainTransition = flag;
return this;
}

/**
* Adds additional execution platform constraints that apply for all targets from this rule.
*
Expand Down Expand Up @@ -1604,6 +1613,7 @@ public Attribute.Builder<?> copy(String name) {

private final ImmutableSet<Label> requiredToolchains;
private final boolean useToolchainResolution;
private final boolean useToolchainTransition;
private final ImmutableSet<Label> executionPlatformConstraints;
private final ImmutableMap<String, ExecGroup> execGroups;

Expand Down Expand Up @@ -1660,6 +1670,7 @@ public Attribute.Builder<?> copy(String name) {
ThirdPartyLicenseExistencePolicy thirdPartyLicenseExistencePolicy,
Set<Label> requiredToolchains,
boolean useToolchainResolution,
boolean useToolchainTransition,
Set<Label> executionPlatformConstraints,
Map<String, ExecGroup> execGroups,
OutputFile.Kind outputFileKind,
Expand Down Expand Up @@ -1700,6 +1711,7 @@ public Attribute.Builder<?> copy(String name) {
this.thirdPartyLicenseExistencePolicy = thirdPartyLicenseExistencePolicy;
this.requiredToolchains = ImmutableSet.copyOf(requiredToolchains);
this.useToolchainResolution = useToolchainResolution;
this.useToolchainTransition = useToolchainTransition;
this.executionPlatformConstraints = ImmutableSet.copyOf(executionPlatformConstraints);
this.execGroups = ImmutableMap.copyOf(execGroups);
this.buildSetting = buildSetting;
Expand Down Expand Up @@ -2612,6 +2624,10 @@ public boolean useToolchainResolution() {
return useToolchainResolution;
}

public boolean useToolchainTransition() {
return useToolchainTransition;
}

public ImmutableSet<Label> getExecutionPlatformConstraints() {
return executionPlatformConstraints;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,15 @@ public interface StarlarkRuleFunctionsApi<FileApiT extends FileApi> {
"If set, the set of toolchains this rule requires. Toolchains will be "
+ "found by checking the current platform, and provided to the rule "
+ "implementation via <code>ctx.toolchain</code>."),
@Param(
name = "incompatible_use_toolchain_transition",
type = Boolean.class,
defaultValue = "False",
named = true,
doc =
"If set, this rule will use the toolchain transition for toolchain dependencies."
+ " This is ignored if the --incompatible_use_toolchain_transition flag is"
+ " set."),
@Param(
name = "doc",
type = String.class,
Expand Down Expand Up @@ -356,6 +365,7 @@ StarlarkCallable rule(
Sequence<?> hostFragments,
Boolean starlarkTestable,
Sequence<?> toolchains,
boolean useToolchainTransition,
String doc,
Sequence<?> providesArg,
Sequence<?> execCompatibleWith,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ public StarlarkCallable rule(
Sequence<?> hostFragments,
Boolean starlarkTestable,
Sequence<?> toolchains,
boolean useToolchainTransition,
String doc,
Sequence<?> providesArg,
Sequence<?> execCompatibleWith,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,7 @@ private static RuleClass newRuleClass(
ThirdPartyLicenseExistencePolicy.USER_CONTROLLABLE,
/*requiredToolchains=*/ ImmutableSet.of(),
/*useToolchainResolution=*/ true,
/*useToolchainTransition=*/ true,
/* executionPlatformConstraints= */ ImmutableSet.of(),
/* execGroups= */ ImmutableMap.of(),
OutputFile.Kind.FILE,
Expand Down
7 changes: 7 additions & 0 deletions src/test/shell/bazel/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,13 @@ sh_test(
tags = ["no_windows"],
)

sh_test(
name = "toolchain_transition_test",
srcs = ["toolchain_transition_test.sh"],
data = [":test-deps"],
tags = ["no_windows"],
)

sh_test(
name = "java_launcher_test",
size = "medium",
Expand Down
Loading

0 comments on commit a01fe35

Please sign in to comment.