Prevent a crash when using --repo_env=VAR without a value

EdSchouten · copybara-github · commit 3ebf658cba43 · 2021-03-12T06:19:43.000-08:00
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
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
@@ -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
@@ -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() {

Original file line number	Diff line number	Diff line change
`@@ -248,7 +248,14 @@ public void exit(AbruptExitException exception) {`
`248`	`248`	`CoreOptions configOpts = options.getOptions(CoreOptions.class);`
`249`	`249`	`if (configOpts != null) {`
`250`	`250`	`for (Map.Entry<String, String> entry : configOpts.repositoryEnvironment) {`
`251`		`- repoEnv.put(entry.getKey(), entry.getValue());`
	`251`	`+ String name = entry.getKey();`
	`252`	`+ String value = entry.getValue();`
	`253`	`+ if (value == null) {`
	`254`	`+ value = System.getenv(name);`
	`255`	`+ }`
	`256`	`+ if (value != null) {`
	`257`	`+ repoEnv.put(name, value);`
	`258`	`+ }`
`252`	`259`	`}`
`253`	`260`	`}`
`254`	`261`	`}`