Release 5.3.0 (#16074)

* Updated Codeowners file (#16032) * Propagate the error message when a credential helper fails. (#16030) Closes #16012. PiperOrigin-RevId: 464732834 Change-Id: If51ce914098ff17ffe23fa4614947e7f4a5088dc Co-authored-by: Tiago Quelhas <tjgq@google.com> * Migrate legacy desugar wrapper to new rlocation() (#16025) RELNOTES: Fix for desugaring failure on Bazel+Android+Windows build scenario. PiperOrigin-RevId: 464654335 Change-Id: I845093b134dc68ae541603008fe7fee701c7451c Co-authored-by: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com> * Correctly report errors thrown by CommandLinePathFactory#create. (#16036) This method is called while initializing the remote module [1]. Any exceptions not derived from java.io.IOException cause a silent server crash. [1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba Closes #16022. PiperOrigin-RevId: 464997904 Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9 Co-authored-by: Tiago Quelhas <tjgq@google.com> * Fix an issue that `incompatible_remote_build_event_upload_respect_no_… (#16045) * Fix an issue that `incompatible_remote_build_event_upload_respect_no_… …cache` doesn't work with BwtB. The root cause is we use `Path` as key to check which files are omitted, but it also compares the underlying filesystem. When BwtB is enabled, the filesystem for the file is different so we failed to mark the file as omitted. This change fixes that by using `PathFragment` as key. Fixes #15527. Closes #16023. PiperOrigin-RevId: 465284879 Change-Id: I49bbd7a6055e0f234b4ac86a224886bd85797f71 * Update ByteStreamBuildEventArtifactUploader changes Removing the import as it is throwing the error Co-authored-by: Chi Wang <chiwang@google.com> Co-authored-by: Tiago Quelhas <tjgq@google.com> Co-authored-by: Ted <tedx@google.com> Co-authored-by: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com> Co-authored-by: Chi Wang <chiwang@google.com>
bazelbuild · Aug 9, 2022 · 2077c54 · 2077c54
1 parent f440f8e
commit 2077c54
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 77 deletions.
diff --git a/CODEOWNERS b/CODEOWNERS
@@ -1,2 +1 @@
-* @ckolli5
 * @kshyanashree
diff --git a/...main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java b/...main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java
@@ -77,15 +77,15 @@ public GetCredentialsResponse getCredentials(CredentialHelperEnvironment environ
 
       process.waitFor();
       if (process.timedout()) {
-        throw new IOException(
+        throw new CredentialHelperException(
             String.format(
                 Locale.US,
                 "Failed to get credentials for '%s' from helper '%s': process timed out",
                 uri,
                 path));
       }
       if (process.exitValue() != 0) {
-        throw new IOException(
+        throw new CredentialHelperException(
             String.format(
                 Locale.US,
                 "Failed to get credentials for '%s' from helper '%s': process exited with code %d."
@@ -99,7 +99,7 @@ public GetCredentialsResponse getCredentials(CredentialHelperEnvironment environ
       try {
         GetCredentialsResponse response = GSON.fromJson(stdout, GetCredentialsResponse.class);
         if (response == null) {
-          throw new IOException(
+          throw new CredentialHelperException(
               String.format(
                   Locale.US,
                   "Failed to get credentials for '%s' from helper '%s': process exited without"
@@ -110,7 +110,7 @@ public GetCredentialsResponse getCredentials(CredentialHelperEnvironment environ
         }
         return response;
       } catch (JsonSyntaxException e) {
-        throw new IOException(
+        throw new CredentialHelperException(
             String.format(
                 Locale.US,
                 "Failed to get credentials for '%s' from helper '%s': error parsing output. stderr:"

diff --git a/.../com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperException.java b/.../com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperException.java
@@ -0,0 +1,28 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.authandtls.credentialhelper;
+
+import java.io.IOException;
+
+/** An {@link Exception} thrown while invoking a credential helper. */
+public class CredentialHelperException extends IOException {
+  public CredentialHelperException(String message) {
+    super(message);
+  }
+
+  public CredentialHelperException(String message, Throwable cause) {
+    super(message, cause);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
@@ -34,6 +34,7 @@
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import io.netty.util.AbstractReferenceCounted;
 import io.netty.util.ReferenceCounted;
 import io.reactivex.rxjava3.core.Flowable;
@@ -67,8 +68,8 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   private final AtomicBoolean shutdown = new AtomicBoolean();
   private final Scheduler scheduler;
 
-  private final Set<Path> omittedFiles = Sets.newConcurrentHashSet();
-  private final Set<Path> omittedTreeRoots = Sets.newConcurrentHashSet();
+  private final Set<PathFragment> omittedFiles = Sets.newConcurrentHashSet();
+  private final Set<PathFragment> omittedTreeRoots = Sets.newConcurrentHashSet();
 
   ByteStreamBuildEventArtifactUploader(
       Executor executor,
@@ -89,11 +90,11 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   }
 
   public void omitFile(Path file) {
-    omittedFiles.add(file);
+    omittedFiles.add(file.asFragment());
   }
 
   public void omitTree(Path treeRoot) {
-    omittedTreeRoots.add(treeRoot);
+    omittedTreeRoots.add(treeRoot.asFragment());
   }
 
   /** Returns {@code true} if Bazel knows that the file is stored on a remote system. */
@@ -153,13 +154,14 @@ private PathMetadata readPathMetadata(Path file) throws IOException {
           /* omitted= */ false);
     }
 
+    PathFragment filePathFragment = file.asFragment();
     boolean omitted = false;
-    if (omittedFiles.contains(file)) {
+    if (omittedFiles.contains(filePathFragment)) {
       omitted = true;
     } else {
-      for (Path treeRoot : omittedTreeRoots) {
+      for (PathFragment treeRoot : omittedTreeRoots) {
         if (file.startsWith(treeRoot)) {
-          omittedFiles.add(file);
+          omittedFiles.add(filePathFragment);
           omitted = true;
         }
       }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/BUILD b/src/main/java/com/google/devtools/build/lib/remote/util/BUILD
@@ -19,6 +19,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
+        "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/remote:ExecutionStatusException",
         "//src/main/java/com/google/devtools/build/lib/remote/common",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java
@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.authandtls.CallCredentialsProvider;
+import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperException;
 import com.google.devtools.build.lib.remote.ExecutionStatusException;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.OutputDigestMismatchException;
@@ -367,6 +368,7 @@ private static String executionStatusExceptionErrorMessage(ExecutionStatusExcept
   public static String grpcAwareErrorMessage(IOException e) {
     io.grpc.Status errStatus = io.grpc.Status.fromThrowable(e);
     if (e.getCause() instanceof ExecutionStatusException) {
+      // Display error message returned by the remote service.
       try {
         return "Remote Execution Failure:\n"
             + executionStatusExceptionErrorMessage((ExecutionStatusException) e.getCause());
@@ -378,9 +380,20 @@ public static String grpcAwareErrorMessage(IOException e) {
       }
     }
     if (!errStatus.getCode().equals(io.grpc.Status.UNKNOWN.getCode())) {
-      // If the error originated in the gRPC library then display it as "STATUS: error message"
-      // to the user
-      return String.format("%s: %s", errStatus.getCode().name(), errStatus.getDescription());
+      // Display error message returned by the gRPC library, prefixed by the status code.
+      StringBuilder sb = new StringBuilder();
+      sb.append(errStatus.getCode().name());
+      sb.append(": ");
+      sb.append(errStatus.getDescription());
+      // If the error originated from a credential helper, print additional debugging information.
+      for (Throwable t = errStatus.getCause(); t != null; t = t.getCause()) {
+        if (t instanceof CredentialHelperException) {
+          sb.append(": ");
+          sb.append(t.getMessage());
+          break;
+        }
+      }
+      return sb.toString();
     }
     return e.getMessage();
   }

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java
@@ -39,7 +39,14 @@
  * (e.g., {@code %workspace%/foo} becomes {@code </path/to/workspace>/foo}).
  */
 public final class CommandLinePathFactory {
-  private static final Pattern REPLACEMENT_PATTERN = Pattern.compile("^(%([a-z_]+)%/)?([^%].*)$");
+  /** An exception thrown while attempting to resolve a path. */
+  public static class CommandLinePathFactoryException extends IOException {
+    public CommandLinePathFactoryException(String message) {
+      super(message);
+    }
+  }
+
+  private static final Pattern REPLACEMENT_PATTERN = Pattern.compile("^(%([a-z_]+)%/+)?([^%].*)$");
 
   private static final Splitter PATH_SPLITTER = Splitter.on(File.pathSeparator);
 
@@ -78,20 +85,17 @@ public Path create(Map<String, String> env, String value) throws IOException {
     String rootName = matcher.group(2);
     PathFragment path = PathFragment.create(matcher.group(3));
     if (path.containsUplevelReferences()) {
-      throw new IllegalArgumentException(
+      throw new CommandLinePathFactoryException(
           String.format(
               Locale.US, "Path must not contain any uplevel references ('..'), got '%s'", value));
     }
 
     // Case 1: `path` is relative to a well-known root.
     if (!Strings.isNullOrEmpty(rootName)) {
-      // The regex above cannot check that `value` is not of form `%foo%//abc` (group 2 will be
-      // `foo` and group 3 will be `/abc`).
-      Preconditions.checkArgument(!path.isAbsolute());
-
       Path root = roots.get(rootName);
       if (root == null) {
-        throw new IllegalArgumentException(String.format(Locale.US, "Unknown root %s", rootName));
+        throw new CommandLinePathFactoryException(
+            String.format(Locale.US, "Unknown root %s", rootName));
       }
       return root.getRelative(path);
     }
@@ -108,7 +112,7 @@ public Path create(Map<String, String> env, String value) throws IOException {
     // flag is from?), we only allow relative paths with a single segment (i.e., no `/`) and treat
     // it as relative to the user's `PATH`.
     if (path.segmentCount() > 1) {
-      throw new IllegalArgumentException(
+      throw new CommandLinePathFactoryException(
           "Path must either be absolute or not contain any path separators");
     }
 

diff --git a/.../java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperTest.java b/.../java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperTest.java
@@ -28,7 +28,6 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.build.runfiles.Runfiles;
-import java.io.IOException;
 import java.net.URI;
 import java.time.Duration;
 import org.junit.Test;
@@ -95,18 +94,20 @@ public void knownUriWithMultipleHeaders() throws Exception {
 
   @Test
   public void unknownUri() {
-    IOException ioException =
+    CredentialHelperException e =
         assertThrows(
-            IOException.class, () -> getCredentialsFromHelper("https://unknown.example.com"));
-    assertThat(ioException).hasMessageThat().contains("Unknown uri 'https://unknown.example.com'");
+            CredentialHelperException.class,
+            () -> getCredentialsFromHelper("https://unknown.example.com"));
+    assertThat(e).hasMessageThat().contains("Unknown uri 'https://unknown.example.com'");
   }
 
   @Test
   public void credentialHelperOutputsNothing() throws Exception {
-    IOException ioException =
+    CredentialHelperException e =
         assertThrows(
-            IOException.class, () -> getCredentialsFromHelper("https://printnothing.example.com"));
-    assertThat(ioException).hasMessageThat().contains("exited without output");
+            CredentialHelperException.class,
+            () -> getCredentialsFromHelper("https://printnothing.example.com"));
+    assertThat(e).hasMessageThat().contains("exited without output");
   }
 
   @Test
@@ -135,9 +136,10 @@ public void helperGetEnvironment() throws Exception {
 
   @Test
   public void helperTimeout() throws Exception {
-    IOException ioException =
+    CredentialHelperException e =
         assertThrows(
-            IOException.class, () -> getCredentialsFromHelper("https://timeout.example.com"));
-    assertThat(ioException).hasMessageThat().contains("process timed out");
+            CredentialHelperException.class,
+            () -> getCredentialsFromHelper("https://timeout.example.com"));
+    assertThat(e).hasMessageThat().contains("process timed out");
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java b/src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java
@@ -19,6 +19,7 @@
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.runtime.CommandLinePathFactory.CommandLinePathFactoryException;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
@@ -99,6 +100,9 @@ public void createWithNamedRoot() throws Exception {
         .isEqualTo(filesystem.getPath("/path/to/output/base/foo"));
     assertThat(factory.create(ImmutableMap.of(), "%output_base%/foo/bar"))
         .isEqualTo(filesystem.getPath("/path/to/output/base/foo/bar"));
+
+    assertThat(factory.create(ImmutableMap.of(), "%workspace%//foo//bar"))
+        .isEqualTo(filesystem.getPath("/path/to/workspace/foo/bar"));
   }
 
   @Test
@@ -108,9 +112,11 @@ public void pathLeakingOutsideOfRoot() {
             filesystem, ImmutableMap.of("a", filesystem.getPath("/path/to/a")));
 
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%a%/../foo"));
+        CommandLinePathFactoryException.class,
+        () -> factory.create(ImmutableMap.of(), "%a%/../foo"));
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%a%/b/../.."));
+        CommandLinePathFactoryException.class,
+        () -> factory.create(ImmutableMap.of(), "%a%/b/../.."));
   }
 
   @Test
@@ -120,29 +126,21 @@ public void unknownRoot() {
             filesystem, ImmutableMap.of("a", filesystem.getPath("/path/to/a")));
 
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%workspace%/foo"));
+        CommandLinePathFactoryException.class,
+        () -> factory.create(ImmutableMap.of(), "%workspace%/foo"));
     assertThrows(
-        IllegalArgumentException.class,
+        CommandLinePathFactoryException.class,
         () -> factory.create(ImmutableMap.of(), "%output_base%/foo"));
   }
 
-  @Test
-  public void rootWithDoubleSlash() {
-    CommandLinePathFactory factory =
-        new CommandLinePathFactory(
-            filesystem, ImmutableMap.of("a", filesystem.getPath("/path/to/a")));
-
-    assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%a%//foo"));
-  }
-
   @Test
   public void relativePathWithMultipleSegments() {
     CommandLinePathFactory factory = new CommandLinePathFactory(filesystem, ImmutableMap.of());
 
-    assertThrows(IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "a/b"));
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "a/b/c/d"));
+        CommandLinePathFactoryException.class, () -> factory.create(ImmutableMap.of(), "a/b"));
+    assertThrows(
+        CommandLinePathFactoryException.class, () -> factory.create(ImmutableMap.of(), "a/b/c/d"));
   }
 
   @Test

diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3536,6 +3536,29 @@ EOF
   expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
 }
 
+function test_uploader_respect_no_cache_minimal() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\" > \$@",
+  tags = ["no-cache"],
+)
+EOF
+
+  bazel build \
+      --remote_cache=grpc://localhost:${worker_port} \
+      --remote_download_minimal \
+      --incompatible_remote_build_event_upload_respect_no_cache \
+      --build_event_json_file=bep.json \
+      //a:foo >& $TEST_log || fail "Failed to build"
+
+  cat bep.json > $TEST_log
+  expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
+  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+}
+
 function test_uploader_alias_action_respect_no_cache() {
   mkdir -p a
   cat > a/BUILD <<EOF

diff --git a/tools/android/BUILD.tools b/tools/android/BUILD.tools
@@ -118,6 +118,7 @@ sh_binary(
     name = "desugar_java8",
     srcs = ["desugar.sh"],
     data = ["//src/tools/android/java/com/google/devtools/build/android/desugar:Desugar"],
+    deps = ["@bazel_tools//tools/bash/runfiles"],
 )
 
 alias(