Skip to content

Commit

Permalink
Support select() on constraint_value for aliases.
Browse files Browse the repository at this point in the history
This implements approach #4 of
bazelbuild#13047 (comment).

The basic change adds a new toolchain resolution mode: "resolve iff the target has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platform info to *all* rules (bazelbuild#12899 (comment)).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes bazelbuild#13047.

Closes bazelbuild#14310.

PiperOrigin-RevId: 411868223
  • Loading branch information
gregestren authored and copybara-github committed Nov 23, 2021
1 parent 89ea68b commit 1c3a245
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 13 deletions.
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,
/** 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
5 changes: 3 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/Alias.java
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

0 comments on commit 1c3a245

Please sign in to comment.