From bbde729fb53ca624ca0d264ed2703e5db010564a Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Thu, 11 Mar 2021 17:52:34 +0800 Subject: [PATCH] Remote: Use execRoot as input root and do NOT set working directory by 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 https://github.com/bazelbuild/bazel/issues/13188. --- .../google/devtools/build/lib/remote/BUILD | 1 + .../remote/RemoteActionContextProvider.java | 24 ++- .../build/lib/remote/RemoteCache.java | 86 ++++---- .../RemoteRepositoryRemoteExecutor.java | 20 +- .../build/lib/remote/RemoteSpawnCache.java | 18 +- .../build/lib/remote/RemoteSpawnRunner.java | 41 ++-- .../devtools/build/lib/remote/common/BUILD | 4 + .../lib/remote/common/RemotePathResolver.java | 194 ++++++++++++++++++ .../merkletree/DirectoryTreeBuilder.java | 8 + .../lib/remote/options/RemoteOptions.java | 12 ++ .../build/lib/remote/GrpcCacheClientTest.java | 55 +++-- .../build/lib/remote/RemoteCacheTests.java | 152 +++++++------- .../lib/remote/RemoteSpawnCacheTest.java | 36 ++-- .../lib/remote/RemoteSpawnRunnerTest.java | 43 ++-- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 7 +- .../build/remote/worker/ExecutionServer.java | 15 +- 16 files changed, 503 insertions(+), 213 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 741a246f7edf19..ad5159da2b8631 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 35f7afba04b1e2..6f3843e94279c0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/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"); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 1406218057ce47..9dcced0190464c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/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 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 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> 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> 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> downloadOutErr( @Nullable public InMemoryOutput downloadMinimal( RemoteActionExecutionContext context, - String actionId, + RemotePathResolver remotePathResolver, ActionResult result, Collection 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> 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 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 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)); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java index 54e200b6de7f80..de079f7c66e87c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java +++ b/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 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 = diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index 488105b91e1a88..322321e81bf303 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/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 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 filesToDownload) { + ImmutableSet 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 inputMap = - context.getInputMapping(PathFragment.create(execRoot.getBaseName())); + SortedMap 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); } @@ -288,10 +290,10 @@ public void store(SpawnResult result) throws ExecException, InterruptedException try (SilentCloseable c = prof.profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) { remoteCache.upload( remoteActionExecutionContext, + remotePathResolver, actionKey, action, command, - execRoot.getParentDirectory(), files, context.getFileOutErr()); } catch (IOException e) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 92027b8410e5ff..4085946f68a139 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -71,6 +71,7 @@ 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.RemoteExecutionClient; +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; @@ -122,6 +123,7 @@ public class RemoteSpawnRunner implements SpawnRunner { private final String commandId; private final DigestUtil digestUtil; private final Path logDir; + private final RemotePathResolver remotePathResolver; /** * If {@link RemoteOutputsMode#TOPLEVEL} is specified it contains the artifacts that should be @@ -145,7 +147,8 @@ public class RemoteSpawnRunner implements SpawnRunner { ListeningScheduledExecutorService retryService, DigestUtil digestUtil, Path logDir, - ImmutableSet filesToDownload) { + ImmutableSet filesToDownload, + RemotePathResolver remotePathResolver) { this.execRoot = execRoot; this.remoteOptions = remoteOptions; this.executionOptions = executionOptions; @@ -159,6 +162,7 @@ public class RemoteSpawnRunner implements SpawnRunner { this.digestUtil = digestUtil; this.logDir = logDir; this.filesToDownload = Preconditions.checkNotNull(filesToDownload, "filesToDownload"); + this.remotePathResolver = remotePathResolver; } @Override @@ -213,14 +217,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) context.report(ProgressStatus.SCHEDULING, getName()); RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode; - // The "root directory" of the action from the point of view of RBE is the parent directory of - // the execroot locally. This is so that paths of artifacts in external repositories don't - // start with an uplevel reference... - SortedMap inputMap = - context.getInputMapping(PathFragment.create(execRoot.getBaseName())); - - // ...however, MerkleTree.build() uses its execRoot parameter to resolve artifacts based on - // ActionInput.getExecPath(), so it needs the execroot and not its parent directory. + + SortedMap inputMap = remotePathResolver.getInputMapping(context); final MerkleTree merkleTree = MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil); SpawnMetrics.Builder spawnMetrics = @@ -238,7 +236,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) spawn.getArguments(), spawn.getEnvironment(), platform, - execRoot.getBaseName()); + remotePathResolver); Digest commandHash = digestUtil.compute(command); Action action = buildAction( @@ -483,8 +481,8 @@ private SpawnResult downloadAndFinalizeSpawnResult( try (SilentCloseable c = Profiler.instance().profile(REMOTE_DOWNLOAD, "download outputs")) { remoteCache.download( remoteActionExecutionContext, + remotePathResolver, actionResult, - execRoot, context.getFileOutErr(), context::lockOutputFiles); } @@ -495,12 +493,11 @@ private SpawnResult downloadAndFinalizeSpawnResult( inMemoryOutput = remoteCache.downloadMinimal( remoteActionExecutionContext, - actionId, + remotePathResolver, actionResult, spawn.getOutputFiles(), inMemoryOutputPath, context.getFileOutErr(), - execRoot, context.getMetadataInjector(), context::lockOutputFiles); } @@ -636,8 +633,8 @@ private SpawnResult handleError( // We try to download all (partial) results even on server error, for debuggability. remoteCache.download( remoteActionExecutionContext, + remotePathResolver, resp.getResult(), - execRoot, outErr, context::lockOutputFiles); } catch (BulkTransferException bulkTransferEx) { @@ -726,17 +723,12 @@ static Command buildCommand( List arguments, ImmutableMap env, @Nullable Platform platform, - @Nullable String workingDirectoryString) { + RemotePathResolver remotePathResolver) { Command.Builder command = Command.newBuilder(); ArrayList outputFiles = new ArrayList<>(); ArrayList outputDirectories = new ArrayList<>(); - PathFragment workingDirectoryPathFragment = - workingDirectoryString == null - ? PathFragment.EMPTY_FRAGMENT - : PathFragment.create(workingDirectoryString); for (ActionInput output : outputs) { - String pathString = - workingDirectoryPathFragment.getRelative(output.getExecPath()).getPathString(); + String pathString = remotePathResolver.resolveOutputPath(output); if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { outputDirectories.add(pathString); } else { @@ -758,8 +750,9 @@ static Command buildCommand( command.addEnvironmentVariablesBuilder().setName(var).setValue(env.get(var)); } - if (!Strings.isNullOrEmpty(workingDirectoryString)) { - command.setWorkingDirectory(workingDirectoryString); + String workingDirectory = remotePathResolver.getWorkingDirectory(); + if (!Strings.isNullOrEmpty(workingDirectory)) { + command.setWorkingDirectory(workingDirectory); } return command.build(); } @@ -815,10 +808,10 @@ SpawnResult execLocallyAndUpload( try (SilentCloseable c = Profiler.instance().profile(UPLOAD_TIME, "upload outputs")) { remoteCache.upload( remoteActionExecutionContext, + remotePathResolver, actionKey, action, command, - execRoot, outputFiles, context.getFileOutErr()); } catch (IOException e) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/BUILD b/src/main/java/com/google/devtools/build/lib/remote/common/BUILD index 515eb63205d0a2..36853a5755d758 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/common/BUILD @@ -14,9 +14,13 @@ java_library( name = "common", srcs = glob(["*.java"]), deps = [ + "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:guava", "//third_party/protobuf:protobuf_java", "@googleapis//:google_longrunning_operations_java_proto", diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java new file mode 100644 index 00000000000000..466d603e36f180 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -0,0 +1,194 @@ +// Copyright 2020 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.remote.common; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; +import java.util.SortedMap; +import javax.annotation.Nullable; + +/** + * A {@link RemotePathResolver} is used to resolve input/output paths for remote execution from + * Bazel's internal path, or vice versa. + */ +public interface RemotePathResolver { + + /** + * @return the {@code workingDirectory} for a remote action. + */ + @Nullable + String getWorkingDirectory(); + + /** + * @return a {@link SortedMap} which maps from input paths for remote action to {@link + * ActionInput}. + */ + SortedMap getInputMapping(SpawnExecutionContext context) + throws IOException; + + /** + * Resolves the output path relative to input root for the given {@link Path}. + */ + String resolveOutputPath(Path path); + + /** + * Resolves the output path relative to input root for the given {@link PathFragment}. + * + * @param execPath a path fragment relative to {@code execRoot}. + */ + String resolveOutputPath(PathFragment execPath); + + /** + * Resolves the output path relative to input root for the {@link ActionInput}. + */ + default String resolveOutputPath(ActionInput actionInput) { + return resolveOutputPath(actionInput.getExecPath()); + } + + /** + * Resolves the local {@link Path} of an output file. + * + * @param outputPath the return value of {@link #resolveOutputPath(PathFragment)}. + */ + Path resolveLocalPath(String outputPath); + + /** + * Resolves the local {@link Path} for the {@link ActionInput}. + */ + default Path resolveLocalPath(ActionInput actionInput) { + String outputPath = resolveOutputPath(actionInput.getExecPath()); + return resolveLocalPath(outputPath); + } + + /** + * Creates the default {@link RemotePathResolver}. + */ + static RemotePathResolver createDefault(Path execRoot) { + return new DefaultRemotePathResolver(execRoot); + } + + /** + * The default {@link RemotePathResolver} which use {@code execRoot} as input root and do NOT + * set {@code workingDirectory} for remote actions. + */ + class DefaultRemotePathResolver implements RemotePathResolver { + + final private Path execRoot; + + public DefaultRemotePathResolver(Path execRoot) { + this.execRoot = execRoot; + } + + @Nullable + @Override + public String getWorkingDirectory() { + return null; + } + + @Override + public SortedMap getInputMapping(SpawnExecutionContext context) + throws IOException { + return context.getInputMapping(PathFragment.EMPTY_FRAGMENT); + } + + @Override + public String resolveOutputPath(Path path) { + return path.relativeTo(execRoot).getPathString(); + } + + @Override + public String resolveOutputPath(PathFragment execPath) { + return execPath.getPathString(); + } + + @Override + public Path resolveLocalPath(String outputPath) { + return execRoot.getRelative(outputPath); + } + + @Override + public Path resolveLocalPath(ActionInput actionInput) { + return ActionInputHelper.toInputPath(actionInput, execRoot); + } + } + + /** + * A {@link RemotePathResolver} used when {@code --experimental_sibling_external_layout} is set. + * Use parent directory of {@code execRoot} and set {@code workingDirectory} to the base name of + * {@code execRoot}. + * + * The paths of outputs are relative to {@code workingDirectory} if + * {@code --incompatible_remote_output_paths_relative_to_input_root} is not set, otherwise, + * relative to input root. + */ + class SiblingExternalLayoutResolver implements RemotePathResolver { + + private final Path execRoot; + private final boolean incompatibleRemoteOutputPathsRelativeToInputRoot; + + public SiblingExternalLayoutResolver(Path execRoot, + boolean incompatibleRemoteOutputPathsRelativeToInputRoot) { + this.execRoot = execRoot; + this.incompatibleRemoteOutputPathsRelativeToInputRoot = incompatibleRemoteOutputPathsRelativeToInputRoot; + } + + @Override + public String getWorkingDirectory() { + return execRoot.getBaseName(); + } + + @Override + public SortedMap getInputMapping(SpawnExecutionContext context) + throws IOException { + // The "root directory" of the action from the point of view of RBE is the parent directory of + // the execroot locally. This is so that paths of artifacts in external repositories don't + // start with an uplevel reference. + return context.getInputMapping(PathFragment.create(checkNotNull(getWorkingDirectory()))); + } + + private Path getBase() { + if (incompatibleRemoteOutputPathsRelativeToInputRoot) { + return execRoot.getParentDirectory(); + } else { + return execRoot; + } + } + + @Override + public String resolveOutputPath(Path path) { + return path.relativeTo(getBase()).getPathString(); + } + + @Override + public String resolveOutputPath(PathFragment execPath) { + return resolveOutputPath(execRoot.getRelative(execPath)); + } + + @Override + public Path resolveLocalPath(String outputPath) { + return getBase().getRelative(outputPath); + } + + @Override + public Path resolveLocalPath(ActionInput actionInput) { + return ActionInputHelper.toInputPath(actionInput, execRoot); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index 1ddcf963520079..19e6aa163e344e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.protobuf.ByteString; import java.io.IOException; import java.util.Collection; import java.util.HashMap; @@ -60,6 +61,13 @@ static DirectoryTree fromActionInputs( throws IOException { Map tree = new HashMap<>(); int numFiles = buildFromActionInputs(inputs, metadataProvider, execRoot, digestUtil, tree); + // // Make sure working directory is exists + // PathFragment workingDirectory = PathFragment.create(execRoot.getBaseName()); + // if (!tree.containsKey(workingDirectory)) { + // DirectoryNode dir = new DirectoryNode(workingDirectory.toString()); + // tree.put(workingDirectory, dir); + // createParentDirectoriesIfNotExist(workingDirectory, dir, tree); + // } return new DirectoryTree(tree, numFiles); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 7f7725a565cbc7..5a2a8424f52669 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -266,6 +266,18 @@ public String getTypeDescription() { + "See #8216 for details.") public boolean incompatibleRemoteResultsIgnoreDisk; + @Option( + name = "incompatible_remote_output_paths_relative_to_input_root", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If set to true, output paths are relative to input root instead of working directory.") + public boolean incompatibleRemoteOutputPathsRelativeToInputRoot; + @Option( name = "remote_instance_name", defaultValue = "", diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 4a05a10775ae2c..a961a0f89f9882 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -64,6 +64,8 @@ import com.google.devtools.build.lib.remote.Retrier.Backoff; 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.common.RemotePathResolver.SiblingExternalLayoutResolver; import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -133,6 +135,7 @@ public class GrpcCacheClientTest { private final String fakeServerName = "fake server for " + getClass(); private Server fakeServer; private RemoteActionExecutionContext context; + private RemotePathResolver remotePathResolver; private ListeningScheduledExecutorService retryService; @Before @@ -149,6 +152,7 @@ public final void setUp() throws Exception { execRoot = fs.getPath("/execroot/main"); FileSystemUtils.createDirectoryAndParents(execRoot); fakeFileCache = new FakeActionInputFileCache(execRoot); + remotePathResolver = RemotePathResolver.createDefault(execRoot); Path stdout = fs.getPath("/tmp/stdout"); Path stderr = fs.getPath("/tmp/stderr"); @@ -376,6 +380,31 @@ public void testDownloadAllResults() throws Exception { GrpcCacheClient client = newClient(remoteOptions); RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); + Digest fooDigest = DIGEST_UTIL.computeAsUtf8("foo-contents"); + Digest barDigest = DIGEST_UTIL.computeAsUtf8("bar-contents"); + Digest emptyDigest = DIGEST_UTIL.compute(new byte[0]); + serviceRegistry.addService( + new FakeImmutableCacheByteStreamImpl(fooDigest, "foo-contents", barDigest, "bar-contents")); + + ActionResult.Builder result = ActionResult.newBuilder(); + result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); + result.addOutputFilesBuilder().setPath("b/empty").setDigest(emptyDigest); + result.addOutputFilesBuilder().setPath("a/bar").setDigest(barDigest).setIsExecutable(true); + remoteCache.download( + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); + assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); + assertThat(DIGEST_UTIL.compute(execRoot.getRelative("b/empty"))).isEqualTo(emptyDigest); + assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar"))).isEqualTo(barDigest); + assertThat(execRoot.getRelative("a/bar").isExecutable()).isTrue(); + } + + @Test + public void testDownloadAllResultsForSiblingLayoutAndRelativeToInputRoot() throws Exception { + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + GrpcCacheClient client = newClient(remoteOptions); + RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); + RemotePathResolver remotePathResolver = new SiblingExternalLayoutResolver(execRoot, true); + Digest fooDigest = DIGEST_UTIL.computeAsUtf8("foo-contents"); Digest barDigest = DIGEST_UTIL.computeAsUtf8("bar-contents"); Digest emptyDigest = DIGEST_UTIL.compute(new byte[0]); @@ -387,7 +416,7 @@ public void testDownloadAllResults() throws Exception { result.addOutputFilesBuilder().setPath("main/b/empty").setDigest(emptyDigest); result.addOutputFilesBuilder().setPath("main/a/bar").setDigest(barDigest).setIsExecutable(true); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("b/empty"))).isEqualTo(emptyDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar"))).isEqualTo(barDigest); @@ -421,10 +450,10 @@ public void testDownloadDirectory() throws Exception { quxDigest, "qux-contents"))); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputFilesBuilder().setPath("main/a/foo").setDigest(fooDigest); - result.addOutputDirectoriesBuilder().setPath("main/a/bar").setTreeDigest(barTreeDigest); + result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); + result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/qux"))).isEqualTo(quxDigest); @@ -444,9 +473,9 @@ public void testDownloadDirectoryEmpty() throws Exception { ImmutableMap.of(barTreeDigest, barTreeMessage.toByteString()))); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputDirectoriesBuilder().setPath("main/a/bar").setTreeDigest(barTreeDigest); + result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); assertThat(execRoot.getRelative("a/bar").isDirectory()).isTrue(); } @@ -486,10 +515,10 @@ public void testDownloadDirectoryNested() throws Exception { quxDigest, "qux-contents"))); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputFilesBuilder().setPath("main/a/foo").setDigest(fooDigest); - result.addOutputDirectoriesBuilder().setPath("main/a/bar").setTreeDigest(barTreeDigest); + result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); + result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar/wobble/qux"))).isEqualTo(quxDigest); @@ -709,7 +738,7 @@ private ActionResult uploadDirectory(RemoteCache remoteCache, List outputs Action action = Action.getDefaultInstance(); ActionKey actionKey = DIGEST_UTIL.computeActionKey(action); Command cmd = Command.getDefaultInstance(); - return remoteCache.upload(context, actionKey, action, cmd, execRoot, outputs, outErr); + return remoteCache.upload(context, remotePathResolver, actionKey, action, cmd, outputs, outErr); } @Test @@ -836,10 +865,10 @@ public void updateActionResult( ActionResult result = remoteCache.upload( context, + remotePathResolver, DIGEST_UTIL.asActionKey(actionDigest), action, command, - execRoot, ImmutableList.of(fooFile, barFile), outErr); ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -899,10 +928,10 @@ public void updateActionResult( ActionResult result = remoteCache.upload( context, + remotePathResolver, DIGEST_UTIL.asActionKey(actionDigest), action, command, - execRoot, ImmutableList.of(fooFile, barFile), outErr); ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1048,10 +1077,10 @@ public void onError(Throwable t) { .queryWriteStatus(any(), any()); remoteCache.upload( context, + remotePathResolver, actionKey, Action.getDefaultInstance(), Command.getDefaultInstance(), - execRoot, ImmutableList.of(fooFile, barFile, bazFile), outErr); // 4 times for the errors, 3 times for the successful uploads. diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index 8761b23de98f4e..4602a92b17353b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -58,6 +58,7 @@ import com.google.devtools.build.lib.remote.RemoteCache.UploadManifest; 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.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; @@ -103,6 +104,7 @@ public class RemoteCacheTests { @Mock private OutputFilesLocker outputFilesLocker; private RemoteActionExecutionContext context; + private RemotePathResolver remotePathResolver; private FileSystem fs; private Path execRoot; ArtifactRoot artifactRoot; @@ -115,12 +117,12 @@ public class RemoteCacheTests { public void setUp() throws Exception { MockitoAnnotations.initMocks(this); RequestMetadata metadata = - TracingMetadataUtils.buildMetadata( - "none", "none", Digest.getDefaultInstance().getHash(), null); + TracingMetadataUtils.buildMetadata("none", "none", "action-id", null); context = RemoteActionExecutionContext.create(metadata); fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); execRoot = fs.getPath("/execroot/main"); execRoot.createDirectoryAndParents(); + remotePathResolver = RemotePathResolver.createDefault(execRoot); fakeFileCache = new FakeActionInputFileCache(execRoot); artifactRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "outputs"); artifactRoot.getRoot().asPath().createDirectoryAndParents(); @@ -143,7 +145,8 @@ public void uploadAbsoluteFileSymlinkAsFile() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(link)); Digest digest = digestUtil.compute(target); assertThat(um.getDigestToFile()).containsExactly(digest, link); @@ -165,7 +168,8 @@ public void uploadAbsoluteDirectorySymlinkAsDirectory() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(link)); Digest digest = digestUtil.compute(foo); assertThat(um.getDigestToFile()).containsExactly(digest, fs.getPath("/execroot/main/link/foo")); @@ -193,7 +197,8 @@ public void uploadRelativeFileSymlinkAsFile() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(link)); Digest digest = digestUtil.compute(target); assertThat(um.getDigestToFile()).containsExactly(digest, link); @@ -215,7 +220,8 @@ public void uploadRelativeDirectorySymlinkAsDirectory() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(link)); Digest digest = digestUtil.compute(foo); assertThat(um.getDigestToFile()).containsExactly(digest, fs.getPath("/execroot/main/link/foo")); @@ -243,7 +249,8 @@ public void uploadRelativeFileSymlink() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(link)); assertThat(um.getDigestToFile()).isEmpty(); @@ -264,7 +271,8 @@ public void uploadRelativeDirectorySymlink() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(link)); assertThat(um.getDigestToFile()).isEmpty(); @@ -282,7 +290,8 @@ public void uploadDanglingSymlinkError() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); IOException e = assertThrows(IOException.class, () -> um.addFiles(ImmutableList.of(link))); assertThat(e).hasMessageThat().contains("dangling"); assertThat(e).hasMessageThat().contains("/execroot/main/link"); @@ -299,7 +308,8 @@ public void uploadSymlinksNoAllowError() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ false); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + false); ExecException e = assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(link))); assertThat(e).hasMessageThat().contains("symbolic link"); assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload"); @@ -317,7 +327,8 @@ public void uploadAbsoluteFileSymlinkInDirectoryAsFile() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(dir)); Digest digest = digestUtil.compute(target); assertThat(um.getDigestToFile()).containsExactly(digest, link); @@ -349,7 +360,8 @@ public void uploadAbsoluteDirectorySymlinkInDirectoryAsDirectory() throws Except UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(dir)); Digest digest = digestUtil.compute(foo); assertThat(um.getDigestToFile()) @@ -387,7 +399,8 @@ public void uploadRelativeFileSymlinkInDirectoryAsFile() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(dir)); Digest digest = digestUtil.compute(target); assertThat(um.getDigestToFile()).containsExactly(digest, link); @@ -419,7 +432,8 @@ public void uploadRelativeDirectorySymlinkInDirectoryAsDirectory() throws Except UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ false, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(dir)); Digest digest = digestUtil.compute(foo); assertThat(um.getDigestToFile()) @@ -457,7 +471,8 @@ public void uploadRelativeFileSymlinkInDirectory() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(dir)); assertThat(um.getDigestToFile()).isEmpty(); @@ -488,7 +503,8 @@ public void uploadRelativeDirectorySymlinkInDirectory() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); um.addFiles(ImmutableList.of(dir)); assertThat(um.getDigestToFile()).isEmpty(); @@ -516,7 +532,8 @@ public void uploadDanglingSymlinkInDirectoryError() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ true); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + true); IOException e = assertThrows(IOException.class, () -> um.addFiles(ImmutableList.of(dir))); assertThat(e).hasMessageThat().contains("dangling"); assertThat(e).hasMessageThat().contains("/execroot/dir/link"); @@ -535,7 +552,8 @@ public void uploadSymlinkInDirectoryNoAllowError() throws Exception { UploadManifest um = new UploadManifest( - digestUtil, result, execRoot, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ false); + digestUtil, remotePathResolver, result, /*uploadSymlinks=*/ true, /*allowSymlinks=*/ + false); ExecException e = assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(dir))); assertThat(e).hasMessageThat().contains("symbolic link"); assertThat(e).hasMessageThat().contains("dir/link"); @@ -546,9 +564,9 @@ public void uploadSymlinkInDirectoryNoAllowError() throws Exception { public void downloadRelativeFileSymlink() throws Exception { RemoteCache cache = newRemoteCache(); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputFileSymlinksBuilder().setPath("main/a/b/link").setTarget("../../foo"); + result.addOutputFileSymlinksBuilder().setPath("a/b/link").setTarget("../../foo"); // Doesn't check for dangling links, hence download succeeds. - cache.download(context, result.build(), execRoot, null, outputFilesLocker); + cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker); Path path = execRoot.getRelative("a/b/link"); assertThat(path.isSymbolicLink()).isTrue(); assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../../foo")); @@ -559,9 +577,9 @@ public void downloadRelativeFileSymlink() throws Exception { public void downloadRelativeDirectorySymlink() throws Exception { RemoteCache cache = newRemoteCache(); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputDirectorySymlinksBuilder().setPath("main/a/b/link").setTarget("foo"); + result.addOutputDirectorySymlinksBuilder().setPath("a/b/link").setTarget("foo"); // Doesn't check for dangling links, hence download succeeds. - cache.download(context, result.build(), execRoot, null, outputFilesLocker); + cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker); Path path = execRoot.getRelative("a/b/link"); assertThat(path.isSymbolicLink()).isTrue(); assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("foo")); @@ -579,9 +597,9 @@ public void downloadRelativeSymlinkInDirectory() throws Exception { .build(); Digest treeDigest = cache.addContents(context, tree.toByteArray()); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputDirectoriesBuilder().setPath("main/dir").setTreeDigest(treeDigest); + result.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); // Doesn't check for dangling links, hence download succeeds. - cache.download(context, result.build(), execRoot, null, outputFilesLocker); + cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker); Path path = execRoot.getRelative("dir/link"); assertThat(path.isSymbolicLink()).isTrue(); assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../foo")); @@ -596,7 +614,7 @@ public void downloadAbsoluteDirectorySymlinkError() throws Exception { IOException expected = assertThrows( IOException.class, - () -> cache.download(context, result.build(), execRoot, null, outputFilesLocker)); + () -> cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker)); assertThat(expected).hasMessageThat().contains("/abs/link"); assertThat(expected).hasMessageThat().contains("absolute path"); verify(outputFilesLocker).lock(); @@ -610,7 +628,7 @@ public void downloadAbsoluteFileSymlinkError() throws Exception { IOException expected = assertThrows( IOException.class, - () -> cache.download(context, result.build(), execRoot, null, outputFilesLocker)); + () -> cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker)); assertThat(expected).hasMessageThat().contains("/abs/link"); assertThat(expected).hasMessageThat().contains("absolute path"); verify(outputFilesLocker).lock(); @@ -631,7 +649,7 @@ public void downloadAbsoluteSymlinkInDirectoryError() throws Exception { IOException expected = assertThrows( IOException.class, - () -> cache.download(context, result.build(), execRoot, null, outputFilesLocker)); + () -> cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker)); assertThat(expected.getSuppressed()).isEmpty(); assertThat(expected).hasMessageThat().contains("dir/link"); assertThat(expected).hasMessageThat().contains("/foo"); @@ -649,14 +667,14 @@ public void downloadFailureMaintainsDirectories() throws Exception { Digest otherFileDigest = cache.addContents(context, "otherfile"); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputDirectoriesBuilder().setPath("main/outputdir").setTreeDigest(treeDigest); + result.addOutputDirectoriesBuilder().setPath("outputdir").setTreeDigest(treeDigest); result.addOutputFiles( - OutputFile.newBuilder().setPath("main/outputdir/outputfile").setDigest(outputFileDigest)); + OutputFile.newBuilder().setPath("outputdir/outputfile").setDigest(outputFileDigest)); result.addOutputFiles( - OutputFile.newBuilder().setPath("main/otherfile").setDigest(otherFileDigest)); + OutputFile.newBuilder().setPath("otherfile").setDigest(otherFileDigest)); assertThrows( BulkTransferException.class, - () -> cache.download(context, result.build(), execRoot, null, outputFilesLocker)); + () -> cache.download(context, remotePathResolver, result.build(), null, outputFilesLocker)); assertThat(cache.getNumFailedDownloads()).isEqualTo(1); assertThat(execRoot.getRelative("outputdir").exists()).isTrue(); assertThat(execRoot.getRelative("outputdir/outputfile").exists()).isFalse(); @@ -690,7 +708,7 @@ public void onErrorWaitForRemainingDownloadsToComplete() throws Exception { BulkTransferException.class, () -> cache.download( - context, result, execRoot, new FileOutErr(stdout, stderr), outputFilesLocker)); + context, remotePathResolver, result, new FileOutErr(stdout, stderr), outputFilesLocker)); assertThat(downloadException.getSuppressed()).hasLength(1); assertThat(cache.getNumSuccessfulDownloads()).isEqualTo(2); assertThat(cache.getNumFailedDownloads()).isEqualTo(1); @@ -722,7 +740,7 @@ public void downloadWithMultipleErrorsAddsThemAsSuppressed() throws Exception { BulkTransferException.class, () -> cache.download( - context, result, execRoot, new FileOutErr(stdout, stderr), outputFilesLocker)); + context, remotePathResolver, result, new FileOutErr(stdout, stderr), outputFilesLocker)); assertThat(e.getSuppressed()).hasLength(2); assertThat(e.getSuppressed()[0]).isInstanceOf(IOException.class); @@ -754,7 +772,7 @@ public void downloadWithDuplicateIOErrorsDoesNotSuppress() throws Exception { BulkTransferException.class, () -> cache.download( - context, result, execRoot, new FileOutErr(stdout, stderr), outputFilesLocker)); + context, remotePathResolver, result, new FileOutErr(stdout, stderr), outputFilesLocker)); for (Throwable t : downloadException.getSuppressed()) { assertThat(t).isInstanceOf(IOException.class); @@ -786,7 +804,7 @@ public void downloadWithDuplicateInterruptionsDoesNotSuppress() throws Exception InterruptedException.class, () -> cache.download( - context, result, execRoot, new FileOutErr(stdout, stderr), outputFilesLocker)); + context, remotePathResolver, result, new FileOutErr(stdout, stderr), outputFilesLocker)); assertThat(e.getSuppressed()).isEmpty(); assertThat(Throwables.getRootCause(e)).hasMessageThat().isEqualTo("reused interruption"); @@ -815,7 +833,7 @@ public void testDownloadWithStdoutStderrOnSuccess() throws Exception { .setStderrDigest(digestStderr) .build(); - cache.download(context, result, execRoot, spyOutErr, outputFilesLocker); + cache.download(context, remotePathResolver, result, spyOutErr, outputFilesLocker); verify(spyOutErr, Mockito.times(2)).childOutErr(); verify(spyChildOutErr).clearOut(); @@ -858,7 +876,7 @@ public void testDownloadWithStdoutStderrOnFailure() throws Exception { .build(); assertThrows( BulkTransferException.class, - () -> cache.download(context, result, execRoot, spyOutErr, outputFilesLocker)); + () -> cache.download(context, remotePathResolver, result, spyOutErr, outputFilesLocker)); verify(spyOutErr, Mockito.times(2)).childOutErr(); verify(spyChildOutErr).clearOut(); verify(spyChildOutErr).clearErr(); @@ -886,8 +904,8 @@ public void testDownloadClashes() throws Exception { ActionResult r = ActionResult.newBuilder() .setExitCode(0) - .addOutputFiles(OutputFile.newBuilder().setPath("main/outputs/foo.tmp").setDigest(d1)) - .addOutputFiles(OutputFile.newBuilder().setPath("main/outputs/foo").setDigest(d2)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/foo.tmp").setDigest(d1)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/foo").setDigest(d2)) .build(); Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "foo.tmp"); @@ -895,7 +913,7 @@ public void testDownloadClashes() throws Exception { // act - remoteCache.download(context, r, execRoot, new FileOutErr(), outputFilesLocker); + remoteCache.download(context, remotePathResolver, r, new FileOutErr(), outputFilesLocker); // assert @@ -917,8 +935,8 @@ public void testDownloadMinimalFiles() throws Exception { ActionResult r = ActionResult.newBuilder() .setExitCode(0) - .addOutputFiles(OutputFile.newBuilder().setPath("main/outputs/file1").setDigest(d1)) - .addOutputFiles(OutputFile.newBuilder().setPath("main/outputs/file2").setDigest(d2)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file1").setDigest(d1)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file2").setDigest(d2)) .build(); Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1"); @@ -930,12 +948,11 @@ public void testDownloadMinimalFiles() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(a1, a2), /* inMemoryOutputPath= */ null, new FileOutErr(), - execRoot, injector, outputFilesLocker); @@ -976,7 +993,7 @@ public void testDownloadMinimalDirectory() throws Exception { ActionResult.newBuilder() .setExitCode(0) .addOutputDirectories( - OutputDirectory.newBuilder().setPath("main/outputs/dir").setTreeDigest(dt)) + OutputDirectory.newBuilder().setPath("outputs/dir").setTreeDigest(dt)) .build(); SpecialArtifact dir = @@ -993,12 +1010,11 @@ public void testDownloadMinimalDirectory() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(dir), /* inMemoryOutputPath= */ null, new FileOutErr(), - execRoot, injector, outputFilesLocker); @@ -1069,12 +1085,11 @@ public void testDownloadMinimalDirectoryFails() throws Exception { () -> remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(dir), /* inMemoryOutputPath= */ null, new FileOutErr(), - execRoot, injector, outputFilesLocker)); assertThat(e.getSuppressed()).hasLength(1); @@ -1104,12 +1119,11 @@ public void testDownloadMinimalWithStdoutStderr() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(), /* inMemoryOutputPath= */ null, outErr, - execRoot, injector, outputFilesLocker); @@ -1135,8 +1149,8 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { ActionResult r = ActionResult.newBuilder() .setExitCode(0) - .addOutputFiles(OutputFile.newBuilder().setPath("main/outputs/file1").setDigest(d1)) - .addOutputFiles(OutputFile.newBuilder().setPath("main/outputs/file2").setDigest(d2)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file1").setDigest(d1)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file2").setDigest(d2)) .build(); Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1"); Artifact a2 = ActionsTestUtil.createArtifact(artifactRoot, "file2"); @@ -1148,12 +1162,11 @@ public void testDownloadMinimalWithInMemoryOutput() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(a1, a2), inMemoryOutputPathFragment, new FileOutErr(), - execRoot, injector, outputFilesLocker); @@ -1189,12 +1202,11 @@ public void testDownloadMinimalWithMissingInMemoryOutput() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(a1), inMemoryOutputPathFragment, new FileOutErr(), - execRoot, injector, outputFilesLocker); @@ -1278,13 +1290,13 @@ public void testDownloadDirectory() throws Exception { cas.put(barTreeDigest, barTreeMessage.toByteArray()); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputFilesBuilder().setPath("main/a/foo").setDigest(fooDigest); - result.addOutputDirectoriesBuilder().setPath("main/a/bar").setTreeDigest(barTreeDigest); + result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); + result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); // act RemoteCache remoteCache = newRemoteCache(cas); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); // assert assertThat(digestUtil.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); @@ -1304,12 +1316,12 @@ public void testDownloadEmptyDirectory() throws Exception { map.put(barTreeDigest, barTreeMessage.toByteArray()); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputDirectoriesBuilder().setPath("main/a/bar").setTreeDigest(barTreeDigest); + result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); // act RemoteCache remoteCache = newRemoteCache(map); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); // assert assertThat(execRoot.getRelative("a/bar").isDirectory()).isTrue(); @@ -1348,13 +1360,13 @@ public void testDownloadNestedDirectory() throws Exception { map.put(quxDigest, "qux-contents".getBytes(UTF_8)); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputFilesBuilder().setPath("main/a/foo").setDigest(fooDigest); - result.addOutputDirectoriesBuilder().setPath("main/a/bar").setTreeDigest(barTreeDigest); + result.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); + result.addOutputDirectoriesBuilder().setPath("a/bar").setTreeDigest(barTreeDigest); // act RemoteCache remoteCache = newRemoteCache(map); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); // assert assertThat(digestUtil.compute(execRoot.getRelative("a/foo"))).isEqualTo(fooDigest); @@ -1400,12 +1412,12 @@ public void testDownloadDirectoryWithSameHash() throws Exception { map.put(treeDigest, tree.toByteArray()); ActionResult.Builder result = ActionResult.newBuilder(); - result.addOutputDirectoriesBuilder().setPath("main/a/").setTreeDigest(treeDigest); + result.addOutputDirectoriesBuilder().setPath("a/").setTreeDigest(treeDigest); // act RemoteCache remoteCache = newRemoteCache(map); remoteCache.download( - context, result.build(), execRoot, null, /* outputFilesLocker= */ () -> {}); + context, remotePathResolver, result.build(), null, /* outputFilesLocker= */ () -> {}); // assert assertThat(digestUtil.compute(execRoot.getRelative("a/bar/foo/file"))).isEqualTo(fileDigest); @@ -1448,10 +1460,10 @@ public void testUploadDirectory() throws Exception { ActionResult result = remoteCache.upload( context, + remotePathResolver, digestUtil.asActionKey(actionDigest), action, cmd, - execRoot, ImmutableList.of(fooFile, barDir), new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); @@ -1485,10 +1497,10 @@ public void testUploadEmptyDirectory() throws Exception { ActionResult result = remoteCache.upload( context, + remotePathResolver, actionDigest, action, cmd, - execRoot, ImmutableList.of(barDir), new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); @@ -1542,10 +1554,10 @@ public void testUploadNestedDirectory() throws Exception { ActionResult result = remoteCache.upload( context, + remotePathResolver, actionDigest, action, cmd, - execRoot, ImmutableList.of(barDir), new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index 6962424956faa7..52c90933251f2d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -60,6 +60,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.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -208,7 +209,8 @@ private RemoteSpawnCache remoteSpawnCacheWithOptions(RemoteOptions options) { COMMAND_ID, reporter, digestUtil, - /* filesToDownload= */ ImmutableSet.of()); + /* filesToDownload= */ ImmutableSet.of(), + RemotePathResolver.createDefault(execRoot)); } @Before @@ -266,20 +268,20 @@ public Void answer(InvocationOnMock invocation) { } }) .when(remoteCache) - .download(any(), eq(actionResult), eq(execRoot), eq(outErr), any()); + .download(any(), any(), eq(actionResult), eq(outErr), any()); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isTrue(); SpawnResult result = entry.getResult(); // All other methods on RemoteActionCache have side effects, so we verify all of them. - verify(remoteCache).download(any(), eq(actionResult), eq(execRoot), eq(outErr), any()); + verify(remoteCache).download(any(), any(), eq(actionResult), eq(outErr), any()); verify(remoteCache, never()) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), any(Collection.class), any(FileOutErr.class)); assertThat(result.setupSuccess()).isTrue(); @@ -317,20 +319,20 @@ public Void answer(InvocationOnMock invocation) { .when(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); entry.store(result); verify(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); assertThat(progressUpdates) @@ -495,10 +497,10 @@ public void failedActionsAreNotUploaded() throws Exception { verify(remoteCache, never()) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); assertThat(progressUpdates) @@ -521,10 +523,10 @@ public void printWarningIfUploadFails() throws Exception { .when(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); @@ -532,10 +534,10 @@ public void printWarningIfUploadFails() throws Exception { verify(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); @@ -580,20 +582,20 @@ public Void answer(InvocationOnMock invocation) { .when(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); entry.store(result); verify(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); @@ -629,7 +631,7 @@ public ActionResult answer(InvocationOnMock invocation) { }); doThrow(new CacheNotFoundException(digest)) .when(remoteCache) - .download(any(), eq(actionResult), eq(execRoot), eq(outErr), any()); + .download(any(), any(), eq(actionResult), eq(outErr), any()); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isFalse(); @@ -655,20 +657,20 @@ public Void answer(InvocationOnMock invocation) { .when(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); entry.store(result); verify(remoteCache) .upload( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionKey.class), any(Action.class), any(Command.class), - any(Path.class), eq(outputFiles), eq(outErr)); assertThat(progressUpdates) @@ -709,7 +711,7 @@ public void testDownloadMinimal() throws Exception { assertThat(cacheHandle.hasResult()).isTrue(); assertThat(cacheHandle.getResult().exitCode()).isEqualTo(0); verify(remoteCache) - .downloadMinimal(any(), any(), any(), anyCollection(), any(), any(), any(), any(), any()); + .downloadMinimal(any(), any(), any(), anyCollection(), any(), any(), any(), any()); } @Test @@ -726,7 +728,7 @@ public void testDownloadMinimalIoError() throws Exception { any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false))) .thenReturn(success); when(remoteCache.downloadMinimal( - any(), any(), any(), anyCollection(), any(), any(), any(), any(), any())) + any(), any(), any(), anyCollection(), any(), any(), any(), any())) .thenThrow(downloadFailure); // act @@ -735,7 +737,7 @@ public void testDownloadMinimalIoError() throws Exception { // assert assertThat(cacheHandle.hasResult()).isFalse(); verify(remoteCache) - .downloadMinimal(any(), any(), any(), anyCollection(), any(), any(), any(), any(), any()); + .downloadMinimal(any(), any(), any(), anyCollection(), any(), any(), any(), any()); assertThat(eventHandler.getEvents().size()).isEqualTo(1); Event evt = eventHandler.getEvents().get(0); assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index dbe72651aa948e..04708717f70f8f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -82,6 +82,7 @@ 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.RemoteExecutionClient; +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.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -145,7 +146,7 @@ public class RemoteSpawnRunnerTest { // The action key of the Spawn returned by newSimpleSpawn(). private final String simpleActionId = - "b9a727771337fd8ce54821f4805e2d451c4739e92fec6f8ecdb18ff9d1983b27"; + "eb45b20cc979d504f96b9efc9a08c48103c6f017afa09c0df5c70a5f92a98ea8"; @Before public final void setUp() throws Exception { @@ -397,8 +398,8 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { verify(cache, never()) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionResult.class), - any(Path.class), eq(outErr), any()); } @@ -688,8 +689,8 @@ public void testNonHumanReadableServerLogsNotSaved() throws Exception { verify(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(result), - eq(execRoot), any(FileOutErr.class), any()); verify(cache, never()) @@ -728,8 +729,8 @@ public void testServerLogsNotSavedForSuccessfulAction() throws Exception { verify(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(result), - eq(execRoot), any(FileOutErr.class), any()); verify(cache, never()) @@ -754,8 +755,8 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { .when(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(cachedResult), - any(Path.class), any(FileOutErr.class), any()); ActionResult execResult = ActionResult.newBuilder().setExitCode(31).build(); @@ -769,8 +770,8 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { .when(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(execResult), - any(Path.class), any(FileOutErr.class), any()); @@ -818,16 +819,16 @@ public void resultsDownloadFailureTriggersRemoteExecutionWithSkipCacheLookup() t .when(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(cachedResult), - any(Path.class), any(FileOutErr.class), any()); doNothing() .when(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(execResult), - any(Path.class), any(FileOutErr.class), any()); @@ -895,8 +896,8 @@ public void testRemoteExecutionTimeout() throws Exception { verify(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(cachedResult), - eq(execRoot), any(FileOutErr.class), any()); } @@ -945,8 +946,8 @@ public void testRemoteExecutionTimeoutDoesNotTriggerFallback() throws Exception verify(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(cachedResult), - eq(execRoot), any(FileOutErr.class), any()); verify(localRunner, never()).exec(eq(spawn), eq(policy)); @@ -990,8 +991,8 @@ public void testRemoteExecutionCommandFailureDoesNotTriggerFallback() throws Exc verify(cache, never()) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(cachedResult), - eq(execRoot), any(FileOutErr.class), any()); verify(localRunner, never()).exec(eq(spawn), eq(policy)); @@ -1075,7 +1076,8 @@ private void testParamFilesAreMaterializedForFlag(String flag) throws Exception retryService, digestUtil, logDir, - /* filesToDownload= */ ImmutableSet.of()); + /* filesToDownload= */ ImmutableSet.of(), + RemotePathResolver.createDefault(execRoot)); ExecuteResponse succeeded = ExecuteResponse.newBuilder() @@ -1145,13 +1147,12 @@ public void testDownloadMinimalOnCacheHit() throws Exception { any(), any(), any(), - any(), any()); verify(cache, never()) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionResult.class), - any(Path.class), eq(outErr), any()); } @@ -1192,13 +1193,12 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { any(), any(), any(), - any(), any()); verify(cache, never()) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionResult.class), - any(Path.class), eq(outErr), any()); } @@ -1223,7 +1223,6 @@ public void testDownloadMinimalIoError() throws Exception { any(), any(), any(), - any(), any())) .thenThrow(downloadFailure); @@ -1246,13 +1245,12 @@ public void testDownloadMinimalIoError() throws Exception { any(), any(), any(), - any(), any()); verify(cache, never()) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), any(ActionResult.class), - any(Path.class), eq(outErr), any()); } @@ -1285,10 +1283,10 @@ public void testDownloadTopLevel() throws Exception { assertThat(result.status()).isEqualTo(Status.SUCCESS); // assert - verify(cache).download(any(), eq(succeededAction), any(Path.class), eq(outErr), any()); + verify(cache).download(any(), any(), eq(succeededAction), eq(outErr), any()); verify(cache, never()) .downloadMinimal( - any(), any(), eq(succeededAction), anyCollection(), any(), any(), any(), any(), any()); + any(), any(), eq(succeededAction), anyCollection(), any(), any(), any(), any()); } @Test @@ -1596,6 +1594,7 @@ private RemoteSpawnRunner newSpawnRunner( retryService, digestUtil, logDir, - topLevelOutputs); + topLevelOutputs, + RemotePathResolver.createDefault(execRoot)); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index ebb1f37c4b4b87..b87f98c662efab 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -70,6 +70,7 @@ import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; +import com.google.devtools.build.lib.remote.common.RemotePathResolver; import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -309,7 +310,8 @@ public int maxConcurrency() { retryService, DIGEST_UTIL, logDir, - /* filesToDownload= */ ImmutableSet.of()); + /* filesToDownload= */ ImmutableSet.of(), + RemotePathResolver.createDefault(execRoot)); inputDigest = fakeFileCache.createScratchInput(simpleSpawn.getInputFiles().getSingleton(), "xyz"); @@ -321,8 +323,7 @@ public int maxConcurrency() { .setName("VARIABLE") .setValue("value") .build()) - .addAllOutputFiles(ImmutableList.of("main/bar", "main/foo")) - .setWorkingDirectory("main") + .addAllOutputFiles(ImmutableList.of("bar", "foo")) .build(); cmdDigest = DIGEST_UTIL.compute(command); channel.release(); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 2830e52b4d74a0..027fda69613bab 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -39,6 +39,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.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.shell.AbnormalTerminationException; @@ -265,9 +266,12 @@ private ActionResult execute( throw StatusUtils.notFoundError(e.getMissingDigest()); } + Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory()); + FileSystemUtils.createDirectoryAndParents(workingDirectory); + List outputs = new ArrayList<>(command.getOutputFilesList().size()); for (String output : command.getOutputFilesList()) { - Path file = execRoot.getRelative(output); + Path file = workingDirectory.getRelative(output); if (file.exists()) { throw new FileAlreadyExistsException("Output file already exists: " + file); } @@ -275,7 +279,7 @@ private ActionResult execute( outputs.add(file); } for (String output : command.getOutputDirectoriesList()) { - Path file = execRoot.getRelative(output); + Path file = workingDirectory.getRelative(output); if (file.exists()) { throw new FileAlreadyExistsException("Output directory/file already exists: " + file); } @@ -283,9 +287,6 @@ private ActionResult execute( outputs.add(file); } - Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory()); - FileSystemUtils.createDirectoryAndParents(workingDirectory); - // TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that // implementation instead of copying it. com.google.devtools.build.lib.shell.Command cmd = @@ -344,7 +345,9 @@ private ActionResult execute( ActionResult result = null; try { result = - cache.upload(context, actionKey, action, command, execRoot, outputs, outErr, exitCode); + cache.upload(context, RemotePathResolver.createDefault(workingDirectory), actionKey, + action, command, outputs, outErr, + exitCode); } catch (ExecException e) { if (errStatus == null) { errStatus =