Fix label conversion for --experimental_propagate_custom_flag

Also fix and enable a test in Bazel. Fixes #25208 Closes #25259. PiperOrigin-RevId: 726541288 Change-Id: If4c021b821c43e288ddcb1798ead13ca5a50d7aa
bazelbuild · Feb 13, 2025 · e1d5e86 · e1d5e86
1 parent 9b211de
commit e1d5e86
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 11 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -2545,6 +2545,7 @@ java_library(
     srcs = ["starlark/FunctionTransitionUtil.java"],
     deps = [
         ":config/build_options",
+        ":config/core_option_converters",
         ":config/core_options",
         ":config/fragment_options",
         ":config/optioninfo",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
@@ -234,6 +234,45 @@ public String getTypeDescription() {
     }
   }
 
+  /**
+   * Flag converter for canonicalizing a label (possibly with a "/..." suffix) and/or define by
+   * converting the label to unambiguous canonical form.
+   */
+  public static class CustomFlagConverter implements Converter<String> {
+    public static final String SUBPACKAGES_SUFFIX = "/...";
+
+    @Override
+    public String convert(String input, Object conversionContext) throws OptionsParsingException {
+      if (!input.startsWith("//") && !input.startsWith("@")) {
+        // This is a --define flag.
+        return input;
+      }
+      // A "/..." suffix is not valid label syntax, so replace it with arbitrary valid syntax and
+      // transform it back after conversion.
+      String invalidSubpackagesSuffix = SUBPACKAGES_SUFFIX;
+      String validSubpackagesSuffix = ":__subpackages__";
+      String escapedUnconvertedLabel =
+          input.endsWith(invalidSubpackagesSuffix)
+              ? input.substring(0, input.length() - invalidSubpackagesSuffix.length())
+                  + validSubpackagesSuffix
+              : input;
+      String escapedConvertedLabel =
+          convertOptionsLabel(escapedUnconvertedLabel, conversionContext)
+              .getUnambiguousCanonicalForm();
+      if (escapedConvertedLabel.endsWith(validSubpackagesSuffix)) {
+        return escapedConvertedLabel.substring(
+                0, escapedConvertedLabel.length() - validSubpackagesSuffix.length())
+            + invalidSubpackagesSuffix;
+      }
+      return escapedConvertedLabel;
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "an absolute label or define";
+    }
+  }
+
   /** Values for the --strict_*_deps option */
   public enum StrictDepsMode {
     /** Silently allow referencing transitive dependencies. */

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -96,6 +96,7 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       name = "experimental_propagate_custom_flag",
       defaultValue = "null",
       allowMultiple = true,
+      converter = CoreOptionConverters.CustomFlagConverter.class,
       documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
       metadataTags = {OptionMetadataTag.EXPERIMENTAL},

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
@@ -32,6 +32,7 @@
 import com.google.common.collect.Sets.SetView;
 import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.CustomFlagConverter;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.OptionInfo;
@@ -217,13 +218,15 @@ private static boolean propagateStarlarkFlagToExec(
     return buildOptions.get(CoreOptions.class).customFlagsToPropagate.stream()
         .anyMatch(
             flagToPropagate ->
-                (flagToPropagate.equals(starlarkFlag.toString())
-                    || (flagToPropagate.endsWith("/...")
+                (flagToPropagate.equals(starlarkFlag.getUnambiguousCanonicalForm())
+                    || (flagToPropagate.endsWith(CustomFlagConverter.SUBPACKAGES_SUFFIX)
                         && starlarkFlag
-                            .toString()
+                            .getUnambiguousCanonicalForm()
                             .startsWith(
                                 flagToPropagate.substring(
-                                    0, flagToPropagate.lastIndexOf("/..."))))));
+                                    0,
+                                    flagToPropagate.lastIndexOf(
+                                        CustomFlagConverter.SUBPACKAGES_SUFFIX))))));
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java
@@ -27,7 +27,6 @@
 import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.common.options.Options;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -325,7 +324,9 @@ def _impl(ctx):
                 "//my_starlark_flag:other_starlark_flag",
                 "true"),
             "--experimental_exclude_starlark_flags_from_exec_config=true",
-            "--experimental_propagate_custom_flag=//my_starlark_flag:starlark_flag");
+            // Verify that labels are parsed rather than compared as strings by specifying the
+            // label in non-canonical form.
+            "--experimental_propagate_custom_flag=@//my_starlark_flag:starlark_flag");
     assertThat(
             cfg.getOptions()
                 .getStarlarkOptions()
@@ -339,10 +340,8 @@ def _impl(ctx):
   }
 
   @Test
-  // TODO: b/377959266 - fix test setup bug that fails Bazel CI. This test's logic doesn't cause the
-  //   bug: it's somehow caused by test naming. See bug for details.
-  @Ignore("b/377959266")
   public void testExecStarlarkFlag_isPropagatedByTargetPattern() throws Exception {
+    scratch.file("my_starlark_flag/BUILD");
     scratch.file(
         "my_starlark_flag/rule_defs.bzl",
         """
@@ -357,7 +356,7 @@ public void testExecStarlarkFlag_isPropagatedByTargetPattern() throws Exception
         load("//my_starlark_flag:rule_defs.bzl", "bool_flag")
         bool_flag(
             name = "include_me",
-            build_setting_default = "False",
+            build_setting_default = False,
         )
         """);
     scratch.file(
@@ -366,7 +365,7 @@ public void testExecStarlarkFlag_isPropagatedByTargetPattern() throws Exception
         load("//my_starlark_flag:rule_defs.bzl", "bool_flag")
         bool_flag(
             name = "exclude_me",
-            build_setting_default = "False",
+            build_setting_default = False,
         )
         """);