diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java index 0a55056e9dc681..7c8f959fdc8677 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java @@ -246,4 +246,8 @@ public enum WorkerProtocolFormat { * followed by a {@code SIGKILL} after a grace period). */ public static final String GRACEFUL_TERMINATION = "supports-graceful-termination"; + + /** Requires the execution service do NOT share caches across different workspace. */ + public static final String DIFFERENTIATE_WORKSPACE_CACHE = + "internal-differentiate-workspace-cache"; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD index be0d6a55cb252f..f11af60338a2cf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD @@ -43,6 +43,7 @@ java_library( srcs = ["PlatformUtils.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/protobuf:failure_details_java_proto", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index 621a598b25aea4..79227355b01b5d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -19,6 +19,7 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Ordering; +import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -101,6 +102,16 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem } } + String workspace = + spawn.getExecutionInfo().get(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE); + if (workspace != null) { + platformBuilder.addProperties( + Property.newBuilder() + .setName("bazel-differentiate-workspace-cache") + .setValue(workspace) + .build()); + } + sortPlatformProperties(platformBuilder); return platformBuilder.build(); } 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..b3ff7ded638886 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.SiblingRepositoryLayoutResolver; 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,22 @@ 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 SiblingRepositoryLayoutResolver( + 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 +128,8 @@ public void registerRemoteSpawnStrategyIfApplicable( retryScheduler, digestUtil, logDir, - filesToDownload); + filesToDownload, + createRemotePathResolver()); registryBuilder.registerStrategy( new RemoteSpawnStrategy(env.getExecRoot(), spawnRunner, verboseFailures), "remote"); } @@ -129,7 +150,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..ce008c1668ef5d 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,17 @@ 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 +152,27 @@ 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 +183,8 @@ private void uploadOutputs( UploadManifest manifest = new UploadManifest( digestUtil, + remotePathResolver, result, - execRoot, options.incompatibleRemoteSymlinks, options.allowSymlinkUpload); manifest.addFiles(files); @@ -309,15 +318,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 +358,12 @@ public void download( try { // Delete any (partially) downloaded output files. for (OutputFile file : result.getOutputFilesList()) { - toTmpDownloadPath(inputRoot.getRelative(file.getPath())).delete(); + toTmpDownloadPath(remotePathResolver.outputPathToLocalPath(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.outputPathToLocalPath(directory.getPath()).deleteTreesBelow(); } if (tmpOutErr != null) { tmpOutErr.clearOut(); @@ -566,7 +572,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 +582,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 +596,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,8 +613,8 @@ public InMemoryOutput downloadMinimal( Digest inMemoryOutputDigest = null; for (ActionInput output : outputs) { if (inMemoryOutputPath != null && output.getExecPath().equals(inMemoryOutputPath)) { - Path p = execRoot.getRelative(output.getExecPath()); - FileMetadata m = metadata.file(p); + Path localPath = remotePathResolver.outputPathToLocalPath(output); + FileMetadata m = metadata.file(localPath); if (m == null) { // A declared output wasn't created. Ignore it here. SkyFrame will fail if not all // outputs were created. @@ -624,7 +624,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 +647,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.outputPathToLocalPath(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 +676,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 +694,7 @@ private void injectRemoteArtifact( DigestUtil.toBinaryDigest(outputMetadata.digest()), outputMetadata.digest().getSizeBytes(), /*locationIndex=*/ 1, - actionId, + context.getRequestMetadata().getActionId(), outputMetadata.isExecutable())); } } @@ -727,14 +728,16 @@ 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.outputPathToLocalPath(dir.getPath()), Futures.transform( downloadBlob(context, dir.getTreeDigest()), (treeBytes) -> { @@ -764,12 +767,10 @@ private ActionResultMetadata parseActionResultMetadata( ImmutableMap.Builder files = ImmutableMap.builder(); for (OutputFile outputFile : actionResult.getOutputFilesList()) { + Path localPath = remotePathResolver.outputPathToLocalPath(outputFile.getPath()); files.put( - inputRoot.getRelative(outputFile.getPath()), - new FileMetadata( - inputRoot.getRelative(outputFile.getPath()), - outputFile.getDigest(), - outputFile.getIsExecutable())); + localPath, + new FileMetadata(localPath, outputFile.getDigest(), outputFile.getIsExecutable())); } ImmutableMap.Builder symlinks = ImmutableMap.builder(); @@ -778,10 +779,9 @@ private ActionResultMetadata parseActionResultMetadata( actionResult.getOutputFileSymlinksList(), actionResult.getOutputDirectorySymlinksList()); for (OutputSymlink symlink : outputSymlinks) { + Path localPath = remotePathResolver.outputPathToLocalPath(symlink.getPath()); symlinks.put( - inputRoot.getRelative(symlink.getPath()), - new SymlinkMetadata( - inputRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget()))); + localPath, new SymlinkMetadata(localPath, PathFragment.create(symlink.getTarget()))); } return new ActionResultMetadata(files.build(), symlinks.build(), directories.build()); @@ -790,8 +790,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 +805,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 +917,21 @@ public Digest getStderrDigest() { private void addFileSymbolicLink(Path file, PathFragment target) throws IOException { result .addOutputFileSymlinksBuilder() - .setPath(file.relativeTo(execRoot).getPathString()) + .setPath(remotePathResolver.localPathToOutputPath(file)) .setTarget(target.toString()); } private void addDirectorySymbolicLink(Path file, PathFragment target) throws IOException { result .addOutputDirectorySymlinksBuilder() - .setPath(file.relativeTo(execRoot).getPathString()) + .setPath(remotePathResolver.localPathToOutputPath(file)) .setTarget(target.toString()); } private void addFile(Digest digest, Path file) throws IOException { result .addOutputFilesBuilder() - .setPath(file.relativeTo(execRoot).getPathString()) + .setPath(remotePathResolver.localPathToOutputPath(file)) .setDigest(digest) .setIsExecutable(file.isExecutable()); @@ -949,7 +949,7 @@ private void addDirectory(Path dir) throws ExecException, IOException { if (result != null) { result .addOutputDirectoriesBuilder() - .setPath(dir.relativeTo(execRoot).getPathString()) + .setPath(remotePathResolver.localPathToOutputPath(dir)) .setTreeDigest(digest); } @@ -1017,7 +1017,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.localPathToOutputPath(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..f239fe3fb04163 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,21 @@ 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..a5a0e563671f8b 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( @@ -281,7 +279,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) try { return downloadAndFinalizeSpawnResult( remoteActionExecutionContext, - actionKey.getDigest().getHash(), cachedResult, /* cacheHit= */ true, spawn, @@ -382,7 +379,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) try { return downloadAndFinalizeSpawnResult( remoteActionExecutionContext, - actionKey.getDigest().getHash(), actionResult, reply.getCachedResult(), spawn, @@ -461,7 +457,6 @@ static void spawnMetricsAccounting( private SpawnResult downloadAndFinalizeSpawnResult( RemoteActionExecutionContext remoteActionExecutionContext, - String actionId, ActionResult actionResult, boolean cacheHit, Spawn spawn, @@ -483,8 +478,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 +490,11 @@ private SpawnResult downloadAndFinalizeSpawnResult( inMemoryOutput = remoteCache.downloadMinimal( remoteActionExecutionContext, - actionId, + remotePathResolver, actionResult, spawn.getOutputFiles(), inMemoryOutputPath, context.getFileOutErr(), - execRoot, context.getMetadataInjector(), context::lockOutputFiles); } @@ -636,8 +630,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 +720,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.localPathToOutputPath(output); if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { outputDirectories.add(pathString); } else { @@ -758,8 +747,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 +805,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..d0678200f0b327 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -0,0 +1,189 @@ +// 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; + +/** + * A {@link RemotePathResolver} is used to resolve input/output paths for remote execution from + * Bazel's internal path, or vice versa. + */ +public interface RemotePathResolver { + + /** + * Returns the {@code workingDirectory} for a remote action. Empty string if working directory is + * the input root. + */ + String getWorkingDirectory(); + + /** + * Returns 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 localPathToOutputPath(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 localPathToOutputPath(PathFragment execPath); + + /** Resolves the output path relative to input root for the {@link ActionInput}. */ + default String localPathToOutputPath(ActionInput actionInput) { + return localPathToOutputPath(actionInput.getExecPath()); + } + + /** + * Resolves the local {@link Path} of an output file. + * + * @param outputPath the return value of {@link #localPathToOutputPath(PathFragment)}. + */ + Path outputPathToLocalPath(String outputPath); + + /** Resolves the local {@link Path} for the {@link ActionInput}. */ + default Path outputPathToLocalPath(ActionInput actionInput) { + String outputPath = localPathToOutputPath(actionInput.getExecPath()); + return outputPathToLocalPath(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 { + + private final Path execRoot; + + public DefaultRemotePathResolver(Path execRoot) { + this.execRoot = execRoot; + } + + @Override + public String getWorkingDirectory() { + return ""; + } + + @Override + public SortedMap getInputMapping(SpawnExecutionContext context) + throws IOException { + return context.getInputMapping(PathFragment.EMPTY_FRAGMENT); + } + + @Override + public String localPathToOutputPath(Path path) { + return path.relativeTo(execRoot).getPathString(); + } + + @Override + public String localPathToOutputPath(PathFragment execPath) { + return execPath.getPathString(); + } + + @Override + public Path outputPathToLocalPath(String outputPath) { + return execRoot.getRelative(outputPath); + } + + @Override + public Path outputPathToLocalPath(ActionInput actionInput) { + return ActionInputHelper.toInputPath(actionInput, execRoot); + } + } + + /** + * A {@link RemotePathResolver} used when {@code --experimental_sibling_repository_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 SiblingRepositoryLayoutResolver implements RemotePathResolver { + + private final Path execRoot; + private final boolean incompatibleRemoteOutputPathsRelativeToInputRoot; + + public SiblingRepositoryLayoutResolver(Path execRoot) { + this(execRoot, /* incompatibleRemoteOutputPathsRelativeToInputRoot= */ false); + } + + public SiblingRepositoryLayoutResolver( + 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 localPathToOutputPath(Path path) { + return path.relativeTo(getBase()).getPathString(); + } + + @Override + public String localPathToOutputPath(PathFragment execPath) { + return localPathToOutputPath(execRoot.getRelative(execPath)); + } + + @Override + public Path outputPathToLocalPath(String outputPath) { + return getBase().getRelative(outputPath); + } + + @Override + public Path outputPathToLocalPath(ActionInput actionInput) { + return ActionInputHelper.toInputPath(actionInput, execRoot); + } + } +} 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..872fe3a8259ac7 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,19 @@ 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/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 5388813d5f6648..9c9f97e5e31e97 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -382,9 +382,7 @@ public void clearAdditionalInputs() { @Override public boolean discoversInputs() { - return shouldScanIncludes - || getDotdFile() != null - || featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES); + return shouldScanIncludes || getDotdFile() != null || shouldParseShowIncludes(); } @Override @@ -1427,7 +1425,7 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx ActionExecutionContext spawnContext; ShowIncludesFilter showIncludesFilterForStdout; ShowIncludesFilter showIncludesFilterForStderr; - if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) { + if (shouldParseShowIncludes()) { showIncludesFilterForStdout = new ShowIncludesFilter(getSourceFile().getFilename()); showIncludesFilterForStderr = new ShowIncludesFilter(getSourceFile().getFilename()); FileOutErr originalOutErr = actionExecutionContext.getFileOutErr(); @@ -1441,7 +1439,8 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx Spawn spawn; try { - spawn = createSpawn(actionExecutionContext.getClientEnv()); + spawn = + createSpawn(actionExecutionContext.getExecRoot(), actionExecutionContext.getClientEnv()); } finally { clearAdditionalInputs(); } @@ -1473,7 +1472,12 @@ protected byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalEx return null; } - protected Spawn createSpawn(Map clientEnv) throws ActionExecutionException { + protected boolean shouldParseShowIncludes() { + return featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES); + } + + protected Spawn createSpawn(Path execRoot, Map clientEnv) + throws ActionExecutionException { // Intentionally not adding {@link CppCompileAction#inputsForInvalidation}, those are not needed // for execution. NestedSetBuilder inputsBuilder = @@ -1486,7 +1490,8 @@ protected Spawn createSpawn(Map clientEnv) throws ActionExecutio } NestedSet inputs = inputsBuilder.build(); - ImmutableMap executionInfo = getExecutionInfo(); + ImmutableMap.Builder executionInfo = + ImmutableMap.builder().putAll(getExecutionInfo()); if (getDotdFile() != null && useInMemoryDotdFiles()) { /* * CppCompileAction does dotd file scanning locally inside the Bazel process and thus @@ -1496,13 +1501,18 @@ protected Spawn createSpawn(Map clientEnv) throws ActionExecutio * in-memory. We can do that via * {@link ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS}. */ - executionInfo = - ImmutableMap.builderWithExpectedSize(executionInfo.size() + 1) - .putAll(executionInfo) - .put( - ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS, - getDotdFile().getExecPathString()) - .build(); + executionInfo.put( + ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS, getDotdFile().getExecPathString()); + } + + if (shouldParseShowIncludes()) { + // Hack on Windows. The included headers dumped by cl.exe in stdout contain absolute paths. + // When compiling the file from different workspace, the shared cache will cause header + // dependency checking to fail. This was initially fixed by a hack (see + // https://github.com/bazelbuild/bazel/issues/9172 for more details), but is broken again due + // to cl/356735700. We require execution service to ignore caches from other workspace. + executionInfo.put( + ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE, execRoot.getBaseName()); } try { @@ -1510,7 +1520,7 @@ protected Spawn createSpawn(Map clientEnv) throws ActionExecutio this, ImmutableList.copyOf(getArguments()), getEnvironment(clientEnv), - executionInfo, + executionInfo.build(), inputs, getOutputs(), estimateResourceConsumptionLocal()); @@ -1865,7 +1875,7 @@ public ActionContinuationOrResult execute() .getOptions(BuildLanguageOptions.class) .experimentalSiblingRepositoryLayout; - if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) { + if (shouldParseShowIncludes()) { NestedSet discoveredInputs = discoverInputsFromShowIncludesFilters( execRoot, @@ -1905,7 +1915,7 @@ public ActionContinuationOrResult execute() private void copyTempOutErrToActionOutErr() throws ActionExecutionException { // If parse_showincludes feature is enabled, instead of parsing dotD file we parse the // output of cl.exe caused by /showIncludes option. - if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) { + if (shouldParseShowIncludes()) { try { FileOutErr tempOutErr = spawnExecutionContext.getFileOutErr(); FileOutErr outErr = actionExecutionContext.getFileOutErr(); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD index 83220dd452a409..9daa5157cd777f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/BUILD @@ -21,6 +21,7 @@ java_test( ], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:template_variable_info", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java index 52db06b984df2b..0b013fbdb81706 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java @@ -18,6 +18,7 @@ import build.bazel.remote.execution.v2.Platform; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -94,4 +95,24 @@ public void testParsePlatformSortsProperties_execProperties() throws Exception { // execProperties are sorted by key assertThat(PlatformUtils.getPlatformProto(s, remoteOptions())).isEqualTo(expected); } + + @Test + public void testGetPlatformProto_differentiateWorkspace() throws Exception { + Spawn s = + new SpawnBuilder("dummy") + .withExecutionInfo(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE, "aa") + .build(); + + Platform expected = + Platform.newBuilder() + .addProperties(Platform.Property.newBuilder().setName("a").setValue("1")) + .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) + .addProperties( + Platform.Property.newBuilder() + .setName("bazel-differentiate-workspace-cache") + .setValue("aa")) + .build(); + // execProperties are sorted by key + assertThat(PlatformUtils.getPlatformProto(s, remoteOptions())).isEqualTo(expected); + } } 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..3c59a8418fb11f 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.SiblingRepositoryLayoutResolver; 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 SiblingRepositoryLayoutResolver(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 1a74c29470a027..7836639534d8e0 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 @@ -57,6 +57,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; @@ -102,6 +103,7 @@ public class RemoteCacheTests { @Mock private OutputFilesLocker outputFilesLocker; private RemoteActionExecutionContext context; + private RemotePathResolver remotePathResolver; private FileSystem fs; private Path execRoot; ArtifactRoot artifactRoot; @@ -114,12 +116,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, "outputs"); artifactRoot.getRoot().asPath().createDirectoryAndParents(); @@ -142,7 +144,11 @@ 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); @@ -164,7 +170,11 @@ 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")); @@ -192,7 +202,11 @@ 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); @@ -214,7 +228,11 @@ 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")); @@ -242,7 +260,11 @@ 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(); @@ -263,7 +285,11 @@ 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(); @@ -281,7 +307,11 @@ 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"); @@ -298,7 +328,11 @@ 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"); @@ -316,7 +350,11 @@ 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); @@ -348,7 +386,11 @@ 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()) @@ -386,7 +428,11 @@ 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); @@ -418,7 +464,11 @@ 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()) @@ -456,7 +506,11 @@ 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(); @@ -487,7 +541,11 @@ 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(); @@ -515,7 +573,11 @@ 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"); @@ -534,7 +596,11 @@ 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"); @@ -545,9 +611,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 +624,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 +644,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 +661,9 @@ 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 +677,9 @@ 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 +700,9 @@ 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 +720,13 @@ 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)); - result.addOutputFiles( - OutputFile.newBuilder().setPath("main/otherfile").setDigest(otherFileDigest)); + OutputFile.newBuilder().setPath("outputdir/outputfile").setDigest(outputFileDigest)); + result.addOutputFiles(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 +760,11 @@ 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 +796,11 @@ 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 +832,11 @@ 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 +868,11 @@ 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 +901,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 +944,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 +972,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 +981,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 +1003,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 +1016,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 +1061,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 +1078,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 +1153,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 +1187,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 +1217,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 +1230,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 +1270,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 +1358,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 +1384,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 +1428,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 +1480,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); @@ -1447,10 +1528,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"))); @@ -1484,10 +1565,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"))); @@ -1541,10 +1622,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/RemotePathResolverTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java new file mode 100644 index 00000000000000..3805649a1e0bab --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java @@ -0,0 +1,137 @@ +// Copyright 2017 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; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +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.remote.common.RemotePathResolver; +import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingRepositoryLayoutResolver; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import java.util.SortedMap; +import java.util.TreeMap; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link RemotePathResolver} */ +@RunWith(JUnit4.class) +public class RemotePathResolverTest { + + private Path execRoot; + private SpawnExecutionContext spawnExecutionContext; + private ActionInput input; + + @Before + public void setup() throws Exception { + FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256); + execRoot = fs.getPath("/execroot/main"); + + input = ActionInputHelper.fromPath("foo"); + spawnExecutionContext = mock(SpawnExecutionContext.class); + when(spawnExecutionContext.getInputMapping(any())) + .thenAnswer( + invocationOnMock -> { + PathFragment baseDirectory = invocationOnMock.getArgument(0); + TreeMap inputMap = new TreeMap<>(); + inputMap.put(baseDirectory.getRelative(input.getExecPath()), input); + return inputMap; + }); + } + + @Test + public void getWorkingDirectory_default_isInputRoot() { + RemotePathResolver remotePathResolver = RemotePathResolver.createDefault(execRoot); + + String workingDirectory = remotePathResolver.getWorkingDirectory(); + + assertThat(workingDirectory).isEqualTo(""); + } + + @Test + public void getWorkingDirectory_sibling_isExecRootBaseName() { + RemotePathResolver remotePathResolver = new SiblingRepositoryLayoutResolver(execRoot); + + String workingDirectory = remotePathResolver.getWorkingDirectory(); + + assertThat(workingDirectory).isEqualTo("main"); + } + + @Test + public void getInputMapping_default_inputsRelativeToExecRoot() throws Exception { + RemotePathResolver remotePathResolver = RemotePathResolver.createDefault(execRoot); + + SortedMap inputs = + remotePathResolver.getInputMapping(spawnExecutionContext); + + assertThat(inputs).containsExactly(PathFragment.create("foo"), input); + } + + @Test + public void getInputMapping_sibling_inputsRelativeToInputRoot() throws Exception { + RemotePathResolver remotePathResolver = new SiblingRepositoryLayoutResolver(execRoot); + + SortedMap inputs = + remotePathResolver.getInputMapping(spawnExecutionContext); + + assertThat(inputs).containsExactly(PathFragment.create("main/foo"), input); + } + + @Test + public void convertPaths_default_relativeToWorkingDirectory() { + RemotePathResolver remotePathResolver = RemotePathResolver.createDefault(execRoot); + + String outputPath = remotePathResolver.localPathToOutputPath(PathFragment.create("bar")); + Path localPath = remotePathResolver.outputPathToLocalPath(outputPath); + + assertThat(outputPath).isEqualTo("bar"); + assertThat(localPath).isEqualTo(execRoot.getRelative("bar")); + } + + @Test + public void convertPaths_siblingCompatible_relativeToWorkingDirectory() { + RemotePathResolver remotePathResolver = + new SiblingRepositoryLayoutResolver( + execRoot, /* incompatibleRemoteOutputPathsRelativeToInputRoot= */ false); + + String outputPath = remotePathResolver.localPathToOutputPath(PathFragment.create("bar")); + Path localPath = remotePathResolver.outputPathToLocalPath(outputPath); + + assertThat(outputPath).isEqualTo("bar"); + assertThat(localPath).isEqualTo(execRoot.getRelative("bar")); + } + + @Test + public void localPathToOutputPath_siblingIncompatible_relativeToWorkingDirectory() { + RemotePathResolver remotePathResolver = + new SiblingRepositoryLayoutResolver( + execRoot, /* incompatibleRemoteOutputPathsRelativeToInputRoot= */ true); + + String outputPath = remotePathResolver.localPathToOutputPath(PathFragment.create("bar")); + Path localPath = remotePathResolver.outputPathToLocalPath(outputPath); + + assertThat(outputPath).isEqualTo("main/bar"); + assertThat(localPath).isEqualTo(execRoot.getRelative("bar")); + } +} 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 9d81fe8650c52f..b694f0141a91b4 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 @@ -81,6 +81,8 @@ 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.common.RemotePathResolver.SiblingRepositoryLayoutResolver; 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; @@ -144,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 { @@ -396,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()); } @@ -621,6 +623,45 @@ public void testHumanReadableServerLogsSavedForFailingAction() throws Exception verify(cache).downloadFile(any(RemoteActionExecutionContext.class), eq(logPath), eq(logDigest)); } + @Test + public void testHumanReadableServerLogsSavedForFailingActionWithSiblingRepositoryLayout() + throws Exception { + RemoteSpawnRunner runner = newSpawnRunner(new SiblingRepositoryLayoutResolver(execRoot)); + Digest logDigest = digestUtil.computeAsUtf8("bla"); + Path logPath = + logDir + .getRelative("b9a727771337fd8ce54821f4805e2d451c4739e92fec6f8ecdb18ff9d1983b27") + .getRelative("logname"); + when(executor.executeRemotely( + any(RemoteActionExecutionContext.class), + any(ExecuteRequest.class), + any(OperationObserver.class))) + .thenReturn( + ExecuteResponse.newBuilder() + .putServerLogs( + "logname", + LogFile.newBuilder().setHumanReadable(true).setDigest(logDigest).build()) + .setResult(ActionResult.newBuilder().setExitCode(31).build()) + .build()); + SettableFuture completed = SettableFuture.create(); + completed.set(null); + when(cache.downloadFile(any(RemoteActionExecutionContext.class), eq(logPath), eq(logDigest))) + .thenReturn(completed); + + Spawn spawn = newSimpleSpawn(); + SpawnExecutionContext policy = getSpawnContext(spawn); + + SpawnResult res = runner.exec(spawn, policy); + assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); + + verify(executor) + .executeRemotely( + any(RemoteActionExecutionContext.class), + any(ExecuteRequest.class), + any(OperationObserver.class)); + verify(cache).downloadFile(any(RemoteActionExecutionContext.class), eq(logPath), eq(logDigest)); + } + @Test public void testHumanReadableServerLogsSavedForFailingActionWithStatus() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); @@ -687,8 +728,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()) @@ -727,8 +768,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()) @@ -753,8 +794,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(); @@ -768,8 +809,8 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { .when(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(execResult), - any(Path.class), any(FileOutErr.class), any()); @@ -817,16 +858,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()); @@ -894,8 +935,8 @@ public void testRemoteExecutionTimeout() throws Exception { verify(cache) .download( any(RemoteActionExecutionContext.class), + any(RemotePathResolver.class), eq(cachedResult), - eq(execRoot), any(FileOutErr.class), any()); } @@ -944,8 +985,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)); @@ -989,8 +1030,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)); @@ -1074,7 +1115,8 @@ private void testParamFilesAreMaterializedForFlag(String flag) throws Exception retryService, digestUtil, logDir, - /* filesToDownload= */ ImmutableSet.of()); + /* filesToDownload= */ ImmutableSet.of(), + RemotePathResolver.createDefault(execRoot)); ExecuteResponse succeeded = ExecuteResponse.newBuilder() @@ -1144,13 +1186,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()); } @@ -1191,13 +1232,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()); } @@ -1222,7 +1262,6 @@ public void testDownloadMinimalIoError() throws Exception { any(), any(), any(), - any(), any())) .thenThrow(downloadFailure); @@ -1245,13 +1284,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()); } @@ -1284,10 +1322,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 @@ -1564,24 +1602,43 @@ private RemoteSpawnRunner newSpawnRunner() { /* verboseFailures= */ false, executor, /* reporter= */ null, - /* topLevelOutputs= */ ImmutableSet.of()); + /* topLevelOutputs= */ ImmutableSet.of(), + RemotePathResolver.createDefault(execRoot)); } private RemoteSpawnRunner newSpawnRunner(Reporter reporter) { return newSpawnRunner( - /* verboseFailures= */ false, executor, reporter, /* topLevelOutputs= */ ImmutableSet.of()); + /* verboseFailures= */ false, + executor, + reporter, + /* topLevelOutputs= */ ImmutableSet.of(), + RemotePathResolver.createDefault(execRoot)); } private RemoteSpawnRunner newSpawnRunner(ImmutableSet topLevelOutputs) { return newSpawnRunner( - /* verboseFailures= */ false, executor, /* reporter= */ null, topLevelOutputs); + /* verboseFailures= */ false, + executor, + /* reporter= */ null, + topLevelOutputs, + RemotePathResolver.createDefault(execRoot)); + } + + private RemoteSpawnRunner newSpawnRunner(RemotePathResolver remotePathResolver) { + return newSpawnRunner( + /* verboseFailures= */ false, + executor, + /* reporter= */ null, + /* topLevelOutputs= */ ImmutableSet.of(), + remotePathResolver); } private RemoteSpawnRunner newSpawnRunner( boolean verboseFailures, @Nullable RemoteExecutionClient executor, @Nullable Reporter reporter, - ImmutableSet topLevelOutputs) { + ImmutableSet topLevelOutputs, + RemotePathResolver remotePathResolver) { return new RemoteSpawnRunner( execRoot, remoteOptions, @@ -1595,6 +1652,7 @@ private RemoteSpawnRunner newSpawnRunner( retryService, digestUtil, logDir, - topLevelOutputs); + topLevelOutputs, + remotePathResolver); } } 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/test/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategyTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategyTest.java index 3b4d95d8f08c1e..8b2a63782ae2cd 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategyTest.java @@ -71,10 +71,11 @@ public void testInMemoryDotdFileAndExecutionRequirement() throws Exception { when(action.getDotdFile()).thenReturn(dotdFile); when(action.useInMemoryDotdFiles()).thenReturn(true); when(action.estimateResourceConsumptionLocal()).thenReturn(AbstractAction.DEFAULT_RESOURCE_SET); - when(action.createSpawn(any())).thenCallRealMethod(); + when(action.shouldParseShowIncludes()).thenReturn(false); + when(action.createSpawn(any(), any())).thenCallRealMethod(); // act - Spawn spawn = action.createSpawn(ImmutableMap.of()); + Spawn spawn = action.createSpawn(execRoot, ImmutableMap.of()); ImmutableMap execInfo = spawn.getExecutionInfo(); assertThat(execInfo.get(ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS)) 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..1c675bb2038de4 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,15 @@ 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 =