Same transition dirs when only cmd line options differs

Ignore user defined build settings only set on
command line and not affected by any transition,
when calculating transitionDirectoryNameFragment
output directory.

Native options on command line already have this
behavior, and this commit provides the same also
for user defined build settings.

This allows reducing transition scalability issues,
by combining transitions with command line options:

 a) Setting a limited number of common options via
    transitions, which affects large parts of the
    code base, but with limited number of variants.
 b) When needed, *also* setting a few of many
    specific options, each of them affecting only
    their parts of the code base, but with many
    variants.

Resolves #12731

Closes #13580.

PiperOrigin-RevId: 396689438

Author: ulrfa

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -261,9 +261,11 @@ public String getTypeDescription() {
   }
 
   /**
-   * This internal option is a *set* of names (e.g. "cpu") of *native* options that have been
-   * changed by starlark transitions at any point in the build at the time of accessing. This is
-   * used to regenerate {@code transitionDirectoryNameFragment} after each starlark transition.
+   * This internal option is a *set* of names of options that have been changed by starlark
+   * transitions at any point in the build at the time of accessing. It contains both native and
+   * starlark options in label form. e.g. "//command_line_option:cpu" for native options and
+   * "//myapp:foo" for starlark options. This is used to regenerate {@code
+   * transitionDirectoryNameFragment} after each starlark transition.
    */
   @Option(
       name = "affected by starlark transition",

src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

@@ -19,6 +19,7 @@
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.VerifyException;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -289,13 +290,13 @@ private static BuildOptions applyTransition(
 
       if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
         // The transition changes a Starlark option.
-        Object oldValue =
-            fromOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+        Label optionLabel = Label.parseAbsoluteUnchecked(optionName);
+        Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel);
         if ((oldValue == null && optionValue != null)
             || (oldValue != null && optionValue == null)
             || (oldValue != null && !oldValue.equals(optionValue))) {
-          changedStarlarkOptions.put(Label.parseAbsoluteUnchecked(optionName), optionValue);
-          convertedNewValues.add(optionName);
+          changedStarlarkOptions.put(optionLabel, optionValue);
+          convertedNewValues.add(optionLabel.toString());
         }
       } else {
         // The transition changes a native option.
@@ -398,9 +399,10 @@ private static BuildOptions applyTransition(
   }
 
   /**
-   * Compute the output directory name fragment corresponding to the new BuildOptions based on (1)
-   * the names and values of all native options previously transitioned anywhere in the build by
-   * starlark transitions, (2) names and values of all entries in the starlark options map.
+   * Compute the output directory name fragment corresponding to the new BuildOptions based on the
+   * names and values of all options (both native and Starlark) previously transitioned anywhere in
+   * the build by Starlark transitions. Options only set on command line are not affecting the
+   * computation.
    *
    * @param changedOptions the names of all options changed by this transition in label form e.g.
    *     "//command_line_option:cpu" for native options and "//myapp:foo" for starlark options.
@@ -421,38 +423,34 @@ private static void updateOutputDirectoryNameFragment(
     }
 
     CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
-    Set<String> updatedAffectedByStarlarkTransition =
-        new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
-    // Add newly changed native options to overall list of changed native options
-    for (String option : changedOptions) {
-      if (option.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
-        updatedAffectedByStarlarkTransition.add(
-            option.substring(COMMAND_LINE_OPTION_PREFIX.length()));
-      }
-    }
-    buildConfigOptions.affectedByStarlarkTransition =
-        ImmutableList.sortedCopyOf(updatedAffectedByStarlarkTransition);
 
-    // hash all relevant native option values;
+    updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);
+
     TreeMap<String, Object> toHash = new TreeMap<>();
-    for (String nativeOption : updatedAffectedByStarlarkTransition) {
-      Object value;
-      try {
-        value =
-            optionInfoMap
-                .get(nativeOption)
-                .getDefinition()
-                .getField()
-                .get(toOptions.get(optionInfoMap.get(nativeOption).getOptionClass()));
-      } catch (IllegalAccessException e) {
-        throw new RuntimeException(
-            "IllegalAccess for option " + nativeOption + ": " + e.getMessage());
+    for (String optionName : buildConfigOptions.affectedByStarlarkTransition) {
+      if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
+        String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
+        Object value;
+        try {
+          value =
+              optionInfoMap
+                  .get(nativeOptionName)
+                  .getDefinition()
+                  .getField()
+                  .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass()));
+        } catch (IllegalAccessException e) {
+          throw new VerifyException(
+              "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage());
+        }
+        toHash.put(optionName, value);
+      } else {
+        Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+        if (value != null) {
+          toHash.put(optionName, value);
+        }
       }
-      toHash.put(nativeOption, value);
     }
 
-    // hash all starlark options in map.
-    toOptions.getStarlarkOptions().forEach((opt, value) -> toHash.put(opt.toString(), value));
     ImmutableList.Builder<String> hashStrs = ImmutableList.builderWithExpectedSize(toHash.size());
     for (Map.Entry<String, Object> singleOptionAndValue : toHash.entrySet()) {
       String toAdd = singleOptionAndValue.getKey() + "=" + singleOptionAndValue.getValue();
@@ -462,6 +460,19 @@ private static void updateOutputDirectoryNameFragment(
         transitionDirectoryNameFragment(hashStrs.build());
   }
 
+  /**
+   * Extend the global build config affectedByStarlarkTransition, by adding any new option names
+   * from changedOptions
+   */
+  private static void updateAffectedByStarlarkTransition(
+      CoreOptions buildConfigOptions, Set<String> changedOptions) {
+    Set<String> mutableCopyToUpdate =
+        new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
+    mutableCopyToUpdate.addAll(changedOptions);
+    buildConfigOptions.affectedByStarlarkTransition =
+        ImmutableList.sortedCopyOf(mutableCopyToUpdate);
+  }
+
   public static String transitionDirectoryNameFragment(Iterable<String> opts) {
     Fingerprint fp = new Fingerprint();
     for (String opt : opts) {

src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java

@@ -1137,10 +1137,59 @@ public void testTransitionOnBuildSetting_fromCommandLine() throws Exception {
         .isEqualTo(StarlarkInt.of(42));
   }
 
+  @Test
+  public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throws Exception {
+    writeAllowlistFile();
+    writeBuildSettingsBzl();
+    writeRulesWithAttrTransitionBzl();
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:rules.bzl', 'my_rule')",
+        "load('//test/starlark:build_settings.bzl', 'int_flag')",
+        "my_rule(name = 'test', dep = ':dep')",
+        "my_rule(name = 'dep')",
+        "int_flag(name = 'the-answer', build_setting_default = 0)",
+        "int_flag(name = 'cmd-line-option', build_setting_default = 0)");
+
+    useConfiguration(ImmutableMap.of("//test/starlark:cmd-line-option", 100), "--cpu=FOO");
+
+    ConfiguredTarget test = getConfiguredTarget("//test/starlark:test");
+
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
+
+    // Assert starlark option set via transition.
+    assertThat(getStarlarkOption(dep, "//test/starlark:the-answer")).isEqualTo(StarlarkInt.of(42));
+
+    // Assert starlark option set via command line.
+    assertThat(getStarlarkOption(dep, "//test/starlark:cmd-line-option")).isEqualTo(100);
+
+    // Assert native option set via command line.
+    assertThat(getCoreOptions(dep).cpu).isEqualTo("FOO");
+
+    // Assert that transitionDirectoryNameFragment is only affected by options
+    // set via transitions. Not by native or starlark options set via command line,
+    // never touched by any transition.
+    assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
+        .isEqualTo(
+            FunctionTransitionUtil.transitionDirectoryNameFragment(
+                ImmutableList.of("//test/starlark:the-answer=42")));
+  }
+
   private CoreOptions getCoreOptions(ConfiguredTarget target) {
     return getConfiguration(target).getOptions().get(CoreOptions.class);
   }
 
+  private ImmutableMap<Label, Object> getStarlarkOptions(ConfiguredTarget target) {
+    return getConfiguration(target).getOptions().getStarlarkOptions();
+  }
+
+  private Object getStarlarkOption(ConfiguredTarget target, String absName) {
+    return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName));
+  }
+
   @Test
   public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception {
     writeAllowlistFile();
@@ -1182,7 +1231,7 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception
 
     List<String> affectedOptions = getCoreOptions(test).affectedByStarlarkTransition;
 
-    assertThat(affectedOptions).containsExactly("foo");
+    assertThat(affectedOptions).containsExactly("//command_line_option:foo");
 
     @SuppressWarnings("unchecked")
     ConfiguredTarget dep =
@@ -1191,17 +1240,19 @@ public void testOutputDirHash_multipleNativeOptionTransitions() throws Exception
 
     affectedOptions = getCoreOptions(dep).affectedByStarlarkTransition;
 
-    assertThat(affectedOptions).containsExactly("foo", "bar");
+    assertThat(affectedOptions)
+        .containsExactly("//command_line_option:foo", "//command_line_option:bar");
 
     assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
-                ImmutableList.of("foo=foosball")));
+                ImmutableList.of("//command_line_option:foo=foosball")));
 
     assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
