Skip to content

Commit

Permalink
Support constraint_value directly in select()
Browse files Browse the repository at this point in the history
No more need for a redundant config_setting. See #8583 for an example.

This was a bit subtle. constraint_value can't directly export a ConfigMatchingProvider because it needs to know the
platform to determine if it's a match. But platforms are built out of constraint_values, so the platform isn't available
yet. So the parent target with the select() provides this detail.

Also beautifies "invalid select() key" errors in support of #11984.

Fixes #8583.

RELNOTES[NEW]: select() directly supports constraint_value (no need for an intermediate config_setting).

Closes #12071.

PiperOrigin-RevId: 331181346
  • Loading branch information
gregestren authored and copybara-github committed Sep 11, 2020
1 parent f5c9e77 commit 12b06b2
Show file tree
Hide file tree
Showing 10 changed files with 235 additions and 75 deletions.
31 changes: 27 additions & 4 deletions site/docs/configurable-attributes.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,15 @@ build parameters, which include `--cpu=arm`. The `tools` attribute changes
## Configuration conditions

Each key in a configurable attribute is a label reference to a
[`config_setting`](be/general.html#config_setting). This is just a collection of
[`config_setting`](be/general.html#config_setting) or
[`constraint_value`](be/platform.html#constraint_value).

`config_setting` is just a collection of
expected command line flag settings. By encapsulating these in a target, it's
easy to maintain "standard" conditions users can reference from multiple places.

`constraint_value` provides support for [multi-platform behavior](#platforms).


### Built-in flags

Expand Down Expand Up @@ -263,7 +268,7 @@ While the ability to specify multiple flags on the command line provides
flexibility, it can also be burdensome to individually set each one every time
you want to build a target.
[Platforms](platforms.html)
allow you to consolidate these into simple bundles.
let you consolidate these into simple bundles.

```python
# myapp/BUILD
Expand Down Expand Up @@ -338,8 +343,26 @@ Without platforms, this might look something like
bazel build //my_app:my_rocks --define color=white --define texture=smooth --define type=metamorphic
```

Platforms are still under development. See the [documentation](platforms.html)
and [roadmap](https://bazel.build/roadmaps/platforms.html) for details.
`select()` can also directly read `constraint_value`s:

```python
constraint_setting(name = "type")
constraint_value(name = "igneous", constraint_setting = "type")
constraint_value(name = "metamorphic", constraint_setting = "type")
sh_binary(
name = "my_rocks",
srcs = select({
":igneous": ["igneous.sh"],
":metamorphic" ["metamorphic.sh"],
}),
)
```

This saves the need for boilerplate `config_setting`s when you only need to
check against single values.

Platforms are still under development. See the
[documentation](platforms-intro.html) for details.

## Combining `select()`s

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,24 +150,20 @@ config_setting(
<p>
The <a href="$expander.expandRef("select")"><code>select()</code></a> function
chooses among different alternative values for a configurable attribute based
on which <a href="$expander.expandRef("config_setting")">
<code>config_setting</code></a> criteria is satisfied in the current
configuration.
on which <a href="$expander.expandRef("config_setting")"> or
<a href="$expander.expandRef("constraint_value")"> criteria the target's
configuration satisfies.
</p>

<p>
Configurable attributes are evaluated after the processing of macros and
before the processing of rules (technically, between the
Bazel evaluates configurable attributes after processing macros and before
processing rules (technically, between the
<a href="https://docs.bazel.build/versions/master/skylark/concepts.html#evaluation-model">
loading and analysis phases</a>.
Any processing that Bazel does before the <code>select()</code> is evaluated
will not know which branch will be chosen. In particular, macros can't change
loading and analysis phases</a>).
Any processing before <code>select()</code> evaluation doesn't know which
branch the <code>select()</code> chooses. Macros, for example, can't change
their behavior based on the chosen branch, and <code>bazel query</code> can
only make conservative guesses about the configurable dependencies of a
target. Conversely, when authoring a new type of rule, you do not need to
worry about the ambiguity of configurable attributes because all
<code>select()</code> expressions have already been replaced by their resolved
values. See
only make conservative guesses about a target's configurable dependencies. See
<a href="https://docs.bazel.build/versions/master/configurable-attributes.html#faq">
this FAQ</a>
for more on using <code>select()</code> with rules and macros.
Expand All @@ -176,13 +172,13 @@ config_setting(
<p>
Attributes marked <code>nonconfigurable</code> in their documentation cannot
use this feature. Usually an attribute is nonconfigurable because Bazel
internally needs to know its value before it can determine how to choose the
<code>select()</code> branch.
internally needs to know its value before it can determine how to resolve a
<code>select()</code>.
</p>

<p>
See <a href="https://docs.bazel.build/versions/master/configurable-attributes.html">
Configurable Build Attributes</a> for more information.
Configurable Build Attributes</a> for a detailed overview.
</p>

<h2 id="implicit-outputs">Implicit output targets</h2>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -577,20 +577,20 @@ select(

<i>almost</i>
any attribute assignment so its value depends on command-line Bazel flags.
This can be used, for example, to define platform-specific dependencies or to
You can use this, for example, to define platform-specific dependencies or to
embed different resources depending on whether a rule is built in "developer"
vs. "release" mode.
</p>

<p>Basic usage is as follows:</p>
<p>Basic use is as follows:</p>

<pre class="code">
sh_binary(
name = "myrule",
name = "mytarget",
srcs = select({
":conditionA": ["myrule_a.sh"],
":conditionB": ["myrule_b.sh"],
"//conditions:default": ["myrule_default.sh"]
":conditionA": ["mytarget_a.sh"],
":conditionB": ["mytarget_b.sh"],
"//conditions:default": ["mytarget_default.sh"]
})
)
</pre>
Expand All @@ -600,13 +600,14 @@ sh_binary(
list assignment with a <code>select</code> call that maps
configuration conditions to matching values. Each condition is a label
reference to
a <code><a href="general.html#config_setting">config_setting</a></code> instance,
which "matches" if the rule's configuration matches an expected set of
values. The value of <code>myrule#srcs</code> then becomes whichever
a <code><a href="general.html#config_setting">config_setting</a></code> or
<code><a href="platform.html#constraint_value">constraint_value</a></code>,
which "matches" if the target's configuration matches an expected set of
values. The value of <code>mytarget#srcs</code> then becomes whichever
label list matches the current invocation.
</p>

<p>Further notes:</p>
<p>Notes:</p>

<ul>
<li>Exactly one condition is selected on any invocation.
Expand Down Expand Up @@ -646,7 +647,7 @@ Conditions checked:
//pkg:conditionB.
</pre>

<code>no_match_error</code> can be used to signal more precise errors.
You can signal more precise errors with <code>no_match_error</code>.

<h3 id="select_example">Examples</h3>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ java_library(
),
deps = [
"//src/main/java/com/google/devtools/build/lib:syntax",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/concurrent",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@

package com.google.devtools.build.lib.analysis.platform;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.BuiltinProvider;
Expand Down Expand Up @@ -58,6 +62,31 @@ public Label label() {
return label;
}

/**
* Returns a {@link ConfigMatchingProvider} that matches if the owning target's platform includes
* this constraint.
*
* <p>The {@link com.google.devtools.build.lib.rules.platform.ConstraintValue} rule can't directly
* return a {@link ConfigMatchingProvider} because, as part of a platform's definition, it doesn't
* have access to the platform during its analysis.
*
* <p>Instead, a target with a <code>select()</code> on a {@link
* com.google.devtools.build.lib.rules.platform.ConstraintValue} passes its platform info to this
* method.
*/
public ConfigMatchingProvider configMatchingProvider(PlatformInfo platformInfo) {
return new ConfigMatchingProvider(
label,
ImmutableMultimap.of(),
ImmutableMap.of(),
// Technically a select() on a constraint_value requires PlatformConfiguration, since that's
// used to build the platform this checks against. But platformInfo's existence implies
// the owning target already depends on PlatformConfiguration. And we can't reference
// PlatformConfiguration.class here without a build dependency cycle.
ImmutableSet.of(),
platformInfo.constraints().hasConstraintValue(this));
}

@Override
public void repr(Printer printer) {
Printer.format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,19 +373,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
associatedConfiguredTargetAndData.getTarget(), aspectConfiguration);
ImmutableList<Aspect> aspectPath = aspectPathBuilder.build();
try {
// Get the configuration targets that trigger this rule's configurable attributes.
ImmutableMap<Label, ConfigMatchingProvider> configConditions =
ConfiguredTargetFunction.getConfigConditions(
associatedConfiguredTargetAndData.getTarget(),
env,
originalTargetAndAspectConfiguration,
transitivePackagesForPackageRootResolution,
transitiveRootCauses);
if (configConditions == null) {
// Those targets haven't yet been resolved.
return null;
}

// Determine what toolchains are needed by this target.
UnloadedToolchainContext unloadedToolchainContext = null;
if (configuration != null) {
Expand All @@ -408,9 +395,22 @@ public SkyValue compute(SkyKey skyKey, Environment env)
throw new AspectCreationException(
e.getMessage(), new LabelCause(key.getLabel(), e.getMessage()));
}
if (env.valuesMissing()) {
return null;
}
}
if (env.valuesMissing()) {
return null;
}

// Get the configuration targets that trigger this rule's configurable attributes.
ImmutableMap<Label, ConfigMatchingProvider> configConditions =
ConfiguredTargetFunction.getConfigConditions(
env,
originalTargetAndAspectConfiguration,
transitivePackagesForPackageRootResolution,
unloadedToolchainContext == null ? null : unloadedToolchainContext.targetPlatform(),
transitiveRootCauses);
if (configConditions == null) {
// Those targets haven't yet been resolved.
return null;
}

OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> depValueMap;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException;
import com.google.devtools.build.lib.causes.AnalysisFailedCause;
import com.google.devtools.build.lib.causes.Cause;
Expand Down Expand Up @@ -267,13 +269,27 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc
// would exit this SkyFunction and restart it when permits were available.
acquireWithLogging(key);
try {
// Determine what toolchains are needed by this target.
unloadedToolchainContexts =
computeUnloadedToolchainContexts(
env,
ruleClassProvider,
defaultBuildOptions,
ctgValue,
configuredTargetKey.getToolchainContextKey());
if (env.valuesMissing()) {
return null;
}

// Get the configuration targets that trigger this rule's configurable attributes.
ImmutableMap<Label, ConfigMatchingProvider> configConditions =
getConfigConditions(
ctgValue.getTarget(),
env,
ctgValue,
transitivePackagesForPackageRootResolution,
unloadedToolchainContexts == null
? null
: unloadedToolchainContexts.getDefaultToolchainContext().targetPlatform(),
transitiveRootCauses);
if (env.valuesMissing()) {
return null;
Expand All @@ -292,18 +308,6 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc
"Cannot compute config conditions", configuration, transitiveRootCauses.build()));
}

// Determine what toolchains are needed by this target.
unloadedToolchainContexts =
computeUnloadedToolchainContexts(
env,
ruleClassProvider,
defaultBuildOptions,
ctgValue,
configuredTargetKey.getToolchainContextKey());
if (env.valuesMissing()) {
return null;
}

// Calculate the dependencies of this target.
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> depValueMap =
computeDependencies(
Expand Down Expand Up @@ -710,12 +714,13 @@ static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDepend
*/
@Nullable
static ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions(
Target target,
Environment env,
TargetAndConfiguration ctgValue,
@Nullable NestedSetBuilder<Package> transitivePackagesForPackageRootResolution,
@Nullable PlatformInfo platformInfo,
NestedSetBuilder<Cause> transitiveRootCauses)
throws DependencyEvaluationException, InterruptedException {
Target target = ctgValue.getTarget();
if (!(target instanceof Rule)) {
return NO_CONFIG_CONDITIONS;
}
Expand Down Expand Up @@ -789,16 +794,32 @@ static ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions(
// Get the configured targets as ConfigMatchingProvider interfaces.
for (Dependency entry : configConditionDeps) {
SkyKey baseKey = entry.getConfiguredTargetKey();
// The code above guarantees that value is non-null here.
ConfiguredTarget value = configValues.get(baseKey).getConfiguredTarget();
// The code above guarantees that value is non-null here and since the rule is a
// config_setting, provider must also be non-null.
ConfigMatchingProvider provider = value.getProvider(ConfigMatchingProvider.class);
if (provider != null) {
configConditions.put(entry.getLabel(), provider);
// The below handles config_setting (which nativly provides ConfigMatchingProvider) and
// constraint_value (which needs a custom-built ConfigMatchingProvider). If we ever add
// support for more rules we should move resolution logic to ConfigMatchingProvider and
// simplify the logic here.
ConfigMatchingProvider matchingProvider = value.getProvider(ConfigMatchingProvider.class);
ConstraintValueInfo constraintValueInfo = value.get(ConstraintValueInfo.PROVIDER);

if (matchingProvider != null) {
configConditions.put(entry.getLabel(), matchingProvider);
} else if (constraintValueInfo != null && platformInfo != null) {
// If platformInfo == null, that means the owning target doesn't invoke toolchain
// resolution, in which case depending on a constraint_value is non-sensical.
configConditions.put(
entry.getLabel(), constraintValueInfo.configMatchingProvider(platformInfo));
} else {
// Not a valid provider for configuration conditions.
String message =
entry.getLabel() + " is not a valid configuration key for " + target.getLabel();
String.format(
"%s is not a valid select() condition for %s.\n",
entry.getLabel(), target.getLabel())
+ String.format(
"To inspect the select(), run: bazel query --output=build %s.\n",
target.getLabel())
+ "For more help, see https://docs.bazel.build/be/functions.html#select.\n\n";
env.getListener().handle(Event.error(TargetUtils.getLocationMaybe(target), message));
throw new DependencyEvaluationException(
new ConfiguredValueCreationException(
Expand Down
Loading

0 comments on commit 12b06b2

Please sign in to comment.