From b723a85208fd803c81004364cba61fced222781b Mon Sep 17 00:00:00 2001
From: juliexxia <juliexxia@google.com>
Date: Tue, 26 Jan 2021 12:26:38 -0800
Subject: [PATCH] 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 https://github.com/bazelbuild/bazel/issues/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 #11128

PiperOrigin-RevId: 353923997
---
 .../StarlarkDefinedConfigTransition.java      |  16 +--
 .../analysis/starlark/StarlarkTransition.java |   6 +-
 ...configurations_external_workspaces_test.sh | 102 +++++++++++++++++-
 3 files changed, 113 insertions(+), 11 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
index 8e7ba90912e32a..099852287f2088 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
+++ b/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;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
index 0dfb587b462a1d..000c0249f039cb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
+++ b/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
diff --git a/src/test/shell/integration/starlark_configurations_external_workspaces_test.sh b/src/test/shell/integration/starlark_configurations_external_workspaces_test.sh
index 6226670040894a..2eef587c768588 100755
--- a/src/test/shell/integration/starlark_configurations_external_workspaces_test.sh
+++ b/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"