Skip to content

Commit

Permalink
Enforce visibility on select() keys
Browse files Browse the repository at this point in the history
Example:

```
my_rule(
  name = "buildme",
  deps = select({
    "//other/package:some_config": [":mydeps"]
  }))
```

Today, `//other/package:some_config` is exempt from visibility checking, even though it's technically a target dep of
 `buildme`.

While this dep is "special" vs. other deps in various ways, there's no obvious reason why it needs to be special in this way. It adds an unclear corner case exception to visibility's API.

### Implementation:
select() keys are not "normal" dependencies and don't generally follow the same code path. Hence them not being automatically visibility checked like others.

In particular, normal dependencies are found in `ConfiguredTargetFunction` and validity-checked in `RuleContext.Builder`. select() keys' only purpose is to figure out which other normal dependencies should exist. There's generally no need to pass them to `RuleContext.Builder`. Instead, Blaze passes their `ConfigMatchingProvider`s, which remain useful for analysis phase attribute lookups.

`RuleContext.Builder` needs a `ConfiguredTargetAndData` to do validity-checking.  This patch propagates that information for select() keys too.

We could alternatively refactor the validity checking logic. But that's an even more invasive change. Or do ad hoc validity checking directly in `ConfiguredTargetFunction`. But that's duplicating logic we really want to keep consolidated.

### Backward compatibility:
This would break existing builds if `config_setting` defaulted to private visibility. So this change specially defaults `config_setting` to public visibility, with clarifying documentation. When ready we'll want to create an incompatible change to make `config_setting` the same as everything else.

Fixes #12669.

Closes #12877.

RELNOTES: config_setting now honors `visibility` attribute (and defaults to `//visibility:public`)
PiperOrigin-RevId: 354310777
  • Loading branch information
