Repository: bazelbuild/bazel
Issue: Bazel 6.0.0rc4 breaks Bazel RBE tests
Labels: type: bug
P1

This was first detected in the downstream test for Bazel 6.0.0rc4: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2761#0184d3b5-3cf0-410a-984d-2fdfdf78a49b

And verified in https://buildkite.com/bazel/bazel-bazel/builds/21471#0184e876-6406-4909-9a81-e8dc957b471f


[Integration tests with test args](https://cs.opensource.google/search?q=%22args%20%3D%20%5B%22$(JAVABASE)%22%5D%22&sq=&ss=bazel) are failing remotely with test logs look like:
```
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //src/test/shell/bazel:bazel_example_test
-----------------------------------------------------------------------------
WARNING: Passing test names in arguments (--test_arg) is deprecated, please use --test_filter='/usr/lib/jvm/java-11-openjdk-amd64' instead.
WARNING: Arguments do not specify tests!
INFO[bazel_example_test 2022-12-06 17:28:07 (+0000)] bazel binary is at /b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles/io_bazel/src/test/shell/bin
INFO[bazel_example_test 2022-12-06 17:28:07 (+0000)] setting up client in /b/f/w/_tmp/55e5a95f98bdfb926d4a13d2054933c5/workspace
```



Issue comments:
This issue also happens with Bazel@HEAD: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2766#0184e535-8df3-4114-9f65-8777f5bc7fe4, but the Bazel Auto Sheriff points to a [commit](https://github.com/bazelbuild/bazel/commit/fb2324620a4adee19bb4e8a030b6fba58f672dc0) that's not in Bazel 6.0.0rc4: https://buildkite.com/bazel/bazel-auto-sheriff-face-with-cowboy-hat/builds/1118

**Update**: culprit finder failed due to not being able to download the Bazel binary.
```
:bazel: Test with Bazel built at fb2324620a4adee19bb4e8a030b6fba58f672dc0
:googlecloud: Downloading Bazel built at fb2324620a4adee19bb4e8a030b6fba58f672dc0
 
gsutil cp gs://bazel-builds/artifacts/centos7/fb2324620a4adee19bb4e8a030b6fba58f672dc0/bazel /tmp/tmp09usce0c/bazel
CommandException: No URLs matched: gs://bazel-builds/artifacts/centos7/fb2324620a4adee19bb4e8a030b6fba58f672dc0/bazel
Failed to download Bazel binary at fb2324620a4adee19bb4e8a030b6fba58f672dc0, error message:
Command '['gsutil', 'cp', 'gs://bazel-builds/artifacts/centos7/fb2324620a4adee19bb4e8a030b6fba58f672dc0/bazel', '/tmp/tmp09usce0c/bazel']' returned non-zero exit status 1.

```

We should fix culprit finder

/cc @coeuvre Since this only happens with RBE enabled. https://github.com/bazelbuild/bazel/blob/2a88c8da7783490ef6192336b091910cd662cc67/.bazelci/postsubmit.yml#L266
@coeuvre <del>Is https://github.com/bazelbuild/bazel/pull/16749 the possible culprit?</del> probably not since the original change was submitted in Oct, we have only been seen the failure with Bazel@HEAD in Nov.
Last green: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2729
First fail: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2730

Culprit should fall into (6e945e8ab422ca0a195d09899cce61ba6c0cfa59, 1500195986dc5095a21f32633cb4e3aabe037f1e]
Bisecting the rc4 branch at https://buildkite.com/bazel/culprit-finder/builds/3876 and it points to https://github.com/bazelbuild/bazel/commit/0b645254b41edc738c6641fd192fca86203ff2e2
Bisecting again on master branch after fixing the CI issue: https://buildkite.com/bazel/culprit-finder/builds/3877
While waiting for the bisect result, I can also reproduce at https://github.com/bazelbuild/bazel/commit/839ce7f5c40240d8b6f49c416c3769e226f43fee (original commit of the 6.0.0 cherrypick https://github.com/bazelbuild/bazel/commit/0b645254b41edc738c6641fd192fca86203ff2e2)
```
USE_BAZEL_VERSION=839ce7f5c40240d8b6f49c416c3769e226f43fee bazel test //src/test/shell/bazel:bazel_example_test --config=ubuntu1804_java11 --remote_executor=grpcs://remotebuildexecution.googleapis.com
```
But not at it's parent commit.

/cc @fmeum @Wyverald 
@meteorcloudy Could you run these tests with `--test_env=RUNFILES_LIB_DEBUG=1`? That should provide us with very helpful information.
@fmeum Here is the test log:
```
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //src/test/shell/bazel:bazel_example_test
-----------------------------------------------------------------------------
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/integration_test_setup.sh): start
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/integration_test_setup.sh): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/integration_test_setup.sh): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/unittest.bash): start
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/unittest.bash): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/unittest.bash): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
WARNING: Passing test names in arguments (--test_arg) is deprecated, please use --test_filter='/usr/lib/jvm/java-11-openjdk-amd64' instead.
WARNING: Arguments do not specify tests!
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/testenv.sh): start
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/testenv.sh): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/testenv.sh): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
INFO[runfiles.bash]: rlocation(io_bazel/src/bazel): start
INFO[runfiles.bash]: rlocation(io_bazel/src/bazel): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/src/bazel): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/bin/bazel): start
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/bin/bazel): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/bin/bazel): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
INFO[runfiles.bash]: rlocation(io_bazel/tools/BUILD): start
INFO[runfiles.bash]: rlocation(io_bazel/tools/BUILD): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/tools/BUILD): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/bazel/testing_server.py): start
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/bazel/testing_server.py): not using repository mapping () since it does not exist
INFO[runfiles.bash]: rlocation(io_bazel/src/test/shell/bazel/testing_server.py): found under RUNFILES_DIR (/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles), return
INFO[bazel_example_test 2022-12-07 09:02:51 (+0000)] bazel binary is at /b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles/io_bazel/src/test/shell/bin
INFO[bazel_example_test 2022-12-07 09:02:51 (+0000)] setting up client in /b/f/w/_tmp/bfbf694ed20a8c587876e28f8f9820b1/workspace
INFO[runfiles.bash]: rlocation(/usr/lib/jvm/java-11-openjdk-amd64/bin/java): start
INFO[runfiles.bash]: rlocation(/usr/lib/jvm/java-11-openjdk-amd64/bin/java): absolute path, return
INFO[runfiles.bash]: rlocation(/usr/lib/jvm/java-11-openjdk-amd64/bin/java): not using repository mapping () since it does not exist
ERROR[runfiles.bash]: cannot look up runfile "/usr/lib/jvm/java-11-openjdk-amd64/bin/java"  (RUNFILES_DIR="/b/f/w/bazel-out/k8-fastbuild/bin/src/test/shell/bazel/bazel_example_test.runfiles", RUNFILES_MANIFEST_FILE="")
```
@coeuvre I suspected a different issue, so I would like to confirm that isn't a problem either: The Bash runfiles library currently relies on absolute (!) paths of generated files containing an `execroot` segment. Is that guaranteed to be the case with remote execution, just like it is with local execution?
The logic for absolute paths passed to `rlocation` broke. I am investigating why the existing test didn't catch that.
> @coeuvre I suspected a different issue, so I would like to confirm that isn't a problem either: The Bash runfiles library currently relies on absolute (!) paths of generated files containing an `execroot` segment. Is that guaranteed to be the case with remote execution, just like it is with local execution?

My gut feeling is that `execroot` is not included in the path when actions inspect the path inside remote worker but I may be wrong.  We use `execroot` as input root when uploading inputs for actions. Remote worker is free to move the input root to any local directories so a file `/.../execroot/bazel-out/a` in bazel host is probably become `/..../bazel-out/a` in remote worker.
@coeuvre Thanks, I will remove that assumption in a second PR.

@meteorcloudy The Bash runfiles test didn't cover the exit codes of `rlocation`, only whatever it printed, and thus missed some corner cases. I have reworked the tests to check the exit codes and am now fixing these cases.
@fmeum @coeuvre Thanks!!
I submitted two PRs with fixes, #16945 and #16946.
I think I know how to fix the RBE test failures, but now that I studied this more closely, there is some underlying inconsistent behavior that makes the library difficult to test and potentially also hard to use: `$(rlocation does/not/exist)` has exit code 0 if and only if the runfiles manifest exists. This means that the behavior differs between local and remote execution on Windows, which is probably why we only noticed this in downstream tests.

@laszlocsomor Sorry for the sideline mention, but can you recall how you intended the exit code of Bash `$(rlocation ...)` to behave?
> `$(rlocationpath does/not/exist)` has exit code 0 if and only if the runfiles manifest exists.

Did you mean `$(rlocation does/not/exist)`?
@Wyverald Yes, sorry, fixed.
Fixing this inconsistency isn't required to get the tests to pass, but if we want to do it, 6.0.0 is probably the right time. Since `$(failing_command)` doesn't usually fail the outer shell script, having `$(rlocation does/not/exist)` exit with exit code 1 in all cases doesn't seem to be *that* breaking.

@Wyverald @meteorcloudy What do you think?

Pull request review comments:

Patch that resolved the issue:
diff --git a/tools/bash/runfiles/runfiles.bash b/tools/bash/runfiles/runfiles.bash
index 72f85d92563a00..29706dfce6a704 100644
--- a/tools/bash/runfiles/runfiles.bash
+++ b/tools/bash/runfiles/runfiles.bash
@@ -260,7 +260,7 @@ function runfiles_current_repository() {
       # The only shell script that is not executed from the runfiles directory (if it is populated)
       # is the sh_binary entrypoint. Parse its path under the execroot, using the last match to
       # allow for nested execroots (e.g. in Bazel integration tests).
-      local -r repository=$(echo "$normalized_caller_path" | __runfiles_maybe_grep -E -o '/execroot/[^/]+/bazel-out/[^/]+/bin/external/[^/]+/' | tail -1 | rev | cut -d / -f 2 | rev)
+      local -r repository=$(echo "$normalized_caller_path" | __runfiles_maybe_grep -E -o '/bazel-out/[^/]+/bin/external/[^/]+/' | tail -1 | rev | cut -d / -f 2 | rev)
       if [[ -n "$repository" ]]; then
         if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
           echo >&2 "INFO[runfiles.bash]: runfiles_current_repository($idx): ($normalized_caller_path) lies in repository ($repository)"


Test patch: