From 0d3c231f5a08861d28e987703e9196890e6164bf Mon Sep 17 00:00:00 2001 From: gregce Date: Mon, 1 Feb 2021 07:01:55 -0800 Subject: [PATCH] Roll forward config_setting visibility enforcement behind a flag. This was rolled back in https://github.com/bazelbuild/bazel/commit/36d228bd792f4332c7486c4e5f9c78e4b55f4b06 because of depot breakages. This version adds incompatible flags to safely introduce enforcement. See https://github.com/bazelbuild/bazel/issues/12932 and https://github.com/bazelbuild/bazel/issues/12933 for flag settings. *** Original change description *** Roll back https://github.com/bazelbuild/bazel/pull/12877. *** Fixes #12669. RELNOTES: enforce config_setting visibility. See https://github.com/bazelbuild/bazel/issues/12932 for details. PiperOrigin-RevId: 354930807 --- site/docs/visibility.md | 19 ++ .../google/devtools/build/lib/analysis/BUILD | 15 ++ .../lib/analysis/ConfiguredTargetFactory.java | 12 +- .../build/lib/analysis/RuleContext.java | 26 ++- .../lib/analysis/config/ConfigConditions.java | 81 +++++++ .../devtools/build/lib/packages/Package.java | 34 +++ .../build/lib/packages/PackageFactory.java | 3 +- .../devtools/build/lib/packages/Rule.java | 20 +- .../build/lib/pkgcache/PackageOptions.java | 33 +++ .../build/lib/skyframe/AspectFunction.java | 8 +- .../google/devtools/build/lib/skyframe/BUILD | 2 +- .../skyframe/ConfiguredTargetFunction.java | 58 ++--- .../build/lib/skyframe/PackageFunction.java | 5 + .../build/lib/skyframe/PrecomputedValue.java | 4 + .../build/lib/skyframe/SkyframeBuildView.java | 4 +- .../build/lib/skyframe/SkyframeExecutor.java | 12 + .../packages/AbstractPackageLoader.java | 3 + .../analysis/ConfigurableAttributesTest.java | 211 ++++++++++++++++++ .../devtools/build/lib/analysis/util/BUILD | 1 + .../analysis/util/BuildViewForTesting.java | 3 +- 20 files changed, 495 insertions(+), 59 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java diff --git a/site/docs/visibility.md b/site/docs/visibility.md index b1c19169d9731c..e1b7291be6fddb 100644 --- a/site/docs/visibility.md +++ b/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`: diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 30510adab44cbd..4ab785731c6f56 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/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"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index ffec432bba5688..128b9a9008cf16 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/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 prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts, ExecGroupCollection.Builder execGroupCollectionBuilder) throws InterruptedException, ActionConflictException, InvalidExecGroupException { @@ -293,7 +293,7 @@ private ConfiguredTarget createRule( BuildConfiguration hostConfiguration, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection 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 prerequisiteMap, - ImmutableMap 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(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 2a71d36fe42fb7..b7ab52665415cb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/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,6 +1603,7 @@ public static final class Builder implements RuleErrorConsumer { private final PrerequisiteValidator prerequisiteValidator; private final RuleErrorConsumer reporter; private OrderedSetMultimap prerequisiteMap; + private ConfigConditions configConditions; private ImmutableMap configConditions = ImmutableMap.of(); private NestedSet visibility; private ImmutableMap aspectAttributes; @@ -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 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 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 +1739,10 @@ public Builder setAspectAttributes(Map 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 configConditions) { + public Builder setConfigConditions(ConfigConditions configConditions) { this.configConditions = Preconditions.checkNotNull(configConditions); return this; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java new file mode 100644 index 00000000000000..40dd5204d2f1c1 --- /dev/null +++ b/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. + * + *

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). + * + *

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 asConfiguredTargets(); + + public abstract ImmutableMap asProviders(); + + public static ConfigConditions create( + ImmutableMap asConfiguredTargets, + ImmutableMap 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}. + * + *

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(); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 80a40ee2dc7411..3ea48b5509395d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/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. + * + *

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 config_setting 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 makeEnv = new TreeMap<>(); private RuleVisibility defaultVisibility = ConstantRuleVisibility.PRIVATE; + private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy; private boolean defaultVisibilitySet; private List defaultCopts = null; private final List features = new ArrayList<>(); @@ -1151,6 +1179,12 @@ Builder setDefaultVisibilitySet(boolean defaultVisibilitySet) { return this; } + /** Sets visibility enforcement policy for config_setting. */ + 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); diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index ecbf3b43720355..126441187c8438 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -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); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java index e5095e8e570594..0afc384080e038 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java @@ -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; @@ -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(); } diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java index a2339649bf16ab..df3450152aaa3a 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java @@ -30,6 +30,7 @@ import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; +import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParsingException; import java.util.List; @@ -121,6 +122,38 @@ public ParallelismConverter() throws OptionsParsingException { ) public RuleVisibility defaultVisibility; + + @Option( + name = "incompatible_enforce_config_setting_visibility", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If true, enforce config_setting visibility restrictions. If false, every " + + "config_setting is visible to every target. See " + + "https://github.com/bazelbuild/bazel/issues/12932." + ) + public boolean enforceConfigSettingVisibility; + + @Option( + name = "incompatible_config_setting_private_default_visibility", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If incompatible_enforce_config_setting_visibility=false, this is a noop. Else, if " + + "this flag is false, any config_setting without an explicit visibility attribute is " + + "//visibility:public. If this flag is true, config_setting follows the same visibility " + + "logic as all other rules. See https://github.com/bazelbuild/bazel/issues/12933." + ) + public boolean configSettingPrivateDefaultVisibility; + @Option( name = "legacy_globbing_threads", defaultValue = "100", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index a4cc98a73101b0..3bb3f1dbcb879b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -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; @@ -434,7 +434,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } // Get the configuration targets that trigger this rule's configurable attributes. - ImmutableMap configConditions = + ConfigConditions configConditions = ConfiguredTargetFunction.getConfigConditions( env, originalTargetAndAspectConfiguration, @@ -454,7 +454,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) resolver, originalTargetAndAspectConfiguration, aspectPath, - configConditions, + configConditions.asProviders(), unloadedToolchainContext == null ? null : ToolchainCollection.builder() @@ -671,7 +671,7 @@ private AspectValue createAspect( ConfiguredAspectFactory aspectFactory, ConfiguredTargetAndData associatedTarget, BuildConfiguration aspectConfiguration, - ImmutableMap configConditions, + ConfigConditions configConditions, ResolvedToolchainContext toolchainContext, OrderedSetMultimap directDeps, @Nullable NestedSetBuilder transitivePackagesForPackageRootResolution) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 701161058a750e..31fdbfa979bbac 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -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", @@ -1892,7 +1893,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/pkgcache", - "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index a1207eeca90f8c..bc922191ca95dc 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -49,13 +49,13 @@ 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.BuildOptionsView; +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.ConfigurationResolver; 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.config.transitions.PatchTransition; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; -import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition.TransitionException; import com.google.devtools.build.lib.causes.AnalysisFailedCause; @@ -96,7 +96,6 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -118,9 +117,6 @@ public final class ConfiguredTargetFunction implements SkyFunction { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - private static final ImmutableMap NO_CONFIG_CONDITIONS = - ImmutableMap.of(); - /** * Attempt to find a {@link ConfiguredValueCreationException} in a {@link ToolchainException}, or * its causes. @@ -292,7 +288,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc execGroupCollectionBuilder = result.execGroupCollectionBuilder; // Get the configuration targets that trigger this rule's configurable attributes. - ImmutableMap configConditions = + ConfigConditions configConditions = getConfigConditions( env, ctgValue, @@ -312,7 +308,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc // Note that this doesn't apply to AspectFunction, because aspects can't have configurable // attributes. if (!transitiveRootCauses.isEmpty() - && !Objects.equals(configConditions, NO_CONFIG_CONDITIONS)) { + && !Objects.equals(configConditions, ConfigConditions.EMPTY)) { NestedSet causes = transitiveRootCauses.build(); throw new ConfiguredTargetFunctionException( new ConfiguredValueCreationException( @@ -329,7 +325,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc resolver, ctgValue, ImmutableList.of(), - configConditions, + configConditions.asProviders(), unloadedToolchainContexts == null ? null : unloadedToolchainContexts.asToolchainContexts(), @@ -746,14 +742,13 @@ static OrderedSetMultimap computeDepend } /** - * Returns the set of {@link ConfigMatchingProvider}s that key the configurable attributes used by - * this rule. + * Returns the targets that key the configurable attributes used by this rule. * *

>If the configured targets supplying those providers aren't yet resolved by the dependency * resolver, returns null. */ @Nullable - static ImmutableMap getConfigConditions( + static ConfigConditions getConfigConditions( Environment env, TargetAndConfiguration ctgValue, @Nullable NestedSetBuilder transitivePackagesForPackageRootResolution, @@ -762,11 +757,11 @@ static ImmutableMap getConfigConditions( throws DependencyEvaluationException, InterruptedException { Target target = ctgValue.getTarget(); if (!(target instanceof Rule)) { - return NO_CONFIG_CONDITIONS; + return ConfigConditions.EMPTY; } RawAttributeMapper attrs = RawAttributeMapper.of(((Rule) target)); if (!attrs.has(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)) { - return NO_CONFIG_CONDITIONS; + return ConfigConditions.EMPTY; } // Collect the labels of the configured targets we need to resolve. @@ -775,7 +770,7 @@ static ImmutableMap getConfigConditions( .map(configLabel -> target.getLabel().resolveRepositoryRelative(configLabel)) .collect(Collectors.toList()); if (configLabels.isEmpty()) { - return NO_CONFIG_CONDITIONS; + return ConfigConditions.EMPTY; } else if (ctgValue.getConfiguration().trimConfigurationsRetroactively()) { String message = target.getLabel() @@ -829,33 +824,24 @@ static ImmutableMap getConfigConditions( throw e; } - Map configConditions = new LinkedHashMap<>(); + ImmutableMap.Builder asConfiguredTargets = + ImmutableMap.builder(); + ImmutableMap.Builder asConfigConditions = ImmutableMap.builder(); // Get the configured targets as ConfigMatchingProvider interfaces. for (Dependency entry : configConditionDeps) { SkyKey baseKey = entry.getConfiguredTargetKey(); - // The code above guarantees that value is non-null here. - ConfiguredTarget value = configValues.get(baseKey).getConfiguredTarget(); - // The below handles config_setting (which nativly provides ConfigMatchingProvider) and - // constraint_value (which needs a custom-built ConfigMatchingProvider). If we ever add - // support for more rules we should move resolution logic to ConfigMatchingProvider and - // simplify the logic here. - ConfigMatchingProvider matchingProvider = value.getProvider(ConfigMatchingProvider.class); - ConstraintValueInfo constraintValueInfo = value.get(ConstraintValueInfo.PROVIDER); - - if (matchingProvider != null) { - configConditions.put(entry.getLabel(), matchingProvider); - } else if (constraintValueInfo != null && platformInfo != null) { - // If platformInfo == null, that means the owning target doesn't invoke toolchain - // resolution, in which case depending on a constraint_value is non-sensical. - configConditions.put( - entry.getLabel(), constraintValueInfo.configMatchingProvider(platformInfo)); - } else { - // Not a valid provider for configuration conditions. + // The code above guarantees that selectKeyTarget is non-null here. + ConfiguredTargetAndData selectKeyTarget = configValues.get(baseKey); + asConfiguredTargets.put(entry.getLabel(), selectKeyTarget); + try { + asConfigConditions.put( + entry.getLabel(), ConfigConditions.fromConfiguredTarget(selectKeyTarget, platformInfo)); + } catch (ConfigConditions.InvalidConditionException e) { String message = String.format( "%s is not a valid select() condition for %s.\n", - entry.getLabel(), target.getLabel()) + selectKeyTarget.getTarget().getLabel(), target.getLabel()) + String.format( "To inspect the select(), run: bazel query --output=build %s.\n", target.getLabel()) @@ -867,7 +853,7 @@ static ImmutableMap getConfigConditions( } } - return ImmutableMap.copyOf(configConditions); + return ConfigConditions.create(asConfiguredTargets.build(), asConfigConditions.build()); } /** @@ -1026,7 +1012,7 @@ private ConfiguredTargetValue createConfiguredTarget( BuildConfiguration configuration, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap depValueMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts, ExecGroupCollection.Builder execGroupCollectionBuilder, @Nullable NestedSetBuilder transitivePackagesForPackageRootResolution) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index b6c4b811421ffb..31e6156fdc90a1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.packages.LegacyGlobber; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException; import com.google.devtools.build.lib.packages.RuleVisibility; @@ -428,6 +429,8 @@ public SkyValue compute(SkyKey key, Environment env) FileValue buildFileValue = getBuildFileValue(env, buildFileRootedPath); RuleVisibility defaultVisibility = PrecomputedValue.DEFAULT_VISIBILITY.get(env); + ConfigSettingVisibilityPolicy configSettingVisibilityPolicy = + PrecomputedValue.CONFIG_SETTING_VISIBILITY_POLICY.get(env); StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); IgnoredPackagePrefixesValue repositoryIgnoredPackagePrefixes = (IgnoredPackagePrefixesValue) @@ -468,6 +471,7 @@ public SkyValue compute(SkyKey key, Environment env) buildFileRootedPath, buildFileValue, defaultVisibility, + configSettingVisibilityPolicy, starlarkSemantics, preludeLabel, packageLookupValue.getRoot(), @@ -1164,6 +1168,7 @@ private LoadedPackageCacheEntry loadPackage( RootedPath buildFilePath, @Nullable FileValue buildFileValue, RuleVisibility defaultVisibility, + ConfigSettingVisibilityPolicy configSettingVisibilityPolicy, StarlarkSemantics starlarkSemantics, @Nullable Label preludeLabel, Root packageRoot, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java index e039158b5b2160..19810fb27de975 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java @@ -19,6 +19,7 @@ import com.google.common.base.Suppliers; import com.google.common.collect.Interner; import com.google.devtools.build.lib.concurrent.BlazeInterners; +import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -75,6 +76,9 @@ public static Injected injected(Precomputed precomputed, T value) { public static final Precomputed DEFAULT_VISIBILITY = new Precomputed<>("default_visibility"); + public static final Precomputed CONFIG_SETTING_VISIBILITY_POLICY = + new Precomputed<>("config_setting_visibility_policy"); + public static final Precomputed STARLARK_SEMANTICS = new Precomputed<>("starlark_semantics"); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index 1b398c77f416e9..19924ad1eebdf6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -56,7 +56,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff; -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.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentClassSet; import com.google.devtools.build.lib.bugreport.BugReport; @@ -928,7 +928,7 @@ ConfiguredTarget createConfiguredTarget( CachingAnalysisEnvironment analysisEnvironment, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts, ExecGroupCollection.Builder execGroupCollectionBuilder) throws InterruptedException, ActionConflictException, InvalidExecGroupException { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index ebd075d00ffab6..6bd088a1036fb9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -120,6 +120,7 @@ import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; +import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.RuleVisibility; @@ -1106,6 +1107,10 @@ private void setDefaultVisibility(RuleVisibility defaultVisibility) { PrecomputedValue.DEFAULT_VISIBILITY.set(injectable(), defaultVisibility); } + private void setConfigSettingVisibilityPolicty(ConfigSettingVisibilityPolicy policy) { + PrecomputedValue.CONFIG_SETTING_VISIBILITY_POLICY.set(injectable(), policy); + } + private void setStarlarkSemantics(StarlarkSemantics starlarkSemantics) { PrecomputedValue.STARLARK_SEMANTICS.set(injectable(), starlarkSemantics); } @@ -1344,6 +1349,13 @@ public void preparePackageLoading( setShowLoadingProgress(packageOptions.showLoadingProgress); setDefaultVisibility(packageOptions.defaultVisibility); + if (!packageOptions.enforceConfigSettingVisibility) { + setConfigSettingVisibilityPolicty(ConfigSettingVisibilityPolicy.LEGACY_OFF); + } else { + setConfigSettingVisibilityPolicty(packageOptions.configSettingPrivateDefaultVisibility + ? ConfigSettingVisibilityPolicy.DEFAULT_STANDARD + : ConfigSettingVisibilityPolicy.DEFAULT_PUBLIC); + } StarlarkSemantics starlarkSemantics = getEffectiveStarlarkSemantics(buildLanguageOptions); setStarlarkSemantics(starlarkSemantics); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index 333c9bfca0fc22..c0d994f6b79317 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -44,6 +44,7 @@ import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Package.Builder.DefaultPackageSettings; +import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension; import com.google.devtools.build.lib.packages.PackageLoadingListener; @@ -318,6 +319,8 @@ public void inject(SkyKey key, SkyValue value) { } PrecomputedValue.PATH_PACKAGE_LOCATOR.set(injectable, pkgLocator); PrecomputedValue.DEFAULT_VISIBILITY.set(injectable, ConstantRuleVisibility.PRIVATE); + PrecomputedValue.CONFIG_SETTING_VISIBILITY_POLICY + .set(injectable, ConfigSettingVisibilityPolicy.LEGACY_OFF); PrecomputedValue.STARLARK_SEMANTICS.set(injectable, starlarkSemantics); return new ImmutableDiff(ImmutableList.of(), valuesToInject); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java index eb1c8eecf4c21d..4cd7ad34d9424d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java @@ -1291,4 +1291,215 @@ public void multipleMatchErrorWhenAliasResolvesToSameSetting() throws Exception + " //a:foo\n" + " //a:alias_to_foo"); } + + @Test + public void defaultVisibilityConfigSetting_noVisibilityEnforcement() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=false"); + scratch.file("c/BUILD", "config_setting(name = 'foo', define_values = { 'foo': '1' })"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNotNull(); + assertNoEvents(); + } + + @Test + public void privateVisibilityConfigSetting_noVisibilityEnforcement() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=false"); + scratch.file( + "c/BUILD", + "config_setting(", + " name = 'foo',", + " define_values = { 'foo': '1' },", + " visibility = ['//visibility:private']", + ")"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNotNull(); + assertNoEvents(); + } + + @Test + public void publicVisibilityConfigSetting__noVisibilityEnforcement() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=false"); + scratch.file( + "c/BUILD", + "config_setting(", + " name = 'foo',", + " define_values = { 'foo': '1' },", + " visibility = ['//visibility:public']", + ")"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNotNull(); + assertNoEvents(); + } + @Test + public void defaultVisibilityConfigSetting_defaultIsPublic() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=true", + "--incompatible_config_setting_private_default_visibility=false"); + scratch.file("c/BUILD", "config_setting(name = 'foo', define_values = { 'foo': '1' })"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNotNull(); + assertNoEvents(); + } + + @Test + public void privateVisibilityConfigSetting_defaultIsPublic() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=true", + "--incompatible_config_setting_private_default_visibility=false"); + scratch.file( + "c/BUILD", + "config_setting(", + " name = 'foo',", + " define_values = { 'foo': '1' },", + " visibility = ['//visibility:private']", + ")"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNull(); + assertContainsEvent("'//c:foo' is not visible from target '//a:binary'"); + } + + @Test + public void publicVisibilityConfigSetting_defaultIsPublic() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=true", + "--incompatible_config_setting_private_default_visibility=false"); + scratch.file( + "c/BUILD", + "config_setting(", + " name = 'foo',", + " define_values = { 'foo': '1' },", + " visibility = ['//visibility:public']", + ")"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNotNull(); + assertNoEvents(); + } + @Test + public void defaultVisibilityConfigSetting_defaultIsPrivate() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=true", + "--incompatible_config_setting_private_default_visibility=true"); + scratch.file("c/BUILD", "config_setting(name = 'foo', define_values = { 'foo': '1' })"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNull(); + assertContainsEvent("'//c:foo' is not visible from target '//a:binary'"); + } + + @Test + public void privateVisibilityConfigSetting_defaultIsPrivate() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=true", + "--incompatible_config_setting_private_default_visibility=true"); + scratch.file( + "c/BUILD", + "config_setting(", + " name = 'foo',", + " define_values = { 'foo': '1' },", + " visibility = ['//visibility:private']", + ")"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNull(); + assertContainsEvent("'//c:foo' is not visible from target '//a:binary'"); + } + + @Test + public void publicVisibilityConfigSetting_defaultIsPrivate() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private", + "--incompatible_enforce_config_setting_visibility=true", + "--incompatible_config_setting_private_default_visibility=true"); + scratch.file( + "c/BUILD", + "config_setting(", + " name = 'foo',", + " define_values = { 'foo': '1' },", + " visibility = ['//visibility:public']", + ")"); + scratch.file( + "a/BUILD", + "rule_with_boolean_attr(", + " name = 'binary',", + " boolean_attr= select({", + " '//c:foo': 0,", + " '//conditions:default': 1", + " }))"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//a:binary")).isNotNull(); + assertNoEvents(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index 8041c0d7d80341..b684a9f0cb37f7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD @@ -49,6 +49,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", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index 1e63067225dda4..56b26489140898 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java @@ -50,6 +50,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildOptions; +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.ConfigurationResolver; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; @@ -566,7 +567,7 @@ public RuleContext getRuleContextForTesting( .setPrerequisites( ConfiguredTargetFactory.transformPrerequisiteMap( prerequisiteMap, target.getAssociatedRule())) - .setConfigConditions(ImmutableMap.of()) + .setConfigConditions(ConfigConditions.EMPTY) .setUniversalFragments(ruleClassProvider.getUniversalFragments()) .setToolchainContexts(resolvedToolchainContext.build()) .setExecGroupCollectionBuilder(execGroupCollectionBuilder)