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..a9586c4afa2696 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; @@ -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.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> 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.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> 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 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()); 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 ba8331f612aff6..384f4b2d21f00f 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 29f89d47cff5fd..2d8a3b98eeb1ed 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); } 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 00fd0fb30c1a04..53d496d08899f8 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.getExecPath()); 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(); } 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..3960ae31c484ce --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -0,0 +1,165 @@ +// 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; + + /** @return the output path for given {@link PathFragment} which a relative to {@code execRoot}. */ + String resolveOutputPath(PathFragment execPath); + + /** + * Resolves the {@link Path} of an output file. + * + * @param outputPath the return value of {@link #resolveOutputPath(PathFragment)}. + */ + Path resolveExecPath(String outputPath); + + /** @return the {@link Path} of an output file by {@link ActionInput}. */ + default Path getExecPath(ActionInput actionInput) { + String outputPath = resolveOutputPath(actionInput.getExecPath()); + return resolveExecPath(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(PathFragment execPath) { + return execPath.toString(); + } + + @Override + public Path resolveExecPath(String outputPath) { + return execRoot.getRelative(outputPath); + } + + @Override + public Path getExecPath(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; + + @Override + public String getWorkingDirectory() { + return execRoot.getBaseName(); + } + + private PathFragment getWorkingDirectoryPathFragment() { + return PathFragment.create(checkNotNull(getWorkingDirectory())); + } + + public SiblingExternalLayoutResolver(Path execRoot, + boolean incompatibleRemoteOutputPathsRelativeToInputRoot) { + this.execRoot = execRoot; + this.incompatibleRemoteOutputPathsRelativeToInputRoot = incompatibleRemoteOutputPathsRelativeToInputRoot; + } + + + @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(getWorkingDirectoryPathFragment()); + } + + @Override + public String resolveOutputPath(PathFragment execPath) { + if (incompatibleRemoteOutputPathsRelativeToInputRoot) { + return getWorkingDirectoryPathFragment().getRelative(execPath).toString(); + } else { + return execPath.toString(); + } + } + + @Override + public Path resolveExecPath(String outputPath) { + if (incompatibleRemoteOutputPathsRelativeToInputRoot) { + return execRoot.getRelative( + PathFragment.create(outputPath).relativeTo(getWorkingDirectoryPathFragment())); + } else { + return execRoot.getRelative(outputPath); + } + } + + @Override + public Path getExecPath(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 4503f741dbbfae..43b12b51ec420b 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 @@ -28,6 +28,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; @@ -61,6 +62,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 9df464991f97cb..6cac3ed47e66b2 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 @@ -253,6 +253,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 d775c8bdde3f55..6c07b74a7ec48b 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"); @@ -375,6 +379,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]); @@ -386,7 +415,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); @@ -420,10 +449,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); @@ -443,9 +472,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(); } @@ -485,10 +514,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); 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 9a3ba38864d3db..8f9efd14ce4774 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,11 +117,12 @@ public class RemoteCacheTests { public void setUp() throws Exception { MockitoAnnotations.initMocks(this); RequestMetadata metadata = - TracingMetadataUtils.buildMetadata("none", "none", Digest.getDefaultInstance().getHash()); + TracingMetadataUtils.buildMetadata("none", "none", "action-id"); 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(); @@ -545,9 +548,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")); @@ -558,9 +561,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")); @@ -578,9 +581,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")); @@ -595,7 +598,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(); @@ -609,7 +612,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(); @@ -630,7 +633,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"); @@ -648,14 +651,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(); @@ -689,7 +692,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); @@ -721,7 +724,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); @@ -753,7 +756,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); @@ -785,7 +788,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"); @@ -814,7 +817,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(); @@ -857,7 +860,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(); @@ -885,8 +888,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"); @@ -894,7 +897,7 @@ public void testDownloadClashes() throws Exception { // act - remoteCache.download(context, r, execRoot, new FileOutErr(), outputFilesLocker); + remoteCache.download(context, remotePathResolver, r, new FileOutErr(), outputFilesLocker); // assert @@ -916,8 +919,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"); @@ -929,12 +932,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); @@ -975,7 +977,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 = @@ -992,12 +994,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); @@ -1068,12 +1069,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); @@ -1103,12 +1103,11 @@ public void testDownloadMinimalWithStdoutStderr() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(), /* inMemoryOutputPath= */ null, outErr, - execRoot, injector, outputFilesLocker); @@ -1134,8 +1133,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"); @@ -1147,12 +1146,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); @@ -1188,12 +1186,11 @@ public void testDownloadMinimalWithMissingInMemoryOutput() throws Exception { InMemoryOutput inMemoryOutput = remoteCache.downloadMinimal( context, - "action-id", + remotePathResolver, r, ImmutableList.of(a1), inMemoryOutputPathFragment, new FileOutErr(), - execRoot, injector, outputFilesLocker); @@ -1277,13 +1274,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); @@ -1303,12 +1300,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(); @@ -1347,13 +1344,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); @@ -1399,12 +1396,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); 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..0956126f77d522 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,13 +268,13 @@ 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), @@ -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(); @@ -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();