Self transition constraint_setting and constraint_value to the NoConf…

…ig state. These rules can only define constants, and all their attributes are not configurable. This reduces the number of configurations constraint values appear in, but does yet reduce that to a single instance per value. For this cquery ``` blaze cquery --compilation_mode=opt --fat_apk_cpu=x86 \ --android_platforms=//buildenv/platforms/android:x86 \ --incompatible_enable_android_toolchain_resolution=1 \ --//third_party/java/android:system_api=true \ 'allpaths(//java/com/android/dialer:dialer, //third_party/bazel_platforms/...)' \ | grep bazel_platforms/os | sort -u ``` We see 71 different combinations of constraint values and cquery configs before this change and 37 after. That is better, but inexplicably much higher than the 8 or so I would expect. RELNOTES: None PiperOrigin-RevId: 501909093 Change-Id: Ie66ac2b144837050fdb3e1fd18f5dc1ba155e480
benradf · Jan 13, 2023 · 0fe4c36 · 0fe4c36
1 parent d429313
commit 0fe4c36
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 4 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/BUILD b/src/main/java/com/google/devtools/build/lib/rules/platform/BUILD
@@ -20,6 +20,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/auto_cpu_converter",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_config_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",

diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintSettingRule.java
@@ -16,11 +16,16 @@
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
+import com.google.devtools.build.lib.analysis.config.transitions.NoConfigTransition;
 import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
+import com.google.devtools.build.lib.packages.Type;
 
 /** Rule definition for {@link ConstraintSetting}. */
 public class ConstraintSettingRule implements RuleDefinition {
@@ -31,6 +36,20 @@ public class ConstraintSettingRule implements RuleDefinition {
   public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
     return builder
         .advertiseStarlarkProvider(ConstraintSettingInfo.PROVIDER.id())
+        .cfg(NoConfigTransition.createFactory())
+        .exemptFromConstraintChecking("this rule helps *define* a constraint")
+        .useToolchainResolution(ToolchainResolutionMode.DISABLED)
+        .override(
+            attr("applicable_licenses", BuildType.LABEL_LIST)
+                // This is a constant which is never linked into a target
+                .value(ImmutableList.of())
+                .allowedFileTypes()
+                .nonconfigurable("fundamental constant, used in platform configuration"))
+        .override(
+            attr("tags", Type.STRING_LIST)
+                // No need to show up in ":all", etc. target patterns.
+                .value(ImmutableList.of("manual"))
+                .nonconfigurable("low-level attribute, used in platform configuration"))
         /* <!-- #BLAZE_RULE(constraint_setting).ATTRIBUTE(default_constraint_value) -->
         The label of the default value for this setting, to be used if no value is given. If this
         attribute is present, the <code>constraint_value</code> it points to must be defined in the
@@ -53,7 +72,7 @@ constraint list (such as for a <code>config_setting</code>) that requires a part
   public RuleDefinition.Metadata getMetadata() {
     return RuleDefinition.Metadata.builder()
         .name(RULE_NAME)
-        .ancestors(PlatformBaseRule.class)
+        .ancestors(BaseRuleClasses.NativeBuildRule.class)
         .factoryClass(ConstraintSetting.class)
         .build();
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/ConstraintValueRule.java
@@ -16,12 +16,17 @@
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
+import com.google.devtools.build.lib.analysis.config.transitions.NoConfigTransition;
 import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
+import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
 
 /** Rule definition for {@link ConstraintValue}. */
@@ -33,6 +38,20 @@ public class ConstraintValueRule implements RuleDefinition {
   public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
     return builder
         .advertiseStarlarkProvider(ConstraintValueInfo.PROVIDER.id())
+        .cfg(NoConfigTransition.createFactory())
+        .exemptFromConstraintChecking("this rule helps *define* a constraint")
+        .useToolchainResolution(ToolchainResolutionMode.DISABLED)
+        .override(
+            attr("applicable_licenses", BuildType.LABEL_LIST)
+                // This is a constant which is never linked into a target
+                .value(ImmutableList.of())
+                .allowedFileTypes()
+                .nonconfigurable("fundamental constant, used in platform configuration"))
+        .override(
+            attr("tags", Type.STRING_LIST)
+                // No need to show up in ":all", etc. target patterns.
+                .value(ImmutableList.of("manual"))
+                .nonconfigurable("low-level attribute, used in platform configuration"))
         /* <!-- #BLAZE_RULE(constraint_value).ATTRIBUTE(constraint_setting) -->
         The <code>constraint_setting</code> for which this <code>constraint_value</code> is a
         possible choice.
@@ -51,7 +70,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
   public Metadata getMetadata() {
     return Metadata.builder()
         .name(RULE_NAME)
-        .ancestors(PlatformBaseRule.class)
+        .ancestors(BaseRuleClasses.NativeBuildRule.class)
         .factoryClass(ConstraintValue.class)
         .build();
   }

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -671,6 +671,9 @@ public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments()
 
           @Override
           public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+            if (options.underlying().hasNoConfig()) {
+              return options.underlying();
+            }
             BuildOptionsView cloned = options.clone();
             cloned.get(DiffResetOptions.class).probablyIrrelevantOption = "(cleared)";
             cloned.get(DiffResetOptions.class).alsoIrrelevantOption = "(cleared)";

diff --git a/src/test/java/com/google/devtools/build/lib/runtime/commands/ConfigCommandTest.java b/src/test/java/com/google/devtools/build/lib/runtime/commands/ConfigCommandTest.java
@@ -168,6 +168,9 @@ private static String getOptionValue(
   }
 
   private static boolean isTargetConfig(ConfigurationForOutput config) {
+    if (config.mnemonic.endsWith("-noconfig")) {
+      return false;
+    }
     return !Boolean.parseBoolean(getOptionValue(config, "CoreOptions", "is exec configuration"));
   }
 
@@ -189,7 +192,7 @@ public void showConfigIds() throws Exception {
     // Should be: target configuration, target configuration without test.
     assertThat(fullJson).isNotNull();
     assertThat(fullJson.has("configuration-IDs")).isTrue();
-    assertThat(fullJson.get("configuration-IDs").getAsJsonArray().size()).isEqualTo(2);
+    assertThat(fullJson.get("configuration-IDs").getAsJsonArray().size()).isEqualTo(3);
   }
 
   @Test
@@ -279,7 +282,7 @@ public void showAllConfigs() throws Exception {
       assertThat(config).isNotNull();
       numConfigs++;
     }
-    assertThat(numConfigs).isEqualTo(2); // Target + target w/o test.
+    assertThat(numConfigs).isEqualTo(3); // Target + target w/o test + nonConfig.
   }
 
   @Test