From b56a2aa709dcb681cfc3faa148a702015ec631d5 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 19 Apr 2021 00:55:59 -0700 Subject: [PATCH] Remote: Use execRoot as input root and do NOT set working directory by default. When --experimental_sibling_external_layout is set, use the parent directory of execRoot as input root and set working directory to the base name of execRoot. Paths of output files are relative to working directory. When --incompatible_remote_paths_relative_to_input_root is set, paths of output files are relative to input root. Introduce RemotePathResolver which is used to convert local paths to remote paths and vice versa. We should prefer this class instead of using execRoot directly whenever possible in remote module. execRoot usages in RemoteCache are all replaced. On Windows, shared action results cache for cl.exe across different workspaces causing header dependency checking to fail. This was initially fixed by #9172, but is broken after https://github.com/bazelbuild/bazel/commit/24c980b6d4adef456cf4b4f84ac3b8ec4580b172. The reason test didn't fail before this change is that two builds from different workspaces do not share the cache since input files are relative to the parent of exec root which contains workspace name. This change fixes that by adding workspace name as platform property for action running on Windows. Fixes https://github.com/bazelbuild/bazel/issues/13188. Closes #13339. PiperOrigin-RevId: 369168230 --- .../lib/actions/ExecutionRequirements.java | 4 + .../build/lib/analysis/platform/BUILD | 1 + .../lib/analysis/platform/PlatformUtils.java | 11 + .../google/devtools/build/lib/remote/BUILD | 1 + .../remote/RemoteActionContextProvider.java | 26 +- .../build/lib/remote/RemoteCache.java | 100 ++++---- .../RemoteRepositoryRemoteExecutor.java | 19 +- .../build/lib/remote/RemoteSpawnCache.java | 18 +- .../build/lib/remote/RemoteSpawnRunner.java | 44 ++-- .../devtools/build/lib/remote/common/BUILD | 4 + .../lib/remote/common/RemotePathResolver.java | 189 +++++++++++++++ .../lib/remote/options/RemoteOptions.java | 13 + .../build/lib/rules/cpp/CppCompileAction.java | 44 ++-- .../build/lib/analysis/platform/BUILD | 1 + .../analysis/platform/PlatformUtilsTest.java | 21 ++ .../build/lib/remote/GrpcCacheClientTest.java | 55 ++++- .../build/lib/remote/RemoteCacheTests.java | 223 ++++++++++++------ .../lib/remote/RemotePathResolverTest.java | 137 +++++++++++ .../lib/remote/RemoteSpawnCacheTest.java | 36 +-- .../lib/remote/RemoteSpawnRunnerTest.java | 110 +++++++-- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 7 +- .../lib/rules/cpp/SpawnGccStrategyTest.java | 5 +- .../build/remote/worker/ExecutionServer.java | 21 +- 23 files changed, 845 insertions(+), 245 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java create mode 100644 src/test/java/com/google/devtools/build/lib/remote/RemotePathResolverTest.java 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 =