Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check the toolchain transition migration status and use the new transition #11583

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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