Skip to content

Commit

Permalink
Roll back #12877.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Too many existing config_settings already set visibility. That was never enforced before. But now they'll break if any deps don't honor that visibility. This needs better depot cleanup.

I'll roll forward again behind a feature flag.

*** Original change description ***

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...

***

PiperOrigin-RevId: 354584882
  • Loading branch information
gregestren authored and copybara-github committed Jan 29, 2021
1 parent 9c5124f commit 36d228b
Show file tree
Hide file tree
Showing 13 changed files with 58 additions and 224 deletions.
7 changes: 0 additions & 7 deletions site/docs/visibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,6 @@ 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`, 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.

### Example

File `//frobber/bin/BUILD`:
Expand Down
15 changes: 0 additions & 15 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,6 @@ java_library(
":buildinfo/build_info_key",
":config/build_configuration",
":config/build_options",
":config/config_conditions",
":config/config_matching_provider",
":config/core_options",
":config/execution_transition_factory",
Expand Down Expand Up @@ -1608,20 +1607,6 @@ java_library(
],
)

java_library(
name = "config/config_conditions",
srcs = ["config/ConfigConditions.java"],
deps = [
":config/config_matching_provider",
":configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//third_party:auto_value",
"//third_party:guava",
],
)

java_library(
name = "config/core_option_converters",
srcs = ["config/CoreOptionConverters.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.RuleContext.InvalidExecGroupException;
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.Fragment;
import com.google.devtools.build.lib.analysis.config.RequiredFragmentsUtil;
import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget;
Expand Down Expand Up @@ -181,7 +181,7 @@ public final ConfiguredTarget createConfiguredTarget(
BuildConfiguration hostConfig,
ConfiguredTargetKey configuredTargetKey,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ConfigConditions configConditions,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
if (target instanceof Rule) {
Expand Down Expand Up @@ -287,7 +287,7 @@ private ConfiguredTarget createRule(
BuildConfiguration hostConfiguration,
ConfiguredTargetKey configuredTargetKey,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ConfigConditions configConditions,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainCollection<ResolvedToolchainContext> toolchainContexts)
throws InterruptedException, ActionConflictException, InvalidExecGroupException {
ConfigurationFragmentPolicy configurationFragmentPolicy =
Expand Down Expand Up @@ -318,7 +318,7 @@ private ConfiguredTarget createRule(
configuration,
ruleClassProvider.getUniversalFragments(),
configurationFragmentPolicy,
configConditions.asProviders(),
configConditions,
prerequisiteMap.values()))
.build();

Expand Down Expand Up @@ -495,7 +495,7 @@ public ConfiguredAspect createAspect(
ConfiguredAspectFactory aspectFactory,
Aspect aspect,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap,
ConfigConditions configConditions,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ResolvedToolchainContext toolchainContext,
BuildConfiguration aspectConfiguration,
BuildConfiguration hostConfiguration,
Expand Down Expand Up @@ -540,7 +540,7 @@ public ConfiguredAspect createAspect(
aspectConfiguration,
ruleClassProvider.getUniversalFragments(),
aspect.getDefinition().getConfigurationFragmentPolicy(),
configConditions.asProviders(),
configConditions,
prerequisiteMap.values()))
.build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum;
Expand Down Expand Up @@ -1765,7 +1764,7 @@ public static final class Builder implements RuleErrorConsumer {
private final PrerequisiteValidator prerequisiteValidator;
private final RuleErrorConsumer reporter;
private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap;
private ConfigConditions configConditions;
private ImmutableMap<Label, ConfigMatchingProvider> configConditions = ImmutableMap.of();
private String toolsRepository;
private StarlarkSemantics starlarkSemantics;
private Mutability mutability;
Expand Down Expand Up @@ -1814,15 +1813,11 @@ public RuleContext build() throws InvalidExecGroupException {
Preconditions.checkNotNull(visibility);
Preconditions.checkNotNull(constraintSemantics);
AttributeMap attributes =
ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions.asProviders());
ConfiguredAttributeMapper.of(target.getAssociatedRule(), configConditions);
checkAttributesNonEmpty(attributes);
ListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap();
Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
for (ConfiguredTargetAndData condition : configConditions.asConfiguredTargets().values()) {
validateDirectPrerequisite(configSettingAttr, condition);
}
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
createFilesetEntryMap(target.getAssociatedRule(), configConditions.asProviders());
createFilesetEntryMap(target.getAssociatedRule(), configConditions);
if (rawExecProperties == null) {
if (!attributes.has(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)) {
rawExecProperties = ImmutableMap.of();
Expand All @@ -1836,7 +1831,7 @@ public RuleContext build() throws InvalidExecGroupException {
attributes,
targetMap,
filesetEntryMap,
configConditions.asProviders(),
configConditions,
universalFragments,
getRuleClassNameForLogging(),
actionOwnerSymbol,
Expand Down Expand Up @@ -1911,10 +1906,11 @@ 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(ConfigConditions configConditions) {
public Builder setConfigConditions(
ImmutableMap<Label, ConfigMatchingProvider> configConditions) {
this.configConditions = Preconditions.checkNotNull(configConditions);
return this;
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -732,11 +732,7 @@ public RuleVisibility getVisibility() {
return visibility;
}

// 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();
return pkg.getDefaultVisibility();
}

public boolean isVisibilitySpecified() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
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;
Expand Down Expand Up @@ -403,7 +403,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

// Get the configuration targets that trigger this rule's configurable attributes.
ConfigConditions configConditions =
ImmutableMap<Label, ConfigMatchingProvider> configConditions =
ConfiguredTargetFunction.getConfigConditions(
env,
originalTargetAndAspectConfiguration,
Expand All @@ -423,7 +423,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
resolver,
originalTargetAndAspectConfiguration,
aspectPath,
configConditions.asProviders(),
configConditions,
unloadedToolchainContext == null
? null
: ToolchainCollection.builder()
Expand Down Expand Up @@ -640,7 +640,7 @@ private AspectValue createAspect(
ConfiguredAspectFactory aspectFactory,
ConfiguredTargetAndData associatedTarget,
BuildConfiguration aspectConfiguration,
ConfigConditions configConditions,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
ResolvedToolchainContext toolchainContext,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> directDeps,
@Nullable NestedSetBuilder<Package> transitivePackagesForPackageRootResolution)
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ 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",
Expand Down
Loading

0 comments on commit 36d228b

Please sign in to comment.