Skip to content

Commit

Permalink
Roll forward config_setting visibility enforcement behind a flag.
Browse files Browse the repository at this point in the history
This was rolled back in 36d228b
 because of depot breakages.

This version adds incompatible flags to safely introduce enforcement.
See #12932 and
#12933 for flag settings.

*** Original change description ***

Roll back #12877.

***

Fixes #12669.

RELNOTES: enforce config_setting visibility. See #12932 for details.
PiperOrigin-RevId: 354930807
  • Loading branch information
gregestren authored and copybara-github committed Feb 1, 2021
1 parent 4fc4868 commit 79989f9
Show file tree
Hide file tree
Showing 19 changed files with 495 additions and 60 deletions.
19 changes: 19 additions & 0 deletions site/docs/visibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,25 @@ 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` visibility has historically not been enforced.
`--incompatible_enforce_config_setting_visibility` and
`--incompatible_config_setting_private_default_visibility` provide migration
logic for converging with other rules.

If `--incompatible_enforce_config_setting_visibility=false`, every
`config_setting` is unconditionally visible to all targets.

Else if `--incompatible_config_setting_private_default_visibility=false`, any
`config_setting` that doesn't explicitly set visibility is `//visibility:public`
(ignoring package [`default_visibility`](be/functions.html#package.default_visibility)).

Else if `--incompatible_config_setting_private_default_visibility=true`,
`config_setting` uses the same visibility logic as all other rules.

Best practice is to treat all `config_setting` targets like other rules:
explicitly set `visibility` on any `config_setting` used anywhere outside its
package.

### 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 @@ -1607,6 +1608,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 @@ -181,7 +181,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 @@ -287,7 +287,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 @@ -318,7 +318,7 @@ private ConfiguredTarget createRule(
configuration,
ruleClassProvider.getUniversalFragments(),
configurationFragmentPolicy,
configConditions,
configConditions.asProviders(),
prerequisiteMap.values()))
.build();

Expand Down Expand Up @@ -495,7 +495,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 @@ -540,7 +540,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 @@ -80,6 +81,7 @@
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.InputFile;
import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.RequiredProviders;
Expand Down Expand Up @@ -1766,7 +1768,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 @@ -1815,11 +1817,21 @@ 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();
// This conditionally checks visibility on config_setting rules based on
// --config_setting_visibility_policy. This should be removed as soon as it's deemed safe
// to unconditionally check visibility. See https://github.com/bazelbuild/bazel/issues/12669.
if (target.getPackage().getConfigSettingVisibilityPolicy()
!= ConfigSettingVisibilityPolicy.LEGACY_OFF) {
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 @@ -1833,7 +1845,7 @@ public RuleContext build() throws InvalidExecGroupException {
attributes,
targetMap,
filesetEntryMap,
configConditions,
configConditions.asProviders(),
universalFragments,
getRuleClassNameForLogging(),
actionOwnerSymbol,
Expand Down Expand Up @@ -1908,11 +1920,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();
}
}
34 changes: 34 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,24 @@ private NameConflictException(String message) {
private RuleVisibility defaultVisibility;
private boolean defaultVisibilitySet;

/**
* How to enforce config_setting visibility settings.
*
* <p>This is a temporary setting in service of https://github.com/bazelbuild/bazel/issues/12669.
* After enough depot cleanup, config_setting will have the same visibility enforcement as all
* other rules.
*/
public enum ConfigSettingVisibilityPolicy {
/** Don't enforce visibility for any config_setting. */
LEGACY_OFF,
/** Honor explicit visibility settings on config_setting, else use //visibility:public. */
DEFAULT_PUBLIC,
/** Enforce config_setting visibility exactly the same as all other rules. */
DEFAULT_STANDARD
}

private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;

/**
* Default package-level 'testonly' value for rules that do not specify it.
*/
Expand Down Expand Up @@ -436,6 +454,7 @@ private void finishInit(Builder builder) {
this.targets = ImmutableSortedKeyMap.copyOf(builder.targets);
this.defaultVisibility = builder.defaultVisibility;
this.defaultVisibilitySet = builder.defaultVisibilitySet;
this.configSettingVisibilityPolicy = builder.configSettingVisibilityPolicy;
if (builder.defaultCopts == null) {
this.defaultCopts = ImmutableList.of();
} else {
Expand Down Expand Up @@ -700,6 +719,14 @@ public RuleVisibility getDefaultVisibility() {
return defaultVisibility;
}

/**
* How to enforce visibility on <code>config_setting</code> See
* {@link ConfigSettingVisibilityPolicy} for details.
*/
public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
return configSettingVisibilityPolicy;
}

/**
* Returns the default testonly value.
*/
Expand Down Expand Up @@ -920,6 +947,7 @@ public boolean recordLoadedModules() {
// serialized representation is deterministic.
private final TreeMap<String, String> makeEnv = new TreeMap<>();
private RuleVisibility defaultVisibility = ConstantRuleVisibility.PRIVATE;
private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
private boolean defaultVisibilitySet;
private List<String> defaultCopts = null;
private final List<String> features = new ArrayList<>();
Expand Down Expand Up @@ -1163,6 +1191,12 @@ public Builder setDefaultVisibilitySet(boolean defaultVisibilitySet) {
return this;
}

/** Sets visibility enforcement policy for <code>config_setting</code>. */
public Builder setConfigSettingVisibilityPolicy(ConfigSettingVisibilityPolicy policy) {
this.configSettingVisibilityPolicy = policy;
return this;
}

/** Sets the default value of 'testonly'. Rule-level 'testonly' will override this. */
Builder setDefaultTestonly(boolean defaultTestonly) {
pkg.setDefaultTestOnly(defaultTestonly);
Expand Down
20 changes: 19 additions & 1 deletion src/main/java/com/google/devtools/build/lib/packages/Rule.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.events.EventHandler;
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.util.BinaryPredicate;
import java.util.Collection;
import java.util.HashSet;
Expand Down Expand Up @@ -842,10 +843,27 @@ public String toString() {
*/
@Override
public RuleVisibility getVisibility() {
// Temporary logic to relax config_setting's visibility enforcement while depot migrations set
// visibility settings properly (legacy code may have visibility settings that would break if
// enforced). See https://github.com/bazelbuild/bazel/issues/12669. Ultimately this entire
// conditional should be removed.
if (ruleClass.getName().equals("config_setting")) {
ConfigSettingVisibilityPolicy policy = getPackage().getConfigSettingVisibilityPolicy();
if (policy == ConfigSettingVisibilityPolicy.LEGACY_OFF) {
return ConstantRuleVisibility.PUBLIC; // Always public, no matter what.
} else if (visibility != null) {
return visibility; // Use explicitly set visibility
} else if (policy == ConfigSettingVisibilityPolicy.DEFAULT_PUBLIC) {
return ConstantRuleVisibility.PUBLIC; // Default: //visibility:public.
} else {
return pkg.getDefaultVisibility(); // Default: same as all other rules.
}
}

// All other rules.
if (visibility != null) {
return visibility;
}

return pkg.getDefaultVisibility();
}

Expand Down
Loading

0 comments on commit 79989f9

Please sign in to comment.