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.

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;
@@ -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.resolveExecPath(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.resolveExecPath(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.getExecPath(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 execPath = remotePathResolver.getExecPath(output);
     if (output.isTreeArtifact()) {
-      DirectoryMetadata directory =
-          metadata.directory(execRoot.getRelative(output.getExecPathString()));
+      DirectoryMetadata directory = metadata.directory(execPath);
       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(execPath);
       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.resolveExecPath(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 execPath = remotePathResolver.resolveExecPath(outputFile.getPath());
       files.put(
-          inputRoot.getRelative(outputFile.getPath()),
+          execPath,
           new FileMetadata(
-              inputRoot.getRelative(outputFile.getPath()),
+              execPath,
               outputFile.getDigest(),
               outputFile.getIsExecutable()));
     }
@@ -778,10 +773,11 @@ private ActionResultMetadata parseActionResultMetadata(
             actionResult.getOutputFileSymlinksList(),
             actionResult.getOutputDirectorySymlinksList());
     for (OutputSymlink symlink : outputSymlinks) {
+      Path execPath = remotePathResolver.resolveExecPath(symlink.getPath());
       symlinks.put(
-          inputRoot.getRelative(symlink.getPath()),
+          execPath,
           new SymlinkMetadata(
-              inputRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget())));
+              execPath, PathFragment.create(symlink.getTarget())));
     }
 
     return new ActionResultMetadata(files.build(), symlinks.build(), directories.build());

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

@@ -44,6 +44,7 @@
 import java.io.IOException;
 import java.time.Duration;
 import java.util.Map;
+import java.util.TreeSet;
 
 /** The remote package's implementation of {@link RepositoryRemoteExecutor}. */
 public class RemoteRepositoryRemoteExecutor implements RepositoryRemoteExecutor {
@@ -110,9 +111,22 @@ public ExecutionResult execute(
     RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata);
 
     Platform platform = PlatformUtils.buildPlatformProto(executionProperties);
-    Command command =
-        RemoteSpawnRunner.buildCommand(
-            /* outputs= */ ImmutableList.of(), arguments, environment, platform, workingDirectory);
+
+    Command.Builder commandBuilder = Command.newBuilder()
+        .addAllArguments(arguments);
+    // Sorting the environment pairs by variable name.
+    TreeSet<String> variables = new TreeSet<>(environment.keySet());
+    for (String var : variables) {
+      commandBuilder.addEnvironmentVariablesBuilder().setName(var).setValue(environment.get(var));
+    }
+    if (platform != null) {
+        commandBuilder.setPlatform(platform);
+    }
+    if (workingDirectory != null) {
+      commandBuilder.setWorkingDirectory(workingDirectory);
+    }
+
+    Command command = commandBuilder.build();
     Digest commandHash = digestUtil.compute(command);
     MerkleTree merkleTree = MerkleTree.build(inputFiles, digestUtil);
     Action action =

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

@@ -51,6 +51,7 @@
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
 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.merkletree.MerkleTree;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
@@ -85,6 +86,7 @@ final class RemoteSpawnCache implements SpawnCache {
   private final Set<String> reportedErrors = new HashSet<>();
 
   private final DigestUtil digestUtil;
+  private final RemotePathResolver remotePathResolver;
 
   /**
    * If {@link RemoteOutputsMode#TOPLEVEL} is specified it contains the artifacts that should be
@@ -101,7 +103,8 @@ final class RemoteSpawnCache implements SpawnCache {
       String commandId,
       @Nullable Reporter cmdlineReporter,
       DigestUtil digestUtil,
-      ImmutableSet<ActionInput> filesToDownload) {
+      ImmutableSet<ActionInput> filesToDownload,
+      RemotePathResolver remotePathResolver) {
     this.execRoot = execRoot;
     this.options = options;
     this.verboseFailures = verboseFailures;
@@ -111,6 +114,7 @@ final class RemoteSpawnCache implements SpawnCache {
     this.commandId = commandId;
     this.digestUtil = digestUtil;
     this.filesToDownload = Preconditions.checkNotNull(filesToDownload, "filesToDownload");
+    this.remotePathResolver = remotePathResolver;
   }
 
   @Override
@@ -125,8 +129,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
 
     Stopwatch totalTime = Stopwatch.createStarted();
 
-    SortedMap<PathFragment, ActionInput> inputMap =
-        context.getInputMapping(PathFragment.create(execRoot.getBaseName()));
+    SortedMap<PathFragment, ActionInput> inputMap = remotePathResolver.getInputMapping(context);
     MerkleTree merkleTree =
         MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
     SpawnMetrics.Builder spawnMetrics =
@@ -144,7 +147,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
             spawn.getArguments(),
             spawn.getEnvironment(),
             platform,
-            execRoot.getBaseName());
+            remotePathResolver);
     RemoteOutputsMode remoteOutputsMode = options.remoteOutputsMode;
     Action action =
         RemoteSpawnRunner.buildAction(
@@ -186,8 +189,8 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
                 prof.profile(ProfilerTask.REMOTE_DOWNLOAD, "download outputs")) {
               remoteCache.download(
                   remoteActionExecutionContext,
+                  remotePathResolver,
                   result,
-                  execRoot,
                   context.getFileOutErr(),
                   context::lockOutputFiles);
             }
@@ -199,12 +202,11 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
               inMemoryOutput =
                   remoteCache.downloadMinimal(
                       remoteActionExecutionContext,
-                      actionKey.getDigest().getHash(),
+                      remotePathResolver,
                       result,
                       spawn.getOutputFiles(),
                       inMemoryOutputPath,
                       context.getFileOutErr(),
-                      execRoot,
                       context.getMetadataInjector(),
                       context::lockOutputFiles);
             }