Skip to content

Commit 0d3c231

Browse files
gregestrenkatre
gregestren
authored and
katre
committed
Roll forward config_setting visibility enforcement behind a flag.
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
1 parent f0d8cc7 commit 0d3c231

File tree

20 files changed

+495
-59
lines changed

20 files changed

+495
-59
lines changed

‎site/docs/visibility.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,25 @@ specified in the [`package`](be/functions.html#package) statement of the
5959
target's BUILD file. If there is no such `default_visibility` declaration, the
6060
visibility is `//visibility:private`.
6161

62+
`config_setting` visibility has historically not been enforced.
63+
`--incompatible_enforce_config_setting_visibility` and
64+
`--incompatible_config_setting_private_default_visibility` provide migration
65+
logic for converging with other rules.
66+
67+
If `--incompatible_enforce_config_setting_visibility=false`, every
68+
`config_setting` is unconditionally visible to all targets.
69+
70+
Else if `--incompatible_config_setting_private_default_visibility=false`, any
71+
`config_setting` that doesn't explicitly set visibility is `//visibility:public`
72+
(ignoring package [`default_visibility`](be/functions.html#package.default_visibility)).
73+
74+
Else if `--incompatible_config_setting_private_default_visibility=true`,
75+
`config_setting` uses the same visibility logic as all other rules.
76+
77+
Best practice is to treat all `config_setting` targets like other rules:
78+
explicitly set `visibility` on any `config_setting` used anywhere outside its
79+
package.
80+
6281
### Example
6382

6483
File `//frobber/bin/BUILD`:

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,7 @@ java_library(
292292
":buildinfo/build_info_key",
293293
":config/build_configuration",
294294
":config/build_options",
295+
":config/config_conditions",
295296
":config/config_matching_provider",
296297
":config/core_options",
297298
":config/execution_transition_factory",
@@ -1619,6 +1620,20 @@ java_library(
16191620
],
16201621
)
16211622

1623+
java_library(
1624+
name = "config/config_conditions",
1625+
srcs = ["config/ConfigConditions.java"],
1626+
deps = [
1627+
":config/config_matching_provider",
1628+
":configured_target",
1629+
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
1630+
"//src/main/java/com/google/devtools/build/lib/cmdline",
1631+
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
1632+
"//third_party:auto_value",
1633+
"//third_party:guava",
1634+
],
1635+
)
1636+
16221637
java_library(
16231638
name = "config/core_option_converters",
16241639
srcs = ["config/CoreOptionConverters.java"],

‎src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
2828
import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException;
2929
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
30-
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
30+
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
3131
import com.google.devtools.build.lib.analysis.config.Fragment;
3232
import com.google.devtools.build.lib.analysis.config.RequiredFragmentsUtil;
3333
import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget;
@@ -185,7 +185,7 @@ public final ConfiguredTarget createConfiguredTarget(
185185
BuildConfiguration hostConfig,
186186
ConfiguredTargetKey configuredTargetKey,
187187
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
188-
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
188+
ConfigConditions configConditions,
189189
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
190190
ExecGroupCollection.Builder execGroupCollectionBuilder)
191191
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
@@ -293,7 +293,7 @@ private ConfiguredTarget createRule(
293293
BuildConfiguration hostConfiguration,
294294
ConfiguredTargetKey configuredTargetKey,
295295
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
296-
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
296+
ConfigConditions configConditions,
297297
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts,
298298
ExecGroupCollection.Builder execGroupCollectionBuilder)
299299
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
@@ -323,7 +323,7 @@ private ConfiguredTarget createRule(
323323
configuration,
324324
ruleClassProvider.getUniversalFragments(),
325325
configurationFragmentPolicy,
326-
configConditions,
326+
configConditions.asProviders(),
327327
prerequisiteMap.values()))
328328
.build();
329329

@@ -498,7 +498,7 @@ public ConfiguredAspect createAspect(
498498
ConfiguredAspectFactory aspectFactory,
499499
Aspect aspect,
500500
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
501-
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
501+
ConfigConditions configConditions,
502502
@Nullable ResolvedToolchainContext toolchainContext,
503503
BuildConfiguration aspectConfiguration,
504504
BuildConfiguration hostConfiguration,
@@ -541,7 +541,7 @@ public ConfiguredAspect createAspect(
541541
aspectConfiguration,
542542
ruleClassProvider.getUniversalFragments(),
543543
aspect.getDefinition().getConfigurationFragmentPolicy(),
544-
configConditions,
544+
configConditions.asProviders(),
545545
prerequisiteMap.values()))
546546
.build();
547547

‎src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
4848
import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
4949
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
50+
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
5051
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
5152
import com.google.devtools.build.lib.analysis.config.CoreOptions;
5253
import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum;
@@ -79,6 +80,7 @@
7980
import com.google.devtools.build.lib.packages.Info;
8081
import com.google.devtools.build.lib.packages.InputFile;
8182
import com.google.devtools.build.lib.packages.OutputFile;
83+
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
8284
import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
8385
import com.google.devtools.build.lib.packages.RawAttributeMapper;
8486
import com.google.devtools.build.lib.packages.RequiredProviders;
@@ -1601,6 +1603,7 @@ public static final class Builder implements RuleErrorConsumer {
16011603
private final PrerequisiteValidator prerequisiteValidator;
16021604
private final RuleErrorConsumer reporter;
16031605
private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap;
1606+
private ConfigConditions configConditions;
16041607
private ImmutableMap<Label, ConfigMatchingProvider> configConditions = ImmutableMap.of();
16051608
private NestedSet<PackageGroupContents> visibility;
16061609
private ImmutableMap<String, Attribute> aspectAttributes;
@@ -1645,17 +1648,27 @@ public RuleContext build() throws InvalidExecGroupException {
16451648
Preconditions.checkNotNull(visibility);
16461649
Preconditions.checkNotNull(constraintSemantics);
16471650
AttributeMap attributes =
1648-
ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions);
1651+
ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions.asProviders());
16491652
checkAttributesNonEmpty(attributes);
16501653
ListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap();
1654+
// This conditionally checks visibility on config_setting rules based on
1655+
// --config_setting_visibility_policy. This should be removed as soon as it's deemed safe
1656+
// to unconditionally check visibility. See https://github.com/bazelbuild/bazel/issues/12669.
1657+
if (target.getPackage().getConfigSettingVisibilityPolicy()
1658+
!= ConfigSettingVisibilityPolicy.LEGACY_OFF) {
1659+
Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
1660+
for (ConfiguredTargetAndData condition : configConditions.asConfiguredTargets().values()) {
1661+
validateDirectPrerequisite(configSettingAttr, condition);
1662+
}
1663+
}
16511664
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
1652-
createFilesetEntryMap(target.getAssociatedRule(), configConditions);
1665+
createFilesetEntryMap(target.getAssociatedRule(), configConditions.asProviders());
16531666
return new RuleContext(
16541667
this,
16551668
attributes,
16561669
targetMap,
16571670
filesetEntryMap,
1658-
configConditions,
1671+
configConditions.asProviders(),
16591672
universalFragments,
16601673
getRuleClassNameForLogging(),
16611674
actionOwnerSymbol,
@@ -1726,11 +1739,10 @@ public Builder setAspectAttributes(Map<String, Attribute> aspectAttributes) {
17261739
}
17271740

17281741
/**
1729-
* Sets the configuration conditions needed to determine which paths to follow for this
1730-
* rule's configurable attributes.
1742+
* Sets the configuration conditions needed to determine which paths to follow for this rule's
1743+
* configurable attributes.
17311744
*/
1732-
public Builder setConfigConditions(
1733-
ImmutableMap<Label, ConfigMatchingProvider> configConditions) {
1745+
public Builder setConfigConditions(ConfigConditions configConditions) {
17341746
this.configConditions = Preconditions.checkNotNull(configConditions);
17351747
return this;
17361748
}

‎src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright 2021 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.analysis.config;
15+
16+
import com.google.auto.value.AutoValue;
17+
import com.google.common.collect.ImmutableMap;
18+
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
19+
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
20+
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
21+
import com.google.devtools.build.lib.cmdline.Label;
22+
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
23+
24+
/**
25+
* Utility class for temporarily tracking {@code select()} keys' {@link ConfigMatchingProvider}s and
26+
* {@link ConfiguredTarget}s.
27+
*
28+
* <p>This is a utility class because its only purpose is to maintain {@link ConfiguredTarget} long
29+
* enough for {@link RuleContext.Builder} to do prerequisite validation on it (for example,
30+
* visibility checks).
31+
*
32+
* <p>Once {@link RuleContext} is instantiated, it should only have access to {@link
33+
* ConfigMatchingProvider}, on the principle that providers are the correct interfaces for storing
34+
* and sharing target metadata. {@link ConfiguredTarget} isn't meant to persist that long.
35+
*/
36+
@AutoValue
37+
public abstract class ConfigConditions {
38+
public abstract ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets();
39+
40+
public abstract ImmutableMap<Label, ConfigMatchingProvider> asProviders();
41+
42+
public static ConfigConditions create(
43+
ImmutableMap<Label, ConfiguredTargetAndData> asConfiguredTargets,
44+
ImmutableMap<Label, ConfigMatchingProvider> asProviders) {
45+
return new AutoValue_ConfigConditions(asConfiguredTargets, asProviders);
46+
}
47+
48+
public static final ConfigConditions EMPTY =
49+
ConfigConditions.create(ImmutableMap.of(), ImmutableMap.of());
50+
51+
/** Exception for when a {@code select()} has an invalid key (for example, wrong target type). */
52+
public static class InvalidConditionException extends Exception {}
53+
54+
/**
55+
* Returns a {@link ConfigMatchingProvider} from the given configured target if appropriate, else
56+
* triggers a {@link InvalidConditionException}.
57+
*
58+
* <p>This is the canonical place to extract {@link ConfigMatchingProvider}s from configured
59+
* targets. It's not as simple as {@link ConfiguredTarget#getProvider}.
60+
*/
61+
public static ConfigMatchingProvider fromConfiguredTarget(
62+
ConfiguredTargetAndData selectKey, PlatformInfo targetPlatform)
63+
throws InvalidConditionException {
64+
ConfiguredTarget selectable = selectKey.getConfiguredTarget();
65+
// The below handles config_setting (which natively provides ConfigMatchingProvider) and
66+
// constraint_value (which needs a custom-built ConfigMatchingProvider).
67+
ConfigMatchingProvider matchingProvider = selectable.getProvider(ConfigMatchingProvider.class);
68+
if (matchingProvider != null) {
69+
return matchingProvider;
70+
}
71+
ConstraintValueInfo constraintValueInfo = selectable.get(ConstraintValueInfo.PROVIDER);
72+
if (constraintValueInfo != null && targetPlatform != null) {
73+
// If platformInfo == null, that means the owning target doesn't invoke toolchain
74+
// resolution, in which case depending on a constraint_value is nonsensical.
75+
return constraintValueInfo.configMatchingProvider(targetPlatform);
76+
}
77+
78+
// Not a valid provider for configuration conditions.
79+
throw new InvalidConditionException();
80+
}
81+
}

‎src/main/java/com/google/devtools/build/lib/packages/Package.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,24 @@ private NameConflictException(String message) {
143143
private RuleVisibility defaultVisibility;
144144
private boolean defaultVisibilitySet;
145145

146+
/**
147+
* How to enforce config_setting visibility settings.
148+
*
149+
* <p>This is a temporary setting in service of https://github.com/bazelbuild/bazel/issues/12669.
150+
* After enough depot cleanup, config_setting will have the same visibility enforcement as all
151+
* other rules.
152+
*/
153+
public enum ConfigSettingVisibilityPolicy {
154+
/** Don't enforce visibility for any config_setting. */
155+
LEGACY_OFF,
156+
/** Honor explicit visibility settings on config_setting, else use //visibility:public. */
157+
DEFAULT_PUBLIC,
158+
/** Enforce config_setting visibility exactly the same as all other rules. */
159+
DEFAULT_STANDARD
160+
}
161+
162+
private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
163+
146164
/**
147165
* Default package-level 'testonly' value for rules that do not specify it.
148166
*/
@@ -436,6 +454,7 @@ private void finishInit(Builder builder) {
436454
this.targets = ImmutableSortedKeyMap.copyOf(builder.targets);
437455
this.defaultVisibility = builder.defaultVisibility;
438456
this.defaultVisibilitySet = builder.defaultVisibilitySet;
457+
this.configSettingVisibilityPolicy = builder.configSettingVisibilityPolicy;
439458
if (builder.defaultCopts == null) {
440459
this.defaultCopts = ImmutableList.of();
441460
} else {
@@ -700,6 +719,14 @@ public RuleVisibility getDefaultVisibility() {
700719
return defaultVisibility;
701720
}
702721

722+
/**
723+
* How to enforce visibility on <code>config_setting</code> See
724+
* {@link ConfigSettingVisibilityPolicy} for details.
725+
*/
726+
public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
727+
return configSettingVisibilityPolicy;
728+
}
729+
703730
/**
704731
* Returns the default testonly value.
705732
*/
@@ -920,6 +947,7 @@ public boolean recordLoadedModules() {
920947
// serialized representation is deterministic.
921948
private final TreeMap<String, String> makeEnv = new TreeMap<>();
922949
private RuleVisibility defaultVisibility = ConstantRuleVisibility.PRIVATE;
950+
private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
923951
private boolean defaultVisibilitySet;
924952
private List<String> defaultCopts = null;
925953
private final List<String> features = new ArrayList<>();
@@ -1151,6 +1179,12 @@ Builder setDefaultVisibilitySet(boolean defaultVisibilitySet) {
11511179
return this;
11521180
}
11531181

1182+
/** Sets visibility enforcement policy for <code>config_setting</code>. */
1183+
public Builder setConfigSettingVisibilityPolicy(ConfigSettingVisibilityPolicy policy) {
1184+
this.configSettingVisibilityPolicy = policy;
1185+
return this;
1186+
}
1187+
11541188
/** Sets the default value of 'testonly'. Rule-level 'testonly' will override this. */
11551189
Builder setDefaultTestonly(boolean defaultTestonly) {
11561190
pkg.setDefaultTestOnly(defaultTestonly);

‎src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -842,7 +842,8 @@ Package.Builder evaluateBuildFile(
842842
// and can be derived from Package.loads (if available) on demand.
843843
.setStarlarkFileDependencies(transitiveClosureOfLabels(loadedModules))
844844
.setThirdPartyLicenceExistencePolicy(
845-
ruleClassProvider.getThirdPartyLicenseExistencePolicy());
845+
ruleClassProvider.getThirdPartyLicenseExistencePolicy())
846+
.setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy);
846847
if (packageSettings.recordLoadedModules()) {
847848
pkgBuilder.setLoads(loadedModules);
848849
}

‎src/main/java/com/google/devtools/build/lib/packages/Rule.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import com.google.devtools.build.lib.events.Event;
3333
import com.google.devtools.build.lib.events.EventHandler;
3434
import com.google.devtools.build.lib.packages.License.DistributionType;
35+
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
3536
import com.google.devtools.build.lib.util.BinaryPredicate;
3637
import java.util.Collection;
3738
import java.util.LinkedHashSet;
@@ -728,10 +729,27 @@ public String toString() {
728729
*/
729730
@Override
730731
public RuleVisibility getVisibility() {
732+
// Temporary logic to relax config_setting's visibility enforcement while depot migrations set
733+
// visibility settings properly (legacy code may have visibility settings that would break if
734+
// enforced). See https://github.com/bazelbuild/bazel/issues/12669. Ultimately this entire
735+
// conditional should be removed.
736+
if (ruleClass.getName().equals("config_setting")) {
737+
ConfigSettingVisibilityPolicy policy = getPackage().getConfigSettingVisibilityPolicy();
738+
if (policy == ConfigSettingVisibilityPolicy.LEGACY_OFF) {
739+
return ConstantRuleVisibility.PUBLIC; // Always public, no matter what.
740+
} else if (visibility != null) {
741+
return visibility; // Use explicitly set visibility
742+
} else if (policy == ConfigSettingVisibilityPolicy.DEFAULT_PUBLIC) {
743+
return ConstantRuleVisibility.PUBLIC; // Default: //visibility:public.
744+
} else {
745+
return pkg.getDefaultVisibility(); // Default: same as all other rules.
746+
}
747+
}
748+
749+
// All other rules.
731750
if (visibility != null) {
732751
return visibility;
733752
}
734-
735753
return pkg.getDefaultVisibility();
736754
}
737755

0 commit comments

Comments
 (0)