remote: enable bes for --experimental_remote_download_outputs=(minima…

…l|toplevel)

--experimental_remote_download_outputs=(minimal|toplevel)

This is the last patch to enable BEP/BES file URI rewriting when being
used with --experimental_remote_download_outputs=(minimal|toplevel).

Closes #8669.

PiperOrigin-RevId: 253981837

src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java

@@ -68,6 +68,11 @@ class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader
             Executors.newFixedThreadPool(Math.min(maxUploadThreads, 1000)));
   }
 
+  private static boolean isRemoteFile(Path file) {
+    return file.getFileSystem() instanceof RemoteActionFileSystem
+        && ((RemoteActionFileSystem) file.getFileSystem()).isRemote(file);
+  }
+
   @Override
   public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
     if (files.isEmpty()) {
@@ -86,6 +91,9 @@ public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
                 }
                 DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction());
                 Digest digest = digestUtil.compute(file);
+                if (isRemoteFile(file)) {
+                  return Futures.immediateFuture(new PathDigestPair(file, digest));
+                }
                 Chunker chunker = Chunker.builder().setInput(digest.getSizeBytes(), file).build();
                 final ListenableFuture<Void> upload;
                 Context prevCtx = ctx.attach();

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -40,14 +40,14 @@
  *
  * <p>This implementation only supports creating local action outputs.
  */
