From 520f30a7ca5813c0fe381df49c5b775944cf4c85 Mon Sep 17 00:00:00 2001 From: gregce Date: Thu, 21 Jan 2021 16:40:32 -0800 Subject: [PATCH 1/5] Enforce visibility on select() keys. Example: my_rule( name = "buildme", deps = select({ "//other/package:some_config": [":mydeps"] })) Today, //other/package:some_config is exempt from visibility checking, even though it's technically a target dep of "buildme". While this dep is "special" vs. other deps in various ways, there's no obvious reason why it needs to be special in this way. It adds an unclear corner case exception to visibility's API. Implementation: --------------- select() keys are not "normal" dependencies and don't generally follow the same code path. Hence them not being automatically visibility checked like others. In particular, normal dependencies are found in ConfiguredTargetFunction and validity-checked in RuleContext.Builder. select() keys' only purpose is to figure out which other normal dependencies should exist. So there's generally no need to pass them to RuleContext.Builder. Instead, Blaze passes their ConfigMatchingProviders, which remain useful for analysis phase attribute lookups. RuleContext.Builder needs a ConfiguredTargetAndData to do validity-checking. This patch passes that information along for select() keys too. We could alternatively refactor the validity checking logic. But that's an even more invasive change. Or do ad hoc validity checking directly in ConfiguredTargetFunction. But that's duplicating logic we really don't want to keep consistent. Backward compatibility: ----------------------- This has a very good chance of breaking builds, since all targets default to private visibility and it's unlikely existing config_settings have changed that. Pushing this forward requires an incompatible change flag. PiperOrigin-RevId: 353131136 Change-Id: Ia5056745d135732bd35ba0ab099800d982f33f74 --- site/docs/visibility.md | 4 + .../google/devtools/build/lib/analysis/BUILD | 15 ++++ .../lib/analysis/ConfiguredTargetFactory.java | 12 +-- .../build/lib/analysis/RuleContext.java | 20 +++-- .../lib/analysis/config/ConfigConditions.java | 81 +++++++++++++++++++ .../devtools/build/lib/packages/Rule.java | 6 +- .../build/lib/skyframe/AspectFunction.java | 8 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ConfiguredTargetFunction.java | 57 +++++-------- .../build/lib/skyframe/SkyframeBuildView.java | 4 +- .../analysis/ConfigurableAttributesTest.java | 67 +++++++++++++++ .../devtools/build/lib/analysis/util/BUILD | 1 + .../analysis/util/BuildViewForTesting.java | 3 +- 13 files changed, 222 insertions(+), 57 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 e8ff2ff7960d50..22d44692c1f279 100644 --- a/site/docs/visibility.md +++ b/site/docs/visibility.md @@ -62,6 +62,10 @@ 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` targets default to `//visibility:public`. This is purely for +legacy reasons. Best practice is to treat `config_setting` targets as if they +also use the private default. + ### 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 cd7e478c54c92d..238ed97f1b9044 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", @@ -1596,6 +1597,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 aea0a0a63bb712..2892050bc118df 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.RuleContext.InvalidExecGroupException; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; -import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; +import com.google.devtools.build.lib.analysis.config.ConfigConditions; import com.google.devtools.build.lib.analysis.config.Fragment; import com.google.devtools.build.lib.analysis.config.RequiredFragmentsUtil; import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget; @@ -186,7 +186,7 @@ public final ConfiguredTarget createConfiguredTarget( BuildConfiguration hostConfig, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts) throws InterruptedException, ActionConflictException, InvalidExecGroupException { if (target instanceof Rule) { @@ -292,7 +292,7 @@ private ConfiguredTarget createRule( BuildConfiguration hostConfiguration, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts) throws InterruptedException, ActionConflictException, InvalidExecGroupException { ConfigurationFragmentPolicy configurationFragmentPolicy = @@ -323,7 +323,7 @@ private ConfiguredTarget createRule( configuration, ruleClassProvider.getUniversalFragments(), configurationFragmentPolicy, - configConditions, + configConditions.asProviders(), prerequisiteMap.values())) .build(); @@ -500,7 +500,7 @@ public ConfiguredAspect createAspect( ConfiguredAspectFactory aspectFactory, Aspect aspect, OrderedSetMultimap prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ResolvedToolchainContext toolchainContext, BuildConfiguration aspectConfiguration, BuildConfiguration hostConfiguration, @@ -545,7 +545,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 27913ecfa63650..792bef339d4b13 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 @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.analysis.config.ConfigConditions; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum; @@ -1764,7 +1765,7 @@ public static final class Builder implements RuleErrorConsumer { private final PrerequisiteValidator prerequisiteValidator; private final RuleErrorConsumer reporter; private OrderedSetMultimap prerequisiteMap; - private ImmutableMap configConditions = ImmutableMap.of(); + private ConfigConditions configConditions; private String toolsRepository; private StarlarkSemantics starlarkSemantics; private Mutability mutability; @@ -1813,11 +1814,15 @@ 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(); + 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()); if (rawExecProperties == null) { if (!attributes.has(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)) { rawExecProperties = ImmutableMap.of(); @@ -1831,7 +1836,7 @@ public RuleContext build() throws InvalidExecGroupException { attributes, targetMap, filesetEntryMap, - configConditions, + configConditions.asProviders(), universalFragments, getRuleClassNameForLogging(), actionOwnerSymbol, @@ -1906,11 +1911,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..f88441bb23bace --- /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 (like visibility + * checking). + * + *

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 (like 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); + ConstraintValueInfo constraintValueInfo = selectable.get(ConstraintValueInfo.PROVIDER); + if (matchingProvider != null) { + return matchingProvider; + } + 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/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java index 0c6a2c3d274188..90d1456d523126 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 @@ -732,7 +732,11 @@ public RuleVisibility getVisibility() { return visibility; } - return pkg.getDefaultVisibility(); + // TODO(bazel-team): give config_setting the same default visibility as everything else. The + // only reason this isn't trivial is depot cleanup. + return ruleClass.getName().equals("config_setting") + ? ConstantRuleVisibility.PUBLIC + : pkg.getDefaultVisibility(); } public boolean isVisibilitySpecified() { 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 ca082924cec535..385c111eed5343 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; @@ -403,7 +403,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, @@ -423,7 +423,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) resolver, originalTargetAndAspectConfiguration, aspectPath, - configConditions, + configConditions.asProviders(), unloadedToolchainContext == null ? null : ToolchainCollection.builder() @@ -640,7 +640,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 888c34ea20c4a5..8b770880e74e6b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -234,6 +234,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/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 84e41d14b50cb5..39beef507dfeea 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 @@ -48,13 +48,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; @@ -116,9 +116,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. @@ -287,7 +284,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc } // Get the configuration targets that trigger this rule's configurable attributes. - ImmutableMap configConditions = + ConfigConditions configConditions = getConfigConditions( env, ctgValue, @@ -307,7 +304,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( @@ -324,7 +321,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc resolver, ctgValue, ImmutableList.of(), - configConditions, + configConditions.asProviders(), unloadedToolchainContexts == null ? null : unloadedToolchainContexts.asToolchainContexts(), @@ -712,14 +709,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, @@ -728,11 +724,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. @@ -741,7 +737,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() @@ -795,33 +791,23 @@ static ImmutableMap getConfigConditions( throw e; } - Map configConditions = new LinkedHashMap<>(); + Map asConfiguredTargets = new LinkedHashMap<>(); + Map asConfigConditions = new LinkedHashMap<>(); // 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()) @@ -833,7 +819,8 @@ static ImmutableMap getConfigConditions( } } - return ImmutableMap.copyOf(configConditions); + return ConfigConditions.create( + ImmutableMap.copyOf(asConfiguredTargets), ImmutableMap.copyOf(asConfigConditions)); } /** @@ -992,7 +979,7 @@ private ConfiguredTargetValue createConfiguredTarget( BuildConfiguration configuration, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap depValueMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts, @Nullable NestedSetBuilder transitivePackagesForPackageRootResolution) throws ConfiguredTargetFunctionException, InterruptedException { 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 84dc59ea8bca89..47348edf86e8a0 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 @@ -55,7 +55,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; @@ -931,7 +931,7 @@ ConfiguredTarget createConfiguredTarget( CachingAnalysisEnvironment analysisEnvironment, ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, - ImmutableMap configConditions, + ConfigConditions configConditions, @Nullable ToolchainCollection toolchainContexts) throws InterruptedException, ActionConflictException, InvalidExecGroupException { Preconditions.checkState( 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 b45e242bfc6e26..40fa2418041ba5 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 @@ -1279,4 +1279,71 @@ public void multipleMatchErrorWhenAliasResolvesToSameSetting() throws Exception + " //a:foo\n" + " //a:alias_to_foo"); } + + @Test + public void defaultVisibilityConfigSetting() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private"); + 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() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private"); + 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() throws Exception { + // Production builds default to private visibility, but BuildViewTestCase defaults to public. + setPackageOptions("--default_visibility=private"); + 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 53ed528afeeb6f..2ec092345d183b 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/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", 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 a71e381ad6b318..8d10729b589a24 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 @@ -49,6 +49,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; @@ -608,7 +609,7 @@ public RuleContext getRuleContextForTesting( .setPrerequisites( ConfiguredTargetFactory.transformPrerequisiteMap( prerequisiteMap, target.getAssociatedRule())) - .setConfigConditions(ImmutableMap.of()) + .setConfigConditions(ConfigConditions.EMPTY) .setUniversalFragments(ruleClassProvider.getUniversalFragments()) .setToolchainContexts(resolvedToolchainContext.build()) .setConstraintSemantics(ruleClassProvider.getConstraintSemantics()) From 337d9834b6f7c6c0e7d74f9eea0b127096ab8260 Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Wed, 27 Jan 2021 14:58:58 -0500 Subject: [PATCH 2/5] Review feedback. --- .../build/lib/analysis/config/ConfigConditions.java | 6 +++--- .../build/lib/skyframe/ConfiguredTargetFunction.java | 8 ++++---- .../build/lib/analysis/ConfigurableAttributesTest.java | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) 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 index f88441bb23bace..c524d5f790478b 100644 --- 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 @@ -26,8 +26,8 @@ * {@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 (like visibility - * checking). + * 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 @@ -65,10 +65,10 @@ public static ConfigMatchingProvider fromConfiguredTarget( // The below handles config_setting (which natively provides ConfigMatchingProvider) and // constraint_value (which needs a custom-built ConfigMatchingProvider). ConfigMatchingProvider matchingProvider = selectable.getProvider(ConfigMatchingProvider.class); - ConstraintValueInfo constraintValueInfo = selectable.get(ConstraintValueInfo.PROVIDER); 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. 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 39beef507dfeea..f34d0cf977841f 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 @@ -791,8 +791,9 @@ static ConfigConditions getConfigConditions( throw e; } - Map asConfiguredTargets = new LinkedHashMap<>(); - Map asConfigConditions = new LinkedHashMap<>(); + ImmutableMap.Builder asConfiguredTargets = + ImmutableMap.builder(); + ImmutableMap.Builder asConfigConditions = ImmutableMap.builder(); // Get the configured targets as ConfigMatchingProvider interfaces. for (Dependency entry : configConditionDeps) { @@ -819,8 +820,7 @@ static ConfigConditions getConfigConditions( } } - return ConfigConditions.create( - ImmutableMap.copyOf(asConfiguredTargets), ImmutableMap.copyOf(asConfigConditions)); + return ConfigConditions.create(asConfiguredTargets.build(), asConfigConditions.build()); } /** 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 40fa2418041ba5..11ff1cc96ce474 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 @@ -1285,7 +1285,7 @@ public void defaultVisibilityConfigSetting() throws Exception { // Production builds default to private visibility, but BuildViewTestCase defaults to public. setPackageOptions("--default_visibility=private"); scratch.file( - "c/BUILD", "config_setting(", " name = 'foo',", " define_values = { 'foo': '1' })"); + "c/BUILD", "config_setting(name = 'foo', define_values = { 'foo': '1' })"); scratch.file( "a/BUILD", "rule_with_boolean_attr(", From 39bbfbaba413d99a900b7c94b719e8b8c5b8efc4 Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Wed, 27 Jan 2021 16:51:43 -0500 Subject: [PATCH 3/5] Update doc feedback. --- site/docs/visibility.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/site/docs/visibility.md b/site/docs/visibility.md index 22d44692c1f279..834a2727688bfc 100644 --- a/site/docs/visibility.md +++ b/site/docs/visibility.md @@ -62,9 +62,12 @@ 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` targets default to `//visibility:public`. This is purely for +`config_setting` targets default to `//visibility:public, regardless of how the +package's [`default_visibility`](be/functions.html#package.default_visibility) +is set. This is purely for legacy reasons. Best practice is to treat `config_setting` targets as if they -also use the private default. +use the private default: any `config_setting` intended for use by other packages +should set its visibility explicitly. ### Example From 290c26c897cf1cdc09409adf1bc77a1bb435c8c1 Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Wed, 27 Jan 2021 16:52:49 -0500 Subject: [PATCH 4/5] Minor formatting fix. --- site/docs/visibility.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/site/docs/visibility.md b/site/docs/visibility.md index 834a2727688bfc..3831c0fbc8e9a9 100644 --- a/site/docs/visibility.md +++ b/site/docs/visibility.md @@ -64,10 +64,10 @@ visibility is `//visibility:private`. `config_setting` targets default to `//visibility:public, regardless of how the package's [`default_visibility`](be/functions.html#package.default_visibility) -is set. This is purely for -legacy reasons. Best practice is to treat `config_setting` targets as if they -use the private default: any `config_setting` intended for use by other packages -should set its visibility explicitly. +is set. This is purely for legacy reasons. Best practice is to treat +`config_setting` targets as if they use the private default: any +`config_setting` intended for use by other packages should set its visibility +explicitly. ### Example From 3b9f2e270534d832c704962994dc3d91d550484f Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Wed, 27 Jan 2021 16:54:33 -0500 Subject: [PATCH 5/5] Doc update. --- .../devtools/build/lib/analysis/config/ConfigConditions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index c524d5f790478b..40dd5204d2f1c1 100644 --- 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 @@ -48,7 +48,7 @@ public static ConfigConditions create( public static final ConfigConditions EMPTY = ConfigConditions.create(ImmutableMap.of(), ImmutableMap.of()); - /** Exception for when a {@code select()} has an invalid key (like wrong target type). */ + /** Exception for when a {@code select()} has an invalid key (for example, wrong target type). */ public static class InvalidConditionException extends Exception {} /**