Repository: bazelbuild/bazel
Issue: clientEnv is cached for workspace status command
Labels: type: bug
P3
category: misc > misc

Please provide the following information. The more we know about your system and use case, the more easily and likely we can help.

### Description of the problem / feature request / question:
The client environment passed to the BazelWorkspaceStatusModule is cached across runs.
https://github.com/bazelbuild/bazel/blob/d926bc40260549b997a6a5a1e82d9e7999dbb65e/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java#L106

This means we have to do `bazel clean` whenever any stable value changes as a result of change in environment variables.

### If possible, provide a minimal example to reproduce the problem:
File env_status.sh
```
#!/bin/bash
env | sed -e 's/^/STABLE_/' -e 's/\=/ /'
```

File WORKSPACE is empty.

Demonstration:
```
(bazel build --workspace_status_command=./env_status.sh @bazel_tools//tools/zip:zipper 2>&1 && cat bazel-out/stable-status.txt) | grep STABLE_FOO
# Not found; expected.

# Set a new environment variable and check.
export FOO="Checking environment input."
(bazel build --workspace_status_command=./env_status.sh @bazel_tools//tools/zip:zipper 2>&1 && cat bazel-out/stable-status.txt) | grep STABLE_FOO
# Not found; unexpected.

# Clean cache and try again.
bazel clean
(bazel build --workspace_status_command=./env_status.sh @bazel_tools//tools/zip:zipper 2>&1 && cat bazel-out/stable-status.txt) | grep STABLE_FOO
# Found; expected.

unset FOO
(bazel build --workspace_status_command=./env_status.sh @bazel_tools//tools/zip:zipper 2>&1 && cat bazel-out/stable-status.txt) | grep STABLE_FOO
# Found; unexpected.
```

### Environment info
* Operating System:
macOS 10.13.1

* Bazel version (output of `bazel info release`):
release 0.8.0-homebrew

###  Have you found anything relevant by searching the web?
Nothing related to client environment in relation to stable or volatile status.

### Anything else, information or logs or outputs that would be helpful?



Issue comments:
cc @razvanm
@brandjon , not sure if anyone owns this area of Bazel; could you maybe take a look?
I can look in my spare cycles but I won't be able to get to this soon.
Forwarding to dslomov as requested.
BazelWorkspaceStatusAction.equals is incorrect - it doesn't check for equality of the clientEnv.
@ulfjack : thanks for the analysis!
@dslomov : can you fix it in Q1?
I sent a fix (based on @ulfjack 's suggestion) in #4705

Pull request review comments:
add a non-stable key as well, assert that changing it will not trigger a rebuild
Please remove the `.sh` ending here and in other `mktemp` calls.
On macOS `mktemp` requires that the XXXs be at the end of the template, otherwise the command creates a file called literally `wsc-XXXXXXXX.sh`.
Good point - thank you!  Fixed.

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
index 9a74ccc95fa64d..ebd3171ebdf8b8 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -252,15 +252,18 @@ public boolean equals(Object o) {
         return false;
       }
 
+      // We consider clientEnv in equality because we pass it when executing the workspace status command
+
       BazelWorkspaceStatusAction that = (BazelWorkspaceStatusAction) o;
-      return this.stableStatus.equals(that.stableStatus)
+      return this.clientEnv.equals(that.clientEnv)
+          && this.stableStatus.equals(that.stableStatus)
           && this.volatileStatus.equals(that.volatileStatus)
           && this.options.equals(that.options);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(stableStatus, volatileStatus, options);
+      return Objects.hash(clientEnv, stableStatus, volatileStatus, options);
     }
 
     @Override


