git cherry-pick for 4.2.0: flag-gated config_setting visibility

site/docs/visibility.md

@@ -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`:

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

@@ -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",
@@ -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"],

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

@@ -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;
@@ -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 {
@@ -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 {
@@ -323,7 +323,7 @@ private ConfiguredTarget createRule(
                     configuration,
                     ruleClassProvider.getUniversalFragments(),
                     configurationFragmentPolicy,
-                    configConditions,
+                    configConditions.asProviders(),
                     prerequisiteMap.values()))
             .build();
 
@@ -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,
@@ -541,7 +541,7 @@ public ConfiguredAspect createAspect(
                     aspectConfiguration,
                     ruleClassProvider.getUniversalFragments(),
                     aspect.getDefinition().getConfigurationFragmentPolicy(),
-                    configConditions,
+                    configConditions.asProviders(),
                     prerequisiteMap.values()))
             .build();
 

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

@@ -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;
@@ -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;
@@ -1601,7 +1603,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 NestedSet<PackageGroupContents> visibility;
     private ImmutableMap<String, Attribute> aspectAttributes;
     private ImmutableList<Aspect> aspects;
@@ -1645,17 +1647,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,
@@ -1726,11 +1738,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;
     }

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

@@ -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();
+  }
+}

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

@@ -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.
    */
@@ -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 {
@@ -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.
    */
@@ -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<>();
@@ -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);

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

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Globber.BadGlobException;
+import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
 import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings;
 import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
@@ -504,7 +505,8 @@ public Package.Builder createPackageFromAst(
       ImmutableMap<String, Module> loadedModules,
       RuleVisibility defaultVisibility,
       StarlarkSemantics starlarkSemantics,
-      Globber globber)
+      Globber globber,
+      ConfigSettingVisibilityPolicy configSettingVisibilityPolicy)
       throws InterruptedException {
     try {
       // At this point the package is guaranteed to exist,
@@ -519,7 +521,8 @@ public Package.Builder createPackageFromAst(
           starlarkSemantics,
           preludeModule,
           loadedModules,
-          repositoryMapping);
+          repositoryMapping,
+          configSettingVisibilityPolicy);
     } catch (InterruptedException e) {
       globber.onInterrupt();
       throw e;
@@ -823,7 +826,8 @@ Package.Builder evaluateBuildFile(
       StarlarkSemantics semantics,
       @Nullable Module preludeModule,
       ImmutableMap<String, Module> loadedModules,
-      ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
+      ImmutableMap<RepositoryName, RepositoryName> repositoryMapping,
+      ConfigSettingVisibilityPolicy configSettingVisibilityPolicy)
       throws InterruptedException {
     Package.Builder pkgBuilder =
         new Package.Builder(
@@ -842,7 +846,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);
     }