Review updates

bazelbuild · Sep 13, 2021 · 104f1dd · 104f1dd
1 parent 68f5f55
commit 104f1dd
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 30 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -275,15 +275,15 @@ private static BuildOptions applyTransition(
       Object optionValue = entry.getValue();
 
       if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
+        Label optionLabel = Label.parseAbsoluteUnchecked(optionName);
         Object oldValue =
-            buildOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+            buildOptions.getStarlarkOptions().get(optionLabel);
         if ((oldValue == null && optionValue != null)
             || (oldValue != null && optionValue == null)
             || (oldValue != null && !oldValue.equals(optionValue))) {
           // TODO(bazel-team): Figure out if we need to create a whole new build options every
           // time. Can we just keep track of the running changes and actually build a new build
           // options after this loop?
-          Label optionLabel = Label.parseAbsoluteUnchecked(optionName);
           buildOptions =
               BuildOptions.builder()
                   .merge(buildOptions)
@@ -383,10 +383,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 options, (2) names and values of entries in the starlark options map previously
-   * transitioned anywhere in the build and not only set on command line.
+   * 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.
@@ -407,41 +407,31 @@ private static void updateOutputDirectoryNameFragment(
     }
 
     CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
-    Set<String> updatedAffectedByStarlarkTransition =
-        new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
-    // Add newly changed options to overall list of changed options
-    for (String option : changedOptions) {
-      updatedAffectedByStarlarkTransition.add(option);
-    }
-    buildConfigOptions.affectedByStarlarkTransition =
-        ImmutableList.sortedCopyOf(updatedAffectedByStarlarkTransition);
-    TreeMap<String, Object> toHash = new TreeMap<>();
 
-    // Hash all affected native options.
+    updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions);
+
+    TreeMap<String, Object> toHash = new TreeMap<>();
     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()));
+              optionInfoMap
+                      .get(nativeOptionName)
+                      .getDefinition()
+                      .getField()
+                      .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass()));
         } catch (IllegalAccessException e) {
           throw new RuntimeException(
                   "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage());
         }
         toHash.put(optionName, value);
-      }
-    }
-
-    // Hash all affected starlark options.
-    for (Map.Entry<Label, Object> opt : toOptions.getStarlarkOptions().entrySet()) {
-      String optionName = opt.getKey().toString();
-      if (updatedAffectedByStarlarkTransition.contains(optionName)) {
-        toHash.put(optionName, opt.getValue());
+      } else {
+        Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
+        if (value != null) {
+          toHash.put(optionName, value);
+        }
       }
     }
 
@@ -454,6 +444,18 @@ 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);
+    for (String option : changedOptions) {
+      mutableCopyToUpdate.add(option);
+    }
+    buildConfigOptions.affectedByStarlarkTransition = ImmutableList.sortedCopyOf(mutableCopyToUpdate);
+  }
+
   public static String transitionDirectoryNameFragment(Iterable<String> opts) {
     Fingerprint fp = new Fingerprint();
     for (String opt : opts) {

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -1470,7 +1470,6 @@ public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEqua
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(
                 ImmutableList.of("//test:foo=1")));
-
     assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
         .isEqualTo(
             FunctionTransitionUtil.transitionDirectoryNameFragment(