From adc887a769044f28729df86da0e44d022db8c2e3 Mon Sep 17 00:00:00 2001 From: gregce Date: Thu, 28 Jan 2021 09:18:53 -0800 Subject: [PATCH] Simplify config_setting visibility checking. Make select() keys normal attribute -> configured target prerequisite dependencies. This automatically opts them into RuleContext.Builder's standard visibility checking. The main historic argument against this is interference with "manual trimming". But that's an on-ice feature that needs fundamental reevaluation and shouldn't block other code in the meantime. This is a simplified alternative to https://github.com/bazelbuild/bazel/pull/12877, for https://github.com/bazelbuild/bazel/issues/12669. PiperOrigin-RevId: 354326088 Change-Id: Ifaafc563f22d401599540ead70e49db6a86a1995 --- .../lib/analysis/DependencyResolver.java | 9 +------ .../build/lib/analysis/RuleContext.java | 4 ---- .../build/lib/packages/RuleClass.java | 24 ++++++++----------- 3 files changed, 11 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index 5eb5d4dba2c1ef..a39df40ce91b00 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -578,14 +578,7 @@ private void resolveAttributes( Label ruleLabel = rule.getLabel(); for (AttributeDependencyKind dependencyKind : getAttributes(rule, aspects)) { Attribute attribute = dependencyKind.getAttribute(); - if (!attribute.getCondition().apply(attributeMap) - // Not only is resolving CONFIG_SETTING_DEPS_ATTRIBUTE deps here wasteful, since the only - // place they're used is in ConfiguredTargetFunction.getConfigConditions, but it actually - // breaks trimming as shown by - // FeatureFlagManualTrimmingTest#featureFlagInUnusedSelectBranchButNotInTransitiveConfigs_DoesNotError - // because it resolves a dep that trimming (correctly) doesn't account for because it's - // part of an unchosen select() branch. - || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)) { + if (!attribute.getCondition().apply(attributeMap)) { continue; } 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 792bef339d4b13..7dbc8816b7d3ad 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 @@ -1817,10 +1817,6 @@ public RuleContext build() throws InvalidExecGroupException { 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.asProviders()); if (rawExecProperties == null) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 5241c42b6f3122..79bd3af72fc953 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -287,28 +287,24 @@ public RuleErrorException(String message, Throwable cause) { /** * Name of the attribute that stores all {@link - * com.google.devtools.build.lib.rules.config.ConfigRuleClasses} labels this rule references (i.e. - * select() keys). This is specially populated in {@link #populateRuleAttributeValues}. + * com.google.devtools.build.lib.rules.config.ConfigRuleClasses} labels in a + * select(). This is specially populated in {@link #populateRuleAttributeValues}. * - *

This isn't technically necessary for builds: select() keys are evaluated in {@link - * com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#getConfigConditions} instead of - * normal dependency resolution because they're needed to determine other dependencies. So there's - * no intrinsic reason why we need an extra attribute to store them. + *

We don't technically need this attribute. This info is only needed for {@link + * com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#getConfigConditions}, which + * could also retrieve it by iterating over every attribute with a select() * - *

There are three reasons why we still create this attribute: + *

We keep it for these reasons: * *

    - *
  1. Collecting them once in {@link #populateRuleAttributeValues} instead of multiple times in - * ConfiguredTargetFunction saves extra looping over the rule's attributes. + *
  2. Collecting conditions once in {@link #populateRuleAttributeValues} instead of multiple + * times in ConfiguredTargetFunction saves extra looping over the rule's attributes. *
  3. Query's dependency resolution has no equivalent of {@link * com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction#getConfigConditions} and * we need to make sure its coverage remains complete. - *
  4. Manual configuration trimming uses the normal dependency resolution process to work - * correctly and config_setting keys are subject to this trimming. + *
  5. Storing it as an attribute guarantees select() conditions are validity + * checked in {@link RuleContext.Builder#validateDirectPrerequisite}. *
- * - *

It should be possible to clean up these issues if we decide we don't want an artificial - * attribute dependency. But care has to be taken to do that safely. */ public static final String CONFIG_SETTING_DEPS_ATTRIBUTE = "$config_dependencies";