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 bazelbuild@36d228b
 because of depot breakages.

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

*** Original change description ***

Roll back bazelbuild#12877.

***

Fixes bazelbuild#12669.

RELNOTES: enforce config_setting visibility. See bazelbuild#12932 for details.
PiperOrigin-RevId: 354930807
  • Loading branch information
gregestren committed Jul 16, 2021
1 parent 9d38cb0 commit 51b3c78
Show file tree
Hide file tree
Showing 20 changed files with 495 additions and 59 deletions.
19 changes: 19 additions & 0 deletions site/docs/visibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,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 @@ -1619,6 +1620,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.ExecGroupCollection.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 @@ -185,7 +185,7 @@ public final ConfiguredTarget createConfiguredTarget(
BuildConfiguration hostConfig,
ConfiguredTargetKey configuredTargetKey,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ConfigConditions configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
ExecGroupCollection.Builder execGroupCollectionBuilder)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
Expand Down Expand Up @@ -293,7 +293,7 @@ private ConfiguredTarget createRule(
BuildConfiguration hostConfiguration,
ConfiguredTargetKey configuredTargetKey,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ConfigConditions configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
ExecGroupCollection.Builder execGroupCollectionBuilder)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
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 @@ -498,7 +498,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 @@ -541,7 +541,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 @@ -47,6 +47,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 @@ -79,6 +80,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 @@ -1601,6 +1603,7 @@ public static final class Builder implements RuleErrorConsumer {
private final PrerequisiteValidator prerequisiteValidator;
private final RuleErrorConsumer reporter;
private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap;
private ConfigConditions configConditions;
private ImmutableMap<Label, ConfigMatchingProvider> configConditions = ImmutableMap.of();
private NestedSet<PackageGroupContents> visibility;
private ImmutableMap<String, Attribute> aspectAttributes;
Expand Down Expand Up @@ -1645,17 +1648,27 @@ 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());
return new RuleContext(
this,
attributes,
targetMap,
filesetEntryMap,
configConditions,
configConditions.asProviders(),
universalFragments,
getRuleClassNameForLogging(),
actionOwnerSymbol,
Expand Down Expand Up @@ -1726,11 +1739,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 @@ -1151,6 +1179,12 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,8 @@ Package.Builder evaluateBuildFile(
// and can be derived from Package.loads (if available) on demand.
.setStarlarkFileDependencies(transitiveClosureOfLabels(loadedModules))
.setThirdPartyLicenceExistencePolicy(
ruleClassProvider.getThirdPartyLicenseExistencePolicy());
ruleClassProvider.getThirdPartyLicenseExistencePolicy())
.setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy);
if (packageSettings.recordLoadedModules()) {
pkgBuilder.setLoads(loadedModules);
}
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 @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
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.LinkedHashSet;
Expand Down Expand Up @@ -728,10 +729,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 51b3c78

Please sign in to comment.