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

Support select() on constraint_value for aliases. #14310

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
26 changes: 26 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/Rule.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.StarlarkImplicitOutputsFunction;
import com.google.devtools.build.lib.packages.License.DistributionType;
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand Down Expand Up @@ -942,6 +943,31 @@ public Collection<Label> getAspectLabelsSuperset(DependencyFilter predicate) {
return labels.values();
}

/**
* Should this rule instance resolve toolchains?
*
* <p>This may happen for two reasons:
*
* <ol>
* <li>The rule uses toolchains by definition ({@link
* RuleClass.Builder#useToolchainResolution(ToolchainResolutionMode)}
* <li>The rule instance has a select(), which means it may depend on target platform properties
* that are only provided when toolchain resolution is enabled.
* </ol>
*/
public boolean useToolchainResolution() {
ToolchainResolutionMode mode = ruleClass.useToolchainResolution();
if (mode.isActive()) {
return true;
} else if (mode == ToolchainResolutionMode.HAS_SELECT) {
RawAttributeMapper attr = RawAttributeMapper.of(this);
return (attr.has(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
&& !attr.get(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE, BuildType.LABEL_LIST).isEmpty());
} else {
return false;
}
}

/**
* @return The repository name.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,22 @@ public enum ToolchainResolutionMode {
ENABLED,
/** The rule should not use toolchain resolution. */
DISABLED,
/**
* The rule instance uses toolchain resolution if it has a select().
*
* <p>This is for rules that don't intrinsically use toolchains but have select()s on {@link
* com.google.devtools.build.lib.rules.platform.ConstraintValue}, which are part of the build's
* platform. Such instances need to know what platform the build is targeting, which Bazel won't
* provide unless toolchain resolution is enabled.
*
* <p>This is set statically in rule definitions on an opt-in basis. Bazel doesn't automatically
* infer this for any target with a select().
*
* <p>Ultimately, we should remove this when <a
* href="https://github.com/bazelbuild/bazel/issues/12899#issuecomment-767759147}#12899</a>is
* addressed, so platforms are unconditionally provided for all rules.
*/
HAS_SELECT,
gregestren marked this conversation as resolved.
Show resolved Hide resolved
/** The rule should inherit the value from its parent rules. */
INHERIT;

Expand All @@ -245,6 +261,7 @@ boolean isActive() {
case ENABLED:
return true;
case DISABLED:
case HAS_SELECT: // Not true for RuleClass, but Rule may enable it.
return false;
default:
}
Expand Down Expand Up @@ -380,7 +397,7 @@ public RuleErrorException(String message, Throwable cause) {
* normal dependency resolution because they're needed to determine other dependencies. So there's
* no intrinsic reason why we need an extra attribute to store them.
*
* <p>There are three reasons why we still create this attribute:
* <p>There are four reasons why we still create this attribute:
*
* <ol>
* <li>Collecting them once in {@link #populateRuleAttributeValues} instead of multiple times in
Expand All @@ -390,6 +407,9 @@ public RuleErrorException(String message, Throwable cause) {
* we need to make sure its coverage remains complete.
* <li>Manual configuration trimming uses the normal dependency resolution process to work
* correctly and config_setting keys are subject to this trimming.
* <li>{@link Rule#useToolchainResolution() supports conditional toolchain resolution for
* targets with non-empty select()s. This requirement would go away if platform info was
* prepared for all rules regardless of toolchain needs.
* </ol>
*
* <p>It should be possible to clean up these issues if we decide we don't want an artificial
Expand Down Expand Up @@ -2753,8 +2773,12 @@ public ImmutableSet<Label> getRequiredToolchains() {
return requiredToolchains;
}

public boolean useToolchainResolution() {
return this.useToolchainResolution.isActive();
/**
* Public callers should use {@link Rule#useToolchainResolution()}, which also takes into account
* target-specific information.
*/
ToolchainResolutionMode useToolchainResolution() {
return this.useToolchainResolution;
}

public boolean useToolchainTransition() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ private static ToolchainCollection<ToolchainContext> getToolchainContexts(
}

Rule rule = ((Rule) target);
if (!rule.getRuleClassObject().useToolchainResolution()) {
if (!rule.useToolchainResolution()) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
.canHaveAnyProvider()
// Aliases themselves do not need toolchains or an execution platform, so this is fine.
// The actual target will resolve platforms and toolchains with no issues regardless of
// this setting.
.useToolchainResolution(ToolchainResolutionMode.DISABLED)
// this setting. The only time an alias directly needs the platform is when it has a
// select() on a constraint_setting, so special-case enable those instances too.
.useToolchainResolution(ToolchainResolutionMode.HAS_SELECT)
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ public static ComputedToolchainContexts computeUnloadedToolchainContexts(
ExecGroupCollection.builder(defaultExecGroup, rule.getRuleClassObject().getExecGroups());

// Short circuit and end now if this target doesn't require toolchain resolution.
if (!rule.getRuleClassObject().useToolchainResolution()) {
if (!rule.useToolchainResolution()) {
ComputedToolchainContexts result = new ComputedToolchainContexts();
result.execGroupCollectionBuilder = execGroupCollectionBuilder;
return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.server.FailureDetails.Toolchain.Code;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
Expand Down Expand Up @@ -161,16 +161,18 @@ private static PlatformInfo findPlatformInfo(
}

static boolean hasPlatformInfo(Target target) {
Rule rule = target.getAssociatedRule();
// If the rule uses toolchain resolution, it can't be used as a target or exec platform.
if (target.getAssociatedRule() == null) {
if (rule == null) {
return false;
}
RuleClass ruleClass = target.getAssociatedRule().getRuleClassObject();
if (ruleClass == null || ruleClass.useToolchainResolution()) {
if (rule.useToolchainResolution()) {
return false;
}

return ruleClass.getAdvertisedProviders().advertises(PlatformInfo.PROVIDER.id());
return rule.getRuleClassObject()
.getAdvertisedProviders()
.advertises(PlatformInfo.PROVIDER.id());
}

/** Exception used when a platform label is not a valid platform. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1290,6 +1290,49 @@ public void nonToolchainResolvingTargetsCantSelectDirectlyOnConstraints() throws
assertContainsEvent("//conditions:apple is not a valid select() condition for //check:hello");
}

@Test
public void selectOnlyToolchainResolvingTargetsCanSelectDirectlyOnConstraints() throws Exception {
// Tests select()ing directly on a constraint_value when the rule uses toolchain resolution
// *only if it has a select()*. As of this test, alias() is the only rule that supports that
// (see Alias#useToolchainResolution(ToolchainResolutionMode.HAS_SELECT).
scratch.file(
"conditions/BUILD",
"constraint_setting(name = 'fruit')",
"constraint_value(name = 'apple', constraint_setting = 'fruit')",
"constraint_value(name = 'banana', constraint_setting = 'fruit')",
"platform(",
" name = 'apple_platform',",
" constraint_values = [':apple'],",
")");
scratch.file(
"check/defs.bzl",
"def _impl(ctx):",
" pass",
"simple_rule = rule(",
" implementation = _impl,",
" attrs = {}",
")");
scratch.file(
"check/BUILD",
"load('//check:defs.bzl', 'simple_rule')",
"filegroup(name = 'bdep', srcs = ['bfile'])",
"simple_rule(name = 'hello')",
"simple_rule(name = 'tere')",
"alias(",
" name = 'selectable_alias',",
" actual = select({",
" '//conditions:apple': ':hello',",
" '//conditions:banana': ':tere',",
" }))");
useConfiguration("--platforms=//conditions:apple_platform");
assertThat(
getConfiguredTarget("//check:selectable_alias")
.getActual()
.getLabel()
.getCanonicalForm())
.isEqualTo("//check:hello");
}

@Test
public void multipleMatchErrorWhenAliasResolvesToSameSetting() throws Exception {
scratch.file(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,9 @@ public void testAlias_withSelect() throws Exception {
myAliasRuleProto.forEach(
configuredTarget -> depNames.add(configuredTarget.getTarget().getRule().getName()));
assertThat(depNames)
.containsExactly("//test:my_alias_rule", "//test:config1", "//test:target1");
// The alias also includes platform info since aliases with select() trigger toolchain
// resolution. We're not interested in those here.
.containsAtLeast("//test:my_alias_rule", "//test:config1", "//test:target1");
}

private MockRule getSimpleRule() {
Expand Down