From 3ebf658cba43bbab1efc36518f0795a7d65e2d46 Mon Sep 17 00:00:00 2001
From: Ed Schouten <eschouten@apple.com>
Date: Fri, 12 Mar 2021 06:18:17 -0800
Subject: [PATCH] Prevent a crash when using --repo_env=VAR without a value

The --repo_env option is documented in the same way as --action_env. In
addition to allowing you to set explicit values, you can also use it to
explicitly pick values from the environment in which Bazel was invoked.
Unfortunately, this causes a null pointer exception in Starlark due to a
null string stored as a map value.

This change extends the logic of converting --repo_env to a map to take
null values into account. When null, the value is loaded from the
current environment. This behaviour is useful in case you want to do
something like this:

--incompatible_strict_action_env --action_env=PATH=... --repo_env=PATH

This allows you to run build actions with a strict value for $PATH (e.g.,
to get a decent action cache hit rate in case of remote execution),
while still allowing repository_ctx.which() to find tools on the host
system using $PATH.

Closes #12886.

PiperOrigin-RevId: 362506900
---
 .../build/lib/runtime/CommandEnvironment.java       |  9 ++++++++-
 src/test/shell/bazel/starlark_repository_test.sh    | 13 +++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index c87d85d969f836..a2e1d150c839e7 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -248,7 +248,14 @@ public void exit(AbruptExitException exception) {
     CoreOptions configOpts = options.getOptions(CoreOptions.class);
     if (configOpts != null) {
       for (Map.Entry<String, String> entry : configOpts.repositoryEnvironment) {
-        repoEnv.put(entry.getKey(), entry.getValue());
+        String name = entry.getKey();
+        String value = entry.getValue();
+        if (value == null) {
+          value = System.getenv(name);
+        }
+        if (value != null) {
+          repoEnv.put(name, value);
+        }
       }
     }
   }
diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
index 0fbbeae8f99abb..e5ef96782993e4 100755
--- a/src/test/shell/bazel/starlark_repository_test.sh
+++ b/src/test/shell/bazel/starlark_repository_test.sh
@@ -889,6 +889,7 @@ EOF
   cat > .bazelrc <<EOF
 build:foo --repo_env=FOO=foo
 build:bar --repo_env=FOO=bar
+build:qux --repo_env=FOO
 EOF
 
   bazel build --config=foo //:repoenv //:unrelated
@@ -924,6 +925,18 @@ EOF
       || fail "Expected FOO to be visible to repo rules"
   diff unrelated1.txt unrelated3.txt \
       || fail "Expected unrelated action to not be rerun"
+
+  FOO=qux bazel build --config=qux //:repoenv //:unrelated
+  # The new config should be picked up, but the unrelated target should
+  # not be rerun
+  cp `bazel info bazel-genfiles 3>/dev/null`/repoenv.txt repoenv4.txt
+  cp `bazel info bazel-genfiles 3> /dev/null`/unrelated.txt unrelated4.txt
+  echo; cat repoenv4.txt; echo; cat unrelated4.txt; echo
+
+  grep -q 'FOO=qux' repoenv4.txt \
+      || fail "Expected FOO to be visible to repo rules"
+  diff unrelated1.txt unrelated4.txt \
+      || fail "Expected unrelated action to not be rerun"
 }
 
 function test_repo_env_inverse() {