Last bug fix for the starlark build settings x repositories clean up.

This brings us home to the principle of "we always deal with canonicalized settings on the back end". Before this CL, this situation described in bazelbuild#10499 was still an issue.

Some background:

By design, the label-object map of starlark options in BuildOptions does not store starlark flags that are set to their default value (set either by never having been set, or explicitly set to the default value). This means before we do starlark transitions, we fill out the configuration with all the relevant default values so they're there if we need to read them. Before this CL, those editions to the map were being added in their given form, not there canonical form. This obviously caused issues when our transition logic tried to find these setting using their canonical form. This CL fixes that. The affected logic is ConfigurationResolver#addDefaultStarlarkOptions.

Fixes bazelbuild#11128

PiperOrigin-RevId: 353923997

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

@@ -75,7 +75,7 @@ public enum Settings {
   }
 
   private final ImmutableMap<String, String> inputsCanonicalizedToGiven;
-  private final ImmutableList<String> outputs;
+  private final ImmutableMap<String, String> outputsCanonicalizedToGiven;
   private final Location location;
 
   private StarlarkDefinedConfigTransition(
@@ -87,12 +87,8 @@ private StarlarkDefinedConfigTransition(
       throws EvalException {
     this.location = location;
 
-    // Though we only need the given forms of the outputs, run it through #getCanonicalizedSettings
-    // in order to get the validity checking that method provides.
-    this.outputs =
-        getCanonicalizedSettings(repoMapping, parentLabel, outputs, Settings.OUTPUTS)
-            .values()
-            .asList();
+    this.outputsCanonicalizedToGiven =
+        getCanonicalizedSettings(repoMapping, parentLabel, outputs, Settings.OUTPUTS);
     this.inputsCanonicalizedToGiven =
         getCanonicalizedSettings(repoMapping, parentLabel, inputs, Settings.INPUTS);
   }
@@ -172,7 +168,11 @@ public ImmutableMap<String, String> getInputsCanonicalizedToGiven() {
    * function must return a dictionary where the options exactly match the elements of this list.
    */
   public ImmutableList<String> getOutputs() {
-    return outputs;
+    return outputsCanonicalizedToGiven.values().asList();
+  }
+
+  public ImmutableMap<String, String> getOutputsCanonicalizedToGiven() {
+    return outputsCanonicalizedToGiven;
   }
 
   /**

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

@@ -66,12 +66,14 @@ public StarlarkTransition(StarlarkDefinedConfigTransition starlarkDefinedConfigT
     this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
   }
 
+  // Get the inputs of the starlark transition as a list of canonicalized labels strings.
   private List<String> getInputs() {
-    return starlarkDefinedConfigTransition.getInputs();
+    return starlarkDefinedConfigTransition.getInputsCanonicalizedToGiven().keySet().asList();
   }
 
+  // Get the outputs of the starlark transition as a list of canonicalized labels strings.
   private List<String> getOutputs() {
-    return starlarkDefinedConfigTransition.getOutputs();
+    return starlarkDefinedConfigTransition.getOutputsCanonicalizedToGiven().keySet().asList();
   }
 
   @Override

src/test/shell/integration/starlark_configurations_external_workspaces_test.sh

@@ -54,7 +54,6 @@ msys*|mingw*|cygwin*)
   declare -r is_windows=false
   ;;
 esac
-
 if "$is_windows"; then
   export MSYS_NO_PATHCONV=1
   export MSYS2_ARG_CONV_EXCL="*"
@@ -139,5 +138,106 @@ function test_set_flag_with_workspace_name() {
   expect_log "type=coffee"
 }
 
+function test_reference_inner_repository_flags() {
+  local -r pkg=$FUNCNAME
+  local -r subpkg="$pkg/sub"
+  mkdir -p $subpkg
+
+  ## set up outer repo
+  cat > $pkg/WORKSPACE <<EOF
+local_repository(
+  name = "sub",
+  path = "./sub")
+EOF
+
+  ## set up inner repo
+  cat > $subpkg/BUILD <<EOF
+load(":rules.bzl", "rule_with_transition", "my_flag")
+
+my_flag(
+    name = "my_flag",
+    build_setting_default = "saguaro",
+)
+
+rule_with_transition(
+    name = "my_target",
+    src = ":my_flag",
+)
+EOF
+
+  cat > $subpkg/rules.bzl <<EOF
+BuildSettingInfo = provider(fields = ['value'])
+
+def _flag_impl(ctx):
+ return BuildSettingInfo(value = ctx.build_setting_value)
+
+my_flag = rule(
+  implementation = _flag_impl,
+  build_setting = config.string(flag = True),
+)
+
+def _my_transition_impl(settings, attr):
+    print("value before transition: " + settings["@sub//:my_flag"])
+    return {"@sub//:my_flag": "prickly-pear"}
+
+my_transition = transition(
+    implementation = _my_transition_impl,
+    inputs = ["@sub//:my_flag"],
+    outputs = ["@sub//:my_flag"],
+)
+
+def _rule_impl(ctx):
+    print("value after transition: " + ctx.attr.src[BuildSettingInfo].value)
+
+rule_with_transition = rule(
+    implementation = _rule_impl,
+    cfg = my_transition,
+    attrs = {
+        "src": attr.label(allow_files = True),
+        "_whitelist_function_transition":
+            attr.label(default = "@bazel_tools//tools/whitelists/function_transition_whitelist"),
+    },
+)
+EOF
+
+  cat > $subpkg/WORKSPACE <<EOF
+workspace(name = "sub")
+EOF
+
+  # from the outer repo
+  cd $pkg
+  bazel build @sub//:my_target \
+      > output 2>"$TEST_log" || fail "Expected success"
+  expect_log "value before transition: saguaro"
+  expect_log "value after transition: prickly-pear"
+
+  bazel build @sub//:my_target --@sub//:my_flag=prickly-pear \
+       > output 2>"$TEST_log" || fail "Expected success"
+  expect_log "value before transition: prickly-pear"
+  expect_log "value after transition: prickly-pear"
+
+  # from the inner repo
+  cd sub
+  bazel build :my_target \
+      > output 2>"$TEST_log" || fail "Expected success"
+  expect_log "value before transition: saguaro"
+  expect_log "value after transition: prickly-pear"
+
+  bazel build :my_target --//:my_flag=prickly-pear \
+      > output 2>"$TEST_log" || fail "Expected success"
+  expect_log "value before transition: prickly-pear"
+  expect_log "value after transition: prickly-pear"
+
+  bazel build :my_target --@sub//:my_flag=prickly-pear \
+      > output 2>"$TEST_log" || fail "Expected success"
+  expect_log "value before transition: prickly-pear"
+  expect_log "value after transition: prickly-pear"
+
+  bazel build :my_target --@//:my_flag=prickly-pear \
+      > output 2>"$TEST_log" || fail "Expected success"
+  expect_log "value before transition: prickly-pear"
+  expect_log "value after transition: prickly-pear"
+}
+
 
 run_suite "${PRODUCT_NAME} starlark configurations tests"