Remote: Use execRoot as input root and do NOT set working directory b…

…y default.

When --experimental_sibling_external_layout is set, use the parent directory of execRoot as input root and set working directory to the base name of execRoot. Paths of output files are relative to working directory. When --incompatible_remote_paths_relative_to_input_root is set, paths of output files are relative to input root.

Introduce RemotePathResolver which is used to convert local paths to remote paths and vice versa. We should prefer this class instead of using execRoot directly whenever possible. execRoot usages in RemoteCache are all replaced.

Fixes bazelbuild#13188.

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -70,6 +70,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/remote/common",
         "//src/main/java/com/google/devtools/build/lib/remote/disk",

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

@@ -27,7 +27,11 @@
 import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
 import com.google.devtools.build.lib.exec.SpawnCache;
 import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.remote.common.RemoteExecutionClient;
+import com.google.devtools.build.lib.remote.common.RemotePathResolver;
+import com.google.devtools.build.lib.remote.common.RemotePathResolver.DefaultRemotePathResolver;
+import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingExternalLayoutResolver;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
@@ -80,6 +84,20 @@ public static RemoteActionContextProvider createForRemoteExecution(
         env, cache, executor, retryScheduler, digestUtil, logDir);
   }
 
+  RemotePathResolver createRemotePathResolver() {
+    Path execRoot = env.getExecRoot();
+    BuildLanguageOptions buildLanguageOptions = env.getOptions()
+        .getOptions(BuildLanguageOptions.class);
+    RemotePathResolver remotePathResolver;
+    if (buildLanguageOptions != null && buildLanguageOptions.experimentalSiblingRepositoryLayout) {
+      RemoteOptions remoteOptions = checkNotNull(env.getOptions().getOptions(RemoteOptions.class));
+      remotePathResolver = new SiblingExternalLayoutResolver(execRoot, remoteOptions.incompatibleRemoteOutputPathsRelativeToInputRoot);
+    } else {
+      remotePathResolver = new DefaultRemotePathResolver(execRoot);
+    }
+    return remotePathResolver;
+  }
+
   /**
    * Registers a remote spawn strategy if this instance was created with an executor, otherwise does
    * nothing.
@@ -108,7 +126,8 @@ public void registerRemoteSpawnStrategyIfApplicable(
             retryScheduler,
             digestUtil,
             logDir,
-            filesToDownload);
+            filesToDownload,
+            createRemotePathResolver());
     registryBuilder.registerStrategy(
         new RemoteSpawnStrategy(env.getExecRoot(), spawnRunner, verboseFailures), "remote");
   }
@@ -129,7 +148,8 @@ public void registerSpawnCache(ModuleActionContextRegistry.Builder registryBuild
             env.getCommandId().toString(),
             env.getReporter(),
             digestUtil,
-            filesToDownload);
+            filesToDownload,
+            createRemotePathResolver());
     registryBuilder.register(SpawnCache.class, spawnCache, "remote-cache");
   }
 

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

@@ -59,6 +59,7 @@
 import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
 import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
+import com.google.devtools.build.lib.remote.common.RemotePathResolver;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput;
@@ -130,16 +131,16 @@ public ActionResult downloadActionResult(
    */
   public ActionResult upload(
       RemoteActionExecutionContext context,
+      RemotePathResolver remotePathResolver,
       ActionKey actionKey,
       Action action,
       Command command,
-      Path execRoot,
       Collection<Path> outputs,
       FileOutErr outErr,
       int exitCode)
       throws ExecException, IOException, InterruptedException {
     ActionResult.Builder resultBuilder = ActionResult.newBuilder();
-    uploadOutputs(context, execRoot, actionKey, action, command, outputs, outErr, resultBuilder);
+    uploadOutputs(context, remotePathResolver, actionKey, action, command, outputs, outErr, resultBuilder);
     resultBuilder.setExitCode(exitCode);
     ActionResult result = resultBuilder.build();
     if (exitCode == 0 && !action.getDoNotCache()) {
@@ -150,20 +151,20 @@ public ActionResult upload(
 
   public ActionResult upload(
       RemoteActionExecutionContext context,
+      RemotePathResolver remotePathResolver,
       ActionKey actionKey,
       Action action,
       Command command,
-      Path execRoot,
       Collection<Path> outputs,
       FileOutErr outErr)
       throws ExecException, IOException, InterruptedException {
     return upload(
-        context, actionKey, action, command, execRoot, outputs, outErr, /* exitCode= */ 0);
+        context, remotePathResolver, actionKey, action, command, outputs, outErr, /* exitCode= */ 0);
   }
 
   private void uploadOutputs(
       RemoteActionExecutionContext context,
-      Path execRoot,
+      RemotePathResolver remotePathResolver,
       ActionKey actionKey,
       Action action,
       Command command,
@@ -174,8 +175,8 @@ private void uploadOutputs(
     UploadManifest manifest =
         new UploadManifest(
             digestUtil,
+            remotePathResolver,
             result,
-            execRoot,
             options.incompatibleRemoteSymlinks,
             options.allowSymlinkUpload);
     manifest.addFiles(files);
@@ -309,15 +310,12 @@ private static Path toTmpDownloadPath(Path actualPath) {
    */
   public void download(
       RemoteActionExecutionContext context,
+      RemotePathResolver remotePathResolver,
       ActionResult result,
-      Path execRoot,
       FileOutErr origOutErr,
       OutputFilesLocker outputFilesLocker)
       throws ExecException, IOException, InterruptedException {
-    // The input root for RBE is the parent directory of the exec root so that paths to files in
-    // external repositories don't start with an uplevel reference
-    Path inputRoot = execRoot.getParentDirectory();
-    ActionResultMetadata metadata = parseActionResultMetadata(context, result, inputRoot);
+    ActionResultMetadata metadata = parseActionResultMetadata(context, remotePathResolver, result);
 
     List<ListenableFuture<FileMetadata>> downloads =
         Stream.concat(
@@ -352,12 +350,12 @@ public void download(
       try {
         // Delete any (partially) downloaded output files.
         for (OutputFile file : result.getOutputFilesList()) {
-          toTmpDownloadPath(inputRoot.getRelative(file.getPath())).delete();
+          toTmpDownloadPath(remotePathResolver.resolveLocalPath(file.getPath())).delete();
         }
         for (OutputDirectory directory : result.getOutputDirectoriesList()) {
           // Only delete the directories below the output directories because the output
           // directories will not be re-created
-          inputRoot.getRelative(directory.getPath()).deleteTreesBelow();
+          remotePathResolver.resolveLocalPath(directory.getPath()).deleteTreesBelow();
         }
         if (tmpOutErr != null) {
           tmpOutErr.clearOut();
@@ -566,7 +564,6 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(
    * @param inMemoryOutputPath the path of an output file whose contents should be returned in
    *     memory by this method.
    * @param outErr stdout and stderr of this action
-   * @param execRoot the execution root
    * @param metadataInjector the action's metadata injector that allows this method to inject
    *     metadata about an action output instead of downloading the output
    * @param outputFilesLocker ensures that we are the only ones writing to the output files when
@@ -577,12 +574,11 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(
   @Nullable
   public InMemoryOutput downloadMinimal(
       RemoteActionExecutionContext context,
-      String actionId,
+      RemotePathResolver remotePathResolver,
       ActionResult result,
       Collection<? extends ActionInput> outputs,
       @Nullable PathFragment inMemoryOutputPath,
       OutErr outErr,
-      Path execRoot,
       MetadataInjector metadataInjector,
       OutputFilesLocker outputFilesLocker)
       throws IOException, InterruptedException {
@@ -592,11 +588,7 @@ public InMemoryOutput downloadMinimal(
 
     ActionResultMetadata metadata;
     try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) {
-      // We tell RBE that the input root of the action is the parent directory of what is locally
-      // the execroot. This is so that paths of artifacts in external repositories don't start with
-      // an uplevel reference.
-      Path inputRoot = execRoot.getParentDirectory();
-      metadata = parseActionResultMetadata(context, result, inputRoot);
+      metadata = parseActionResultMetadata(context, remotePathResolver, result);
     }
 
     if (!metadata.symlinks().isEmpty()) {
@@ -613,7 +605,7 @@ public InMemoryOutput downloadMinimal(
     Digest inMemoryOutputDigest = null;
     for (ActionInput output : outputs) {
       if (inMemoryOutputPath != null && output.getExecPath().equals(inMemoryOutputPath)) {
-        Path p = execRoot.getRelative(output.getExecPath());
+        Path p = remotePathResolver.resolveLocalPath(output);
         FileMetadata m = metadata.file(p);
         if (m == null) {
           // A declared output wasn't created. Ignore it here. SkyFrame will fail if not all
@@ -624,7 +616,8 @@ public InMemoryOutput downloadMinimal(
         inMemoryOutput = output;
       }
       if (output instanceof Artifact) {
-        injectRemoteArtifact((Artifact) output, metadata, execRoot, metadataInjector, actionId);
+        injectRemoteArtifact(context, remotePathResolver, (Artifact) output, metadata,
+            metadataInjector);
       }
     }
 
@@ -646,15 +639,15 @@ public InMemoryOutput downloadMinimal(
   }
 
   private void injectRemoteArtifact(
+      RemoteActionExecutionContext context,
+      RemotePathResolver remotePathResolver,
       Artifact output,
       ActionResultMetadata metadata,
-      Path execRoot,
-      MetadataInjector metadataInjector,
-      String actionId)
+      MetadataInjector metadataInjector)
       throws IOException {
+    Path path = remotePathResolver.resolveLocalPath(output);
     if (output.isTreeArtifact()) {
-      DirectoryMetadata directory =
-          metadata.directory(execRoot.getRelative(output.getExecPathString()));
+      DirectoryMetadata directory = metadata.directory(path);
       if (directory == null) {
         // A declared output wasn't created. It might have been an optional output and if not
         // SkyFrame will make sure to fail.
@@ -675,13 +668,13 @@ private void injectRemoteArtifact(
                 DigestUtil.toBinaryDigest(file.digest()),
                 file.digest().getSizeBytes(),
                 /*locationIndex=*/ 1,
-                actionId,
+                context.getRequestMetadata().getActionId(),
                 file.isExecutable());
         tree.putChild(child, value);
       }
       metadataInjector.injectTree(parent, tree.build());
     } else {
-      FileMetadata outputMetadata = metadata.file(execRoot.getRelative(output.getExecPathString()));
+      FileMetadata outputMetadata = metadata.file(path);
       if (outputMetadata == null) {
         // A declared output wasn't created. It might have been an optional output and if not
         // SkyFrame will make sure to fail.
@@ -693,7 +686,7 @@ private void injectRemoteArtifact(
               DigestUtil.toBinaryDigest(outputMetadata.digest()),
               outputMetadata.digest().getSizeBytes(),
               /*locationIndex=*/ 1,
-              actionId,
+              context.getRequestMetadata().getActionId(),
               outputMetadata.isExecutable()));
     }
   }
@@ -727,14 +720,15 @@ private DirectoryMetadata parseDirectory(
   }
 
   private ActionResultMetadata parseActionResultMetadata(
-      RemoteActionExecutionContext context, ActionResult actionResult, Path inputRoot)
+      RemoteActionExecutionContext context, RemotePathResolver remotePathResolver,
+      ActionResult actionResult)
       throws IOException, InterruptedException {
     Preconditions.checkNotNull(actionResult, "actionResult");
     Map<Path, ListenableFuture<Tree>> dirMetadataDownloads =
         Maps.newHashMapWithExpectedSize(actionResult.getOutputDirectoriesCount());
     for (OutputDirectory dir : actionResult.getOutputDirectoriesList()) {
       dirMetadataDownloads.put(
-          inputRoot.getRelative(dir.getPath()),
+          remotePathResolver.resolveLocalPath(dir.getPath()),
           Futures.transform(
               downloadBlob(context, dir.getTreeDigest()),
               (treeBytes) -> {
@@ -764,10 +758,11 @@ private ActionResultMetadata parseActionResultMetadata(
 
     ImmutableMap.Builder<Path, FileMetadata> files = ImmutableMap.builder();
     for (OutputFile outputFile : actionResult.getOutputFilesList()) {
+      Path path = remotePathResolver.resolveLocalPath(outputFile.getPath());
       files.put(
-          inputRoot.getRelative(outputFile.getPath()),
+          path,
           new FileMetadata(
-              inputRoot.getRelative(outputFile.getPath()),
+              path,
               outputFile.getDigest(),
               outputFile.getIsExecutable()));
     }
@@ -778,10 +773,11 @@ private ActionResultMetadata parseActionResultMetadata(
             actionResult.getOutputFileSymlinksList(),
             actionResult.getOutputDirectorySymlinksList());
     for (OutputSymlink symlink : outputSymlinks) {
+      Path path = remotePathResolver.resolveLocalPath(symlink.getPath());
       symlinks.put(
-          inputRoot.getRelative(symlink.getPath()),
+          path,
           new SymlinkMetadata(
-              inputRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget())));
+              path, PathFragment.create(symlink.getTarget())));
     }
 
     return new ActionResultMetadata(files.build(), symlinks.build(), directories.build());
@@ -790,8 +786,8 @@ private ActionResultMetadata parseActionResultMetadata(
   /** UploadManifest adds output metadata to a {@link ActionResult}. */
   static class UploadManifest {
     private final DigestUtil digestUtil;
+    private final RemotePathResolver remotePathResolver;
     private final ActionResult.Builder result;
-    private final Path execRoot;
     private final boolean allowSymlinks;
     private final boolean uploadSymlinks;
     private final Map<Digest, Path> digestToFile = new HashMap<>();
@@ -805,13 +801,13 @@ static class UploadManifest {
      */
     public UploadManifest(
         DigestUtil digestUtil,
+        RemotePathResolver remotePathResolver,
         ActionResult.Builder result,
-        Path execRoot,
         boolean uploadSymlinks,
         boolean allowSymlinks) {
       this.digestUtil = digestUtil;
+      this.remotePathResolver = remotePathResolver;
       this.result = result;
-      this.execRoot = execRoot;
       this.uploadSymlinks = uploadSymlinks;
       this.allowSymlinks = allowSymlinks;
     }
@@ -917,21 +913,21 @@ public Digest getStderrDigest() {
     private void addFileSymbolicLink(Path file, PathFragment target) throws IOException {
       result
           .addOutputFileSymlinksBuilder()
-          .setPath(file.relativeTo(execRoot).getPathString())
+          .setPath(remotePathResolver.resolveOutputPath(file))
           .setTarget(target.toString());
     }
 
     private void addDirectorySymbolicLink(Path file, PathFragment target) throws IOException {
       result
           .addOutputDirectorySymlinksBuilder()
-          .setPath(file.relativeTo(execRoot).getPathString())
+          .setPath(remotePathResolver.resolveOutputPath(file))
           .setTarget(target.toString());
     }
 
     private void addFile(Digest digest, Path file) throws IOException {
       result
           .addOutputFilesBuilder()
-          .setPath(file.relativeTo(execRoot).getPathString())
+          .setPath(remotePathResolver.resolveOutputPath(file))
           .setDigest(digest)
           .setIsExecutable(file.isExecutable());
 
@@ -949,7 +945,7 @@ private void addDirectory(Path dir) throws ExecException, IOException {
       if (result != null) {
         result
             .addOutputDirectoriesBuilder()
-            .setPath(dir.relativeTo(execRoot).getPathString())
+            .setPath(remotePathResolver.resolveOutputPath(dir))
             .setTreeDigest(digest);
       }
 
@@ -1017,7 +1013,7 @@ private void illegalOutput(Path what) throws ExecException {
               "Output %s is a %s. Only regular files and directories may be "
                   + "uploaded to a remote cache. "
                   + "Change the file type or use --remote_allow_symlink_upload.",
-              what.relativeTo(execRoot), kind);
+              remotePathResolver.resolveOutputPath(what), kind);
       throw new UserExecException(createFailureDetail(message, Code.ILLEGAL_OUTPUT));
     }
   }