-                ImmutableList.of("bar=barsball", "foo=foosball")));
+                ImmutableList.of(
+                    "//command_line_option:bar=barsball", "//command_line_option:foo=foosball")));
   }
 
   // Test that a no-op starlark transition to an already starlark transitioned configuration
@@ -1523,8 +1574,7 @@ public void testOutputDirHash_multipleStarlarkTransitions() throws Exception {
     List<String> affectedOptions =
         getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
 
-    // Assert that affectedOptions is empty but final fragment is still different.
-    assertThat(affectedOptions).isEmpty();
+    assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo");
     assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
@@ -1611,12 +1661,12 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
     List<String> affectedOptionsTop =
         getConfiguration(top).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
 
-    assertThat(affectedOptionsTop).containsExactly("foo");
+    assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo");
     assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty();
     assertThat(getCoreOptions(top).transitionDirectoryNameFragment)
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
-                ImmutableList.of("foo=foosball")));
+                ImmutableList.of("//command_line_option:foo=foosball")));
 
     // test:middle (foo_transition, zee_transition, bar_transition)
     @SuppressWarnings("unchecked")
@@ -1626,14 +1676,20 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
     List<String> affectedOptionsMiddle =
         getConfiguration(middle).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
 
-    assertThat(affectedOptionsMiddle).containsExactly("foo", "bar");
+    assertThat(affectedOptionsMiddle)
+        .containsExactly("//command_line_option:foo", "//command_line_option:bar", "//test:zee");
+
     assertThat(getConfiguration(middle).getOptions().getStarlarkOptions().entrySet())
         .containsExactly(
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"));
+
     assertThat(getCoreOptions(middle).transitionDirectoryNameFragment)
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
-                ImmutableList.of("//test:zee=zeesball", "bar=barsball", "foo=foosball")));
+                ImmutableList.of(
+                    "//command_line_option:bar=barsball",
+                    "//command_line_option:foo=foosball",
+                    "//test:zee=zeesball")));
 
     // test:bottom (foo_transition, zee_transition, bar_transition, xan_transition)
     @SuppressWarnings("unchecked")
@@ -1644,7 +1700,10 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
     List<String> affectedOptionsBottom =
         getConfiguration(bottom).getOptions().get(CoreOptions.class).affectedByStarlarkTransition;
 
-    assertThat(affectedOptionsBottom).containsExactly("foo", "bar");
+    assertThat(affectedOptionsBottom)
+        .containsExactly(
+            "//command_line_option:foo", "//command_line_option:bar", "//test:xan", "//test:zee");
+
     assertThat(getConfiguration(bottom).getOptions().getStarlarkOptions().entrySet())
         .containsExactly(
             Maps.immutableEntry(Label.parseAbsoluteUnchecked("//test:zee"), "zeesball"),
@@ -1653,7 +1712,8 @@ public void testOutputDirHash_multipleMixedTransitions() throws Exception {
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of(
-                    "//test:xan=xansball", "//test:zee=zeesball", "bar=barsball", "foo=foosball")));
+                    "//command_line_option:bar=barsball", "//command_line_option:foo=foosball",
+                    "//test:xan=xansball", "//test:zee=zeesball")));
   }
 
   @Test