gregestren authored and copybara-github committed Jan 28, 2021
1 parent a2b7e40 commit cac82cf
Show file tree
Hide file tree
Showing 13 changed files with 224 additions and 58 deletions.
7 changes: 7 additions & 0 deletions site/docs/visibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ specified in the [`package`](be/functions.html#package) statement of the
target's BUILD file. If there is no such `default_visibility` declaration, the
visibility is `//visibility:private`.

`config_setting` targets default to `//visibility:public`, regardless of how the
package's [`default_visibility`](be/functions.html#package.default_visibility)
is set. This is purely for legacy reasons. Best practice is to treat
`config_setting` targets as if they use the private default: any
`config_setting` intended for use by other packages should set its visibility
explicitly.

### Example

File `//frobber/bin/BUILD`:
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ java_library(
":buildinfo/build_info_key",
":config/build_configuration",
":config/build_options",
":config/config_conditions",
":config/config_matching_provider",
":config/core_options",
":config/execution_transition_factory",
Expand Down Expand Up @@ -1598,6 +1599,20 @@ java_library(
],
)

java_library(
name = "config/config_conditions",
srcs = ["config/ConfigConditions.java"],
deps = [
":config/config_matching_provider",
":configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//third_party:auto_value",
"//third_party:guava",
],
)

java_library(
name = "config/core_option_converters",
srcs = ["config/CoreOptionConverters.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.RuleContext.InvalidExecGroupException;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.RequiredFragmentsUtil;
import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget;
Expand Down Expand Up @@ -186,7 +186,7 @@ public final ConfiguredTarget createConfiguredTarget(
BuildConfiguration hostConfig,
ConfiguredTargetKey configuredTargetKey,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ConfigConditions configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
if (target instanceof Rule) {
Expand Down Expand Up @@ -292,7 +292,7 @@ private ConfiguredTarget createRule(
BuildConfiguration hostConfiguration,
ConfiguredTargetKey configuredTargetKey,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ConfigConditions configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
ConfigurationFragmentPolicy configurationFragmentPolicy =
Expand Down Expand Up @@ -323,7 +323,7 @@ private ConfiguredTarget createRule(
configuration,
ruleClassProvider.getUniversalFragments(),
configurationFragmentPolicy,
configConditions,
configConditions.asProviders(),
prerequisiteMap.values()))
.build();

Expand Down Expand Up @@ -500,7 +500,7 @@ public ConfiguredAspect createAspect(
ConfiguredAspectFactory aspectFactory,
Aspect aspect,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ConfigConditions configConditions,
@Nullable ResolvedToolchainContext toolchainContext,
BuildConfiguration aspectConfiguration,
BuildConfiguration hostConfiguration,
Expand Down Expand Up @@ -545,7 +545,7 @@ public ConfiguredAspect createAspect(
aspectConfiguration,
ruleClassProvider.getUniversalFragments(),
aspect.getDefinition().getConfigurationFragmentPolicy(),
configConditions,
configConditions.asProviders(),
prerequisiteMap.values()))
.build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum;
Expand Down Expand Up @@ -1764,7 +1765,7 @@ public static final class Builder implements RuleErrorConsumer {
private final PrerequisiteValidator prerequisiteValidator;
private final RuleErrorConsumer reporter;
private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap;
private ImmutableMap<Label, ConfigMatchingProvider> configConditions = ImmutableMap.of();
private ConfigConditions configConditions;
private String toolsRepository;
private StarlarkSemantics starlarkSemantics;
private Mutability mutability;
Expand Down Expand Up @@ -1813,11 +1814,15 @@ public RuleContext build() throws InvalidExecGroupException {
Preconditions.checkNotNull(visibility);
Preconditions.checkNotNull(constraintSemantics);
AttributeMap attributes =
ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions);
ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions.asProviders());
checkAttributesNonEmpty(attributes);
ListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap();
Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
for (ConfiguredTargetAndData condition : configConditions.asConfiguredTargets().values()) {
validateDirectPrerequisite(configSettingAttr, condition);
}
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
createFilesetEntryMap(target.getAssociatedRule(), configConditions);
createFilesetEntryMap(target.getAssociatedRule(), configConditions.asProviders());
if (rawExecProperties == null) {
if (!attributes.has(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)) {
rawExecProperties = ImmutableMap.of();
Expand All @@ -1831,7 +1836,7 @@ public RuleContext build() throws InvalidExecGroupException {
attributes,
targetMap,
filesetEntryMap,
configConditions,
configConditions.asProviders(),
universalFragments,
getRuleClassNameForLogging(),
actionOwnerSymbol,
Expand Down Expand Up @@ -1906,11 +1911,10 @@ public Builder setAspectAttributes(Map<String, Attribute> aspectAttributes) {
}

/**
* Sets the configuration conditions needed to determine which paths to follow for this
* rule's configurable attributes.
* Sets the configuration conditions needed to determine which paths to follow for this rule's
* configurable attributes.
*/
public Builder setConfigConditions(
ImmutableMap<Label, ConfigMatchingProvider> configConditions) {
public Builder setConfigConditions(ConfigConditions configConditions) {
this.configConditions = Preconditions.checkNotNull(configConditions);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Copyright 2021 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.devtools.build.lib.analysis.config;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;

/**
* Utility class for temporarily tracking {@code select()} keys' {@link ConfigMatchingProvider}s and
* {@link ConfiguredTarget}s.
*
* <p>This is a utility class because its only purpose is to maintain {@link ConfiguredTarget} long
* enough for {@link RuleContext.Builder} to do prerequisite validation on it (for example,
* visibility checks).
*
* <p>Once {@link RuleContext} is instantiated, it should only have access to {@link
* ConfigMatchingProvider}, on the principle that providers are the correct interfaces for storing
* and sharing target metadata. {@link ConfiguredTarget} isn't meant to persist that long.
*/
@AutoValue
public abstract class ConfigConditions {
public abstract ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets();

public abstract ImmutableMap<Label, ConfigMatchingProvider> asProviders();

public static ConfigConditions create(
ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets,
ImmutableMap<Label, ConfigMatchingProvider> asProviders) {
return new AutoValue_ConfigConditions(asConfiguredTargets, asProviders);
}

public static final ConfigConditions EMPTY =
ConfigConditions.create(ImmutableMap.of(), ImmutableMap.of());

/** Exception for when a {@code select()} has an invalid key (for example, wrong target type). */
public static class InvalidConditionException extends Exception {}

/**
* Returns a {@link ConfigMatchingProvider} from the given configured target if appropriate, else
* triggers a {@link InvalidConditionException}.
*
* <p>This is the canonical place to extract {@link ConfigMatchingProvider}s from configured
* targets. It's not as simple as {@link ConfiguredTarget#getProvider}.
*/
public static ConfigMatchingProvider fromConfiguredTarget(
ConfiguredTargetAndData selectKey, PlatformInfo targetPlatform)
throws InvalidConditionException {
ConfiguredTarget selectable = selectKey.getConfiguredTarget();
// The below handles config_setting (which natively provides ConfigMatchingProvider) and
// constraint_value (which needs a custom-built ConfigMatchingProvider).
ConfigMatchingProvider matchingProvider = selectable.getProvider(ConfigMatchingProvider.class);
if (matchingProvider != null) {
return matchingProvider;
}
ConstraintValueInfo constraintValueInfo = selectable.get(ConstraintValueInfo.PROVIDER);
if (constraintValueInfo != null && targetPlatform != null) {
// If platformInfo == null, that means the owning target doesn't invoke toolchain
// resolution, in which case depending on a constraint_value is nonsensical.
return constraintValueInfo.configMatchingProvider(targetPlatform);
}

// Not a valid provider for configuration conditions.
throw new InvalidConditionException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,11 @@ public RuleVisibility getVisibility() {
return visibility;
}

return pkg.getDefaultVisibility();
// TODO(bazel-team): give config_setting the same default visibility as everything else. The
// only reason this isn't trivial is depot cleanup.
return ruleClass.getName().equals("config_setting")
? ConstantRuleVisibility.PUBLIC
: pkg.getDefaultVisibility();
}

public boolean isVisibilitySpecified() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import com.google.devtools.build.lib.analysis.ToolchainCollection;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget;
Expand Down Expand Up @@ -403,7 +403,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

// Get the configuration targets that trigger this rule's configurable attributes.
ImmutableMap<Label, ConfigMatchingProvider> configConditions =
ConfigConditions configConditions =
ConfiguredTargetFunction.getConfigConditions(
env,
originalTargetAndAspectConfiguration,
Expand All @@ -423,7 +423,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
resolver,
originalTargetAndAspectConfiguration,
aspectPath,
configConditions,
configConditions.asProviders(),
unloadedToolchainContext == null
? null
: ToolchainCollection.builder()
Expand Down Expand Up @@ -640,7 +640,7 @@ private AspectValue createAspect(
ConfiguredAspectFactory aspectFactory,
ConfiguredTargetAndData associatedTarget,
BuildConfiguration aspectConfiguration,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ConfigConditions configConditions,
ResolvedToolchainContext toolchainContext,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> directDeps,
@Nullable NestedSetBuilder<Package> transitivePackagesForPackageRootResolution)
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_key",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
Expand Down
Loading

0 comments on commit cac82cf

Please sign in to comment.