-public class RemoteActionFileSystem extends DelegateFileSystem {
+class RemoteActionFileSystem extends DelegateFileSystem {
 
   private final Path execRoot;
   private final Path outputBase;
   private final ActionInputMap inputArtifactData;
   private final RemoteActionInputFetcher inputFetcher;
 
-  public RemoteActionFileSystem(
+  RemoteActionFileSystem(
       FileSystem localDelegate,
       PathFragment execRootFragment,
       String relativeOutputPath,
@@ -61,6 +61,11 @@ public RemoteActionFileSystem(
     this.inputFetcher = Preconditions.checkNotNull(inputFetcher, "inputFetcher");
   }
 
+  /** Returns true if {@code path} is a file that's stored remotely. */
+  boolean isRemote(Path path) {
+    return getRemoteInputMetadata(path) != null;
+  }
+
   @Override
   public String getFileSystemType(Path path) {
     return "remoteActionFS";

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -255,24 +255,14 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
                 digestUtil,
                 uploader.retain());
         uploader.release();
-        if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
-          Context requestContext =
-              TracingMetadataUtils.contextWithMetadata(buildRequestId, invocationId, "bes-upload");
-          buildEventArtifactUploaderFactoryDelegate.init(
-              new ByteStreamBuildEventArtifactUploaderFactory(
-                  uploader,
-                  cacheChannel.authority(),
-                  requestContext,
-                  remoteOptions.remoteInstanceName));
-        } else {
-          // TODO(buchgr): Fix BES local file upload to work with
-          // --experimental_remote_download_outputs
-          env.getReporter()
-              .handle(
-                  Event.warn(
-                      "BES artifact upload is disabled when using "
-                          + "--experimental_remote_download_outputs=minimal"));
-        }
+        Context requestContext =
+            TracingMetadataUtils.contextWithMetadata(buildRequestId, invocationId, "bes-upload");
+        buildEventArtifactUploaderFactoryDelegate.init(
+            new ByteStreamBuildEventArtifactUploaderFactory(
+                uploader,
+                cacheChannel.authority(),
+                requestContext,
+                remoteOptions.remoteInstanceName));
       }
 
       if (enableBlobStoreCache) {

src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java

@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.vfs.BatchStat;
@@ -29,6 +30,8 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import java.util.Collection;
+import java.util.Map;
 import java.util.UUID;
 import java.util.function.Function;
 import javax.annotation.Nullable;
@@ -113,7 +116,29 @@ public void clean() {
   }
 
   @Override
-  public boolean isRemoteFile(Artifact file) {
-    return false;
+  public boolean isRemoteFile(Artifact artifact) {
+    Path path = artifact.getPath();
+    return path.getFileSystem() instanceof RemoteActionFileSystem
+        && ((RemoteActionFileSystem) path.getFileSystem()).isRemote(path);
+  }
+
+  @Override
+  public boolean supportsPathResolverForArtifactValues() {
+    return actionFileSystemType() != ActionFileSystemType.DISABLED;
+  }
+
+  @Override
+  public ArtifactPathResolver createPathResolverForArtifactValues(
+      PathFragment execRoot,
+      String relativeOutputPath,
+      FileSystem fileSystem,
+      ImmutableList<Root> pathEntries,
+      ActionInputMap actionInputMap,
+      Map<Artifact, Collection<Artifact>> expandedArtifacts,
+      Iterable<Artifact> filesets) {
+    FileSystem remoteFileSystem =
+        new RemoteActionFileSystem(
+            fileSystem, execRoot, relativeOutputPath, actionInputMap, actionInputFetcher);
+    return ArtifactPathResolver.createPathResolver(remoteFileSystem, fileSystem.getPath(execRoot));
   }
 }

src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java

@@ -16,14 +16,22 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import build.bazel.remote.execution.v2.Digest;
 import com.google.bytestream.ByteStreamProto.WriteRequest;
 import com.google.bytestream.ByteStreamProto.WriteResponse;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.hash.HashCode;
 import com.google.common.io.BaseEncoding;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+import com.google.devtools.build.lib.actions.ActionInputMap;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
@@ -46,6 +54,7 @@
 import io.grpc.stub.StreamObserver;
 import io.grpc.util.MutableHandlerRegistry;
 import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Random;
@@ -59,6 +68,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 
 /** Test for {@link ByteStreamBuildEventArtifactUploader}. */
@@ -76,6 +86,9 @@ public class ByteStreamBuildEventArtifactUploaderTest {
   private Context prevContext;
   private final FileSystem fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256);
 
+  private final Path execRoot = fs.getPath("/execroot");
+  private ArtifactRoot outputRoot;
+
   @BeforeClass
   public static void beforeEverything() {
     retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1));
@@ -98,6 +111,9 @@ public final void setUp() throws Exception {
     // Needs to be repeated in every test that uses the timeout setting, since the tests run
     // on different threads than the setUp.
     prevContext = withEmptyMetadata.attach();
+
+    outputRoot = ArtifactRoot.asDerivedRoot(execRoot, execRoot.getRelative("out"));
+    outputRoot.getRoot().asPath().createDirectoryAndParents();
   }
 
   @After
@@ -247,4 +263,63 @@ public void onCompleted() {
     assertThat(uploader.refCnt()).isEqualTo(0);
     assertThat(refCntChannel.isShutdown()).isTrue();
   }
+
+  @Test
+  public void remoteFileShouldNotBeUploaded() throws Exception {
+    // Test that we don't attempt to upload remotely stored file but convert the remote path
+    // to a bytestream:// URI.
+
+    // arrange
+
+    ByteStreamUploader uploader = Mockito.mock(ByteStreamUploader.class);
+    RemoteActionInputFetcher actionInputFetcher = Mockito.mock(RemoteActionInputFetcher.class);
+    ByteStreamBuildEventArtifactUploader artifactUploader =
+        new ByteStreamBuildEventArtifactUploader(
+            uploader, "localhost", withEmptyMetadata, "instance", /* maxUploadThreads= */ 100);
+
+    ActionInputMap outputs = new ActionInputMap(2);
+    Artifact artifact = createRemoteArtifact("file1.txt", "foo", outputs);
+
+    RemoteActionFileSystem remoteFs =
+        new RemoteActionFileSystem(
+            fs,
+            execRoot.asFragment(),
+            outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(),
+            outputs,
+            actionInputFetcher);
+    Path remotePath = remoteFs.getPath(artifact.getPath().getPathString());
+    assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs);
+    LocalFile file = new LocalFile(remotePath, LocalFileType.OUTPUT);
+
+    // act
+
+    PathConverter pathConverter = artifactUploader.upload(ImmutableMap.of(remotePath, file)).get();
+
+    FileArtifactValue metadata = outputs.getMetadata(artifact);
+    Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
+
+    // assert
+
+    String conversion = pathConverter.apply(remotePath);
+    assertThat(conversion)
+        .isEqualTo(
+            "bytestream://localhost/instance/blobs/"
+                + digest.getHash()
+                + "/"
+                + digest.getSizeBytes());
+    verifyNoMoreInteractions(uploader);
+  }
+
+  /** Returns a remote artifact and puts its metadata into the action input map. */
+  private Artifact createRemoteArtifact(
+      String pathFragment, String contents, ActionInputMap inputs) {
+    Path p = outputRoot.getRoot().asPath().getRelative(pathFragment);
+    Artifact a = ActionsTestUtil.createArtifact(outputRoot, p);
+    byte[] b = contents.getBytes(StandardCharsets.UTF_8);
+    HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash());
+    FileArtifactValue f =
+        new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1);
+    inputs.putWithNoDepOwner(a, f);
+    return a;
+  }
 }

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -1061,6 +1061,42 @@ EOF
 
 
 }
+
+function test_downloads_minimal_bep() {
+  # Test that when using --experimental_remote_download_outputs=minimal all URI's in the BEP
+  # are rewritten as bytestream://..
+  mkdir -p a
+  cat > a/success.sh <<'EOF'
+#!/bin/sh
+exit 0
+EOF
+  chmod 755 a/success.sh
+  cat > a/BUILD <<'EOF'
+sh_test(
+  name = "success_test",
+  srcs = ["success.sh"],
+)
+
+genrule(
+  name = "foo",
+  srcs = [],
+  outs = ["foo.txt"],
+  cmd = "echo \"foo\" > \"$@\"",
+)
+EOF
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_inmemory_jdeps_files \
+    --experimental_inmemory_dotd_files \
+    --experimental_remote_download_outputs=minimal \
+    --build_event_text_file=$TEST_log \
+    //a:foo //a:success_test || fail "Failed to test //a:foo //a:success_test"
+
+  expect_not_log 'uri:.*file://'
+  expect_log "uri:.*bytestream://localhost"
+}
+
 # TODO(alpha): Add a test that fails remote execution when remote worker
 # supports sandbox.