Test patch:
diff --git a/src/test/shell/bazel/bazel_workspace_status_test.sh b/src/test/shell/bazel/bazel_workspace_status_test.sh
index 9d589efc08fbc4..508dabb9055b68 100755
--- a/src/test/shell/bazel/bazel_workspace_status_test.sh
+++ b/src/test/shell/bazel/bazel_workspace_status_test.sh
@@ -48,7 +48,7 @@ EOF
 function test_workspace_status_parameters() {
   create_new_workspace
 
-  local cmd=$TEST_TMPDIR/status.sh
+  local cmd=`mktemp $TEST_TMPDIR/wsc-XXXXXXXX`
   cat > $cmd <<EOF
 #!/bin/bash
 
@@ -74,7 +74,7 @@ EOF
 function test_workspace_status_cpp() {
   create_new_workspace
 
-  local cmd=$TEST_TMPDIR/status.sh
+  local cmd=`mktemp $TEST_TMPDIR/wsc-XXXXXXXX`
   cat > $cmd <<EOF
 #!/bin/bash
 
@@ -135,13 +135,14 @@ EOF
 
 function test_stable_and_volatile_status() {
   create_new_workspace
-  cat >$TEST_TMPDIR/wsc.sh <<EOF
+  local wsc=`mktemp $TEST_TMPDIR/wsc-XXXXXXXX`
+  cat >$wsc <<EOF
 #!/bin/bash
 
 cat $TEST_TMPDIR/status
 EOF
 
-  chmod +x $TEST_TMPDIR/wsc.sh
+  chmod +x $wsc
 
   cat > BUILD <<'EOF'
 genrule(
@@ -157,7 +158,7 @@ STABLE_NAME alice
 NUMBER 1
 EOF
 
-  bazel build --workspace_status_command=$TEST_TMPDIR/wsc.sh --stamp //:a || fail "build failed"
+  bazel build --workspace_status_command=$wsc --stamp //:a || fail "build failed"
   assert_contains "STABLE_NAME alice" bazel-genfiles/ao
   assert_contains "NUMBER 1" bazel-genfiles/ao
 
@@ -168,7 +169,7 @@ NUMBER 2
 EOF
 
   # Changes to volatile fields should not result in a rebuild
-  bazel build --workspace_status_command=$TEST_TMPDIR/wsc.sh --stamp //:a || fail "build failed"
+  bazel build --workspace_status_command=$wsc --stamp //:a || fail "build failed"
   assert_contains "STABLE_NAME alice" bazel-genfiles/ao
   assert_contains "NUMBER 1" bazel-genfiles/ao
 
@@ -178,10 +179,54 @@ NUMBER 3
 EOF
 
   # Changes to stable fields should result in a rebuild
-  bazel build --workspace_status_command=$TEST_TMPDIR/wsc.sh --stamp //:a || fail "build failed"
+  bazel build --workspace_status_command=$wsc --stamp //:a || fail "build failed"
   assert_contains "STABLE_NAME bob" bazel-genfiles/ao
   assert_contains "NUMBER 3" bazel-genfiles/ao
 
 }
 
+function test_env_var_in_workspace_status() {
+  create_new_workspace
+  local wsc=`mktemp $TEST_TMPDIR/wsc-XXXXXXXX`
+  cat >$wsc <<'EOF'
+#!/bin/bash
+
+echo "STABLE_ENV" ${STABLE_VAR}
+echo "VOLATILE_ENV" ${VOLATILE_VAR}
+EOF
+
+  chmod +x $wsc
+
+  cat > BUILD <<'EOF'
+genrule(
+    name = "a",
+    srcs = [],
+    outs = ["ao"],
+    cmd="(echo volatile; cat bazel-out/volatile-status.txt; echo; echo stable; cat bazel-out/stable-status.txt; echo) > $@",
+    stamp=1)
+EOF
+
+  STABLE_VAR=alice VOLATILE_VAR=one bazel build --workspace_status_command=$wsc --stamp //:a || fail "build failed"
+  assert_contains "STABLE_ENV alice" bazel-out/stable-status.txt
+  assert_contains "VOLATILE_ENV one" bazel-out/volatile-status.txt
+  assert_contains "STABLE_ENV alice" bazel-genfiles/ao
+  assert_contains "VOLATILE_ENV one" bazel-genfiles/ao
+
+  # Changes to the env var should be reflected into the stable-status file, and thus trigger a rebuild
+  STABLE_VAR=bob VOLATILE_VAR=two bazel build --workspace_status_command=$wsc --stamp //:a || fail "build failed"
+  assert_contains "STABLE_ENV bob" bazel-out/stable-status.txt
+  assert_contains "VOLATILE_ENV two" bazel-out/volatile-status.txt
+  assert_contains "STABLE_ENV bob" bazel-genfiles/ao
+  assert_contains "VOLATILE_ENV two" bazel-genfiles/ao
+
+  # Changes to volatile fields should not result in a rebuild (but should update the stable & volatile status files)
+  STABLE_VAR=bob VOLATILE_VAR=three bazel build --workspace_status_command=$wsc --stamp //:a || fail "build failed"
+  assert_contains "STABLE_ENV bob" bazel-out/stable-status.txt
+  assert_contains "VOLATILE_ENV three" bazel-out/volatile-status.txt
+  # We did not rebuild, so the output remains at the previous values
+  assert_contains "STABLE_ENV bob" bazel-genfiles/ao
+  assert_contains "VOLATILE_ENV two" bazel-genfiles/ao
+
+}
+
 run_suite "workspace status tests"