Correctly report stderr content for repository_ctx.execute

The shell.Command class throw an exception when exit code is non
null, resulting in unwanted case where calling a failing command
as just echoing "Exited with status 4".

--
MOS_MIGRATED_REVID=116361594

2 files changed, 33 insertions, 7 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java
index 934bc6ce02..9677439705 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.bazel.repository.skylark;
 
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.shell.BadExitStatusException;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.shell.CommandException;
 import com.google.devtools.build.lib.shell.CommandResult;
@@ -45,6 +46,15 @@ public class SkylarkExecutionResult {
     this.stderr = stderr;
   }
 
+  private SkylarkExecutionResult(CommandResult result) {
+    // TODO(dmarting): if a lot of data is sent to stdout, this will use all the memory and
+    // Bazel will crash. Maybe we should use custom output streams that throw an appropriate
+    // exception when reaching a specific size.
+    this.stdout = new String(result.getStdout(), StandardCharsets.UTF_8);
+    this.stderr = new String(result.getStderr(), StandardCharsets.UTF_8);
+    this.returnCode = result.getTerminationStatus().getExitCode();
+  }
+
   @SkylarkCallable(
     name = "return_code",
     structField = true,
@@ -89,13 +99,9 @@ public class SkylarkExecutionResult {
         argsArray[i] = arg.toString();
       }
       CommandResult result = new Command(argsArray).execute(new byte[] {}, timeout, false);
-      // TODO(dmarting): if a lot of data is sent to stdout, this will use all the memory and
-      // Bazel will crash. Maybe we should use custom output streams that throw an appropriate
-      // exception when reaching a specific size.
-      String stdout = new String(result.getStdout(), StandardCharsets.UTF_8);
-      String stderr = new String(result.getStderr(), StandardCharsets.UTF_8);
-      return new SkylarkExecutionResult(
-          result.getTerminationStatus().getExitCode(), stdout, stderr);
+      return new SkylarkExecutionResult(result);
+    } catch (BadExitStatusException e) {
+      return new SkylarkExecutionResult(e.getResult());
     } catch (CommandException e) {
       return new SkylarkExecutionResult(256, "", e.getMessage());
     }
diff --git a/src/test/shell/bazel/skylark_repository_test.sh b/src/test/shell/bazel/skylark_repository_test.sh
index 7c6cb7b4ee..8207c5ccc6 100755
--- a/src/test/shell/bazel/skylark_repository_test.sh
+++ b/src/test/shell/bazel/skylark_repository_test.sh
@@ -299,6 +299,26 @@ EOF
   expect_log "version"
 }
 
+function test_skylark_repository_execute_stderr() {
+  setup_skylark_repository
+
+  cat >test.bzl <<EOF
+def _impl(ctx):
+  result = ctx.execute([str(ctx.which("bash")), "-c", "echo erf >&2; exit 1"])
+  if result.return_code != 1:
+    fail("Incorrect return code from bash (should be 1): " + result.return_code)
+  if result.stdout != "":
+    fail("Non-empty output: %s (stderr was %s)" % (result.stdout, result.stderr))
+  print(result.stderr)
+  # Symlink so a repository is created
+  ctx.symlink(ctx.path("$repo2"), ctx.path(""))
+repo = repository_rule(implementation=_impl, local=True)
+EOF
+
+  bazel build @foo//:bar >& $TEST_log || fail "Failed to build"
+  expect_log "erf"
+}
+
 function test_skylark_repository_environ() {
   setup_skylark_repository