Fix NativeAndStarlarkFlags to strip default-valued Starlark flags.

Part of bazelbuild#23147. PiperOrigin-RevId: 659666453 Change-Id: Ib380abe58e9eda3adddab21851dec3a599e8cf36
fmeum · Aug 5, 2024 · 3820bc1 · 3820bc1
1 parent eaf114c
commit 3820bc1
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 13 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -393,6 +393,13 @@ public Builder addStarlarkOption(Label key, Object value) {
       return this;
     }
 
+    /** Removes the Starlark option from this builder. */
+    @CanIgnoreReturnValue
+    public Builder removeStarklarkOption(Label key) {
+      starlarkOptions.remove(key);
+      return this;
+    }
+
     /** Removes the value for the Starlark option with the given key. */
     @CanIgnoreReturnValue
     public Builder removeStarlarkOption(Label key) {

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/NativeAndStarlarkFlags.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/NativeAndStarlarkFlags.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe.config;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -144,12 +145,8 @@ public BuildOptions mergeWith(BuildOptions source) throws OptionsParsingExceptio
     }
 
     // Also copy Starlark options.
-    Map<Label, Object> parsedStarlarkOptions =
-        BuildOptions.labelizeStarlarkOptions(parsingResult.getStarlarkOptions());
-    for (Map.Entry<Label, Object> starlarkOption : parsedStarlarkOptions.entrySet()) {
-      // TODO: https://github.com/bazelbuild/bazel/issues/23147 -check for default values and remove
-      // them.
-      builder.addStarlarkOption(starlarkOption.getKey(), starlarkOption.getValue());
+    for (Map.Entry<String, Object> starlarkOption : parsingResult.getStarlarkOptions().entrySet()) {
+      updateStarlarkFlag(builder, starlarkOption.getKey(), starlarkOption.getValue());
     }
 
     return builder.build();
@@ -165,4 +162,23 @@ private static void updateOptionValue(
     Object value = optionValue.getValue();
     optionDefinition.setValue(fragment, value);
   }
+
+  private void updateStarlarkFlag(
+      BuildOptions.Builder builder, String rawFlagName, Object rawFlagValue) {
+    Label flagName = Label.parseCanonicalUnchecked(rawFlagName);
+    // If the known default value is the same as the new value, unset it.
+    if (isStarlarkFlagSetToDefault(rawFlagName, rawFlagValue)) {
+      builder.removeStarklarkOption(flagName);
+    } else {
+      builder.addStarlarkOption(flagName, rawFlagValue);
+    }
+  }
+
+  private boolean isStarlarkFlagSetToDefault(String rawFlagName, Object rawFlagValue) {
+    if (this.starlarkFlagDefaults().containsKey(rawFlagName)) {
+      Object defaultValue = this.starlarkFlagDefaults().get(rawFlagName);
+      return Objects.equal(defaultValue, rawFlagValue);
+    }
+    return false;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/ParsedFlagsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/ParsedFlagsFunction.java
@@ -79,16 +79,18 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     } catch (OptionsParsingException e) {
       throw new ParsedFlagsFunctionException(e);
     }
-    NativeAndStarlarkFlags flags =
+    NativeAndStarlarkFlags.Builder flags =
         NativeAndStarlarkFlags.builder()
             .nativeFlags(nativeFlags.build())
             .starlarkFlags(starlarkFlagParser.getStarlarkOptions())
-            .starlarkFlagDefaults(starlarkFlagParser.getDefaultValues())
             .optionsClasses(optionsClasses)
-            .repoMapping(key.packageContext().repoMapping())
-            .build();
+            .repoMapping(key.packageContext().repoMapping());
+
+    if (key.includeDefaultValues()) {
+      flags.starlarkFlagDefaults(starlarkFlagParser.getDefaultValues());
+    }
 
-    return ParsedFlagsValue.create(flags);
+    return ParsedFlagsValue.create(flags.build());
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/NativeAndStarlarkFlagsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/NativeAndStarlarkFlagsTest.java
@@ -29,7 +29,6 @@
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionsParsingResult;
 import java.util.List;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -265,7 +264,6 @@ public void mergeWith_starlark() throws Exception {
   }
 
   @Test
-  @Ignore("https://github.com/bazelbuild/bazel/issues/23147")
   public void mergeWith_starlark_resetToDefault() throws Exception {
     BuildOptions original =
         BuildOptions.of(BUILD_CONFIG_OPTIONS).toBuilder()