diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index ffb8ee87ca71ff..2f38d0193651fa 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -22,6 +22,7 @@ import build.bazel.remote.execution.v2.RequestMetadata; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; @@ -65,6 +66,9 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted private final AtomicBoolean shutdown = new AtomicBoolean(); private final Scheduler scheduler; + private final Set omittedFiles = Sets.newConcurrentHashSet(); + private final Set omittedTreeRoots = Sets.newConcurrentHashSet(); + ByteStreamBuildEventArtifactUploader( Executor executor, ExtendedEventHandler reporter, @@ -83,6 +87,14 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted this.scheduler = Schedulers.from(executor); } + public void omitFile(Path file) { + omittedFiles.add(file); + } + + public void omitTree(Path treeRoot) { + omittedTreeRoots.add(treeRoot); + } + /** Returns {@code true} if Bazel knows that the file is stored on a remote system. */ private static boolean isRemoteFile(Path file) { return file.getFileSystem() instanceof RemoteActionFileSystem @@ -124,10 +136,21 @@ public boolean isRemote() { * Collects metadata for {@code file}. Depending on the underlying filesystem used this method * might do I/O. */ - private static PathMetadata readPathMetadata(Path file) throws IOException { + private PathMetadata readPathMetadata(Path file) throws IOException { if (file.isDirectory()) { return new PathMetadata(file, /* digest= */ null, /* directory= */ true, /* remote= */ false); } + if (omittedFiles.contains(file)) { + return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false); + } + + for (Path treeRoot : omittedTreeRoots) { + if (file.startsWith(treeRoot)) { + omittedFiles.add(file); + return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false); + } + } + DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction()); Digest digest = digestUtil.compute(file); return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file)); @@ -248,7 +271,7 @@ private Single upload(Set files) { .collect(Collectors.toList()) .flatMap(paths -> queryRemoteCache(remoteCache, context, paths)) .flatMap(paths -> uploadLocalFiles(remoteCache, context, paths)) - .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths)), + .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths, omittedFiles)), RemoteCache::release); } @@ -280,8 +303,10 @@ private static class PathConverterImpl implements PathConverter { private final String remoteServerInstanceName; private final Map pathToDigest; private final Set skippedPaths; + private final Set localPaths; - PathConverterImpl(String remoteServerInstanceName, List uploads) { + PathConverterImpl( + String remoteServerInstanceName, List uploads, Set localPaths) { Preconditions.checkNotNull(uploads); this.remoteServerInstanceName = remoteServerInstanceName; pathToDigest = new HashMap<>(uploads.size()); @@ -296,11 +321,17 @@ private static class PathConverterImpl implements PathConverter { } } this.skippedPaths = skippedPaths.build(); + this.localPaths = localPaths; } @Override public String apply(Path path) { Preconditions.checkNotNull(path); + + if (localPaths.contains(path)) { + return String.format("file://%s", path.getPathString()); + } + Digest digest = pathToDigest.get(path); if (digest == null) { if (skippedPaths.contains(path)) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java index 31edf398e75653..23fa5d513d1137 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java @@ -13,11 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +import static com.google.common.base.Preconditions.checkState; + import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory; import com.google.devtools.build.lib.runtime.CommandEnvironment; import java.util.concurrent.Executor; +import javax.annotation.Nullable; /** A factory for {@link ByteStreamBuildEventArtifactUploader}. */ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactUploaderFactory { @@ -30,6 +33,8 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU private final String buildRequestId; private final String commandId; + @Nullable private ByteStreamBuildEventArtifactUploader uploader; + ByteStreamBuildEventArtifactUploaderFactory( Executor executor, ExtendedEventHandler reporter, @@ -49,13 +54,21 @@ class ByteStreamBuildEventArtifactUploaderFactory implements BuildEventArtifactU @Override public BuildEventArtifactUploader create(CommandEnvironment env) { - return new ByteStreamBuildEventArtifactUploader( - executor, - reporter, - verboseFailures, - remoteCache.retain(), - remoteServerInstanceName, - buildRequestId, - commandId); + checkState(uploader == null, "Already created"); + uploader = + new ByteStreamBuildEventArtifactUploader( + executor, + reporter, + verboseFailures, + remoteCache.retain(), + remoteServerInstanceName, + buildRequestId, + commandId); + return uploader; + } + + @Nullable + public ByteStreamBuildEventArtifactUploader get() { + return uploader; } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 8cb9a9e8762716..9ea3ea212d80f9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -356,6 +356,19 @@ public boolean shouldAcceptCachedResult(Spawn spawn) { } } + public static boolean shouldUploadLocalResults( + RemoteOptions remoteOptions, @Nullable Map executionInfo) { + if (useRemoteCache(remoteOptions)) { + if (useDiskCache(remoteOptions)) { + return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo); + } else { + return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); + } + } else { + return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); + } + } + /** * Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote * cache. @@ -365,15 +378,7 @@ public boolean shouldUploadLocalResults(Spawn spawn) { return false; } - if (useRemoteCache(remoteOptions)) { - if (useDiskCache(remoteOptions)) { - return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn); - } else { - return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn); - } - } else { - return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn); - } + return shouldUploadLocalResults(remoteOptions, spawn.getExecutionInfo()); } /** Returns {@code true} if the spawn may be executed remotely. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index ad916d45ab17c8..de7ed308d27d0c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -30,6 +30,7 @@ import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; +import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionInput; @@ -111,6 +112,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadFactory; import java.util.concurrent.ThreadPoolExecutor; +import javax.annotation.Nullable; /** RemoteModule provides distributed cache and remote execution for Bazel. */ public final class RemoteModule extends BlazeModule { @@ -126,7 +128,7 @@ public final class RemoteModule extends BlazeModule { private RemoteActionContextProvider actionContextProvider; private RemoteActionInputFetcher actionInputFetcher; - private RemoteOutputsMode remoteOutputsMode; + private RemoteOptions remoteOptions; private RemoteOutputService remoteOutputService; private ChannelFactory channelFactory = @@ -244,7 +246,7 @@ private void initHttpAndDiskCache( public void beforeCommand(CommandEnvironment env) throws AbruptExitException { Preconditions.checkState(actionContextProvider == null, "actionContextProvider must be null"); Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null"); - Preconditions.checkState(remoteOutputsMode == null, "remoteOutputsMode must be null"); + Preconditions.checkState(remoteOptions == null, "remoteOptions must be null"); RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); if (remoteOptions == null) { @@ -252,7 +254,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { return; } - remoteOutputsMode = remoteOptions.remoteOutputsMode; + this.remoteOptions = remoteOptions; AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); DigestHashFunction hashFn = env.getRuntime().getFileSystem().getDigestFunction(); @@ -705,8 +707,8 @@ public void afterAnalysis( AnalysisResult analysisResult) { // The actionContextProvider may be null if remote execution is disabled or if there was an // error during initialization. - if (remoteOutputsMode != null - && remoteOutputsMode.downloadToplevelOutputsOnly() + if (remoteOptions != null + && remoteOptions.remoteOutputsMode.downloadToplevelOutputsOnly() && actionContextProvider != null) { boolean isTestCommand = env.getCommandName().equals("test"); TopLevelArtifactContext artifactContext = request.getTopLevelArtifactContext(); @@ -726,6 +728,41 @@ public void afterAnalysis( } actionContextProvider.setFilesToDownload(ImmutableSet.copyOf(filesToDownload)); } + + if (remoteOptions != null && remoteOptions.incompatibleRemoteBuildEventUploadRespectNoCache) { + parseNoCacheOutputs(analysisResult); + } + } + + private void parseNoCacheOutputs(AnalysisResult analysisResult) { + if (actionContextProvider == null || actionContextProvider.getRemoteCache() == null) { + return; + } + + ByteStreamBuildEventArtifactUploader uploader = buildEventArtifactUploaderFactoryDelegate.get(); + if (uploader == null) { + return; + } + + for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) { + if (configuredTarget instanceof RuleConfiguredTarget) { + RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget; + for (ActionAnalysisMetadata action : ruleConfiguredTarget.getActions()) { + boolean uploadLocalResults = + RemoteExecutionService.shouldUploadLocalResults( + remoteOptions, action.getExecutionInfo()); + if (!uploadLocalResults) { + for (Artifact output : action.getOutputs()) { + if (output.isTreeArtifact()) { + uploader.omitTree(output.getPath()); + } else { + uploader.omitFile(output.getPath()); + } + } + } + } + } + } } // This is a short-term fix for top-level outputs that are symlinks. Unfortunately, we cannot @@ -829,7 +866,7 @@ public void afterCommand() throws AbruptExitException { remoteDownloaderSupplier.set(null); actionContextProvider = null; actionInputFetcher = null; - remoteOutputsMode = null; + remoteOptions = null; remoteOutputService = null; if (failure != null) { @@ -873,7 +910,7 @@ public void registerActionContexts( @Override public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null"); - Preconditions.checkNotNull(remoteOutputsMode, "remoteOutputsMode must not be null"); + Preconditions.checkNotNull(remoteOptions, "remoteOptions must not be null"); if (actionContextProvider == null) { return; @@ -897,7 +934,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB @Override public OutputService getOutputService() { Preconditions.checkState(remoteOutputService == null, "remoteOutputService must be null"); - if (remoteOutputsMode != null && !remoteOutputsMode.downloadAllOutputs()) { + if (remoteOptions != null && !remoteOptions.remoteOutputsMode.downloadAllOutputs()) { remoteOutputService = new RemoteOutputService(); } return remoteOutputService; @@ -913,13 +950,21 @@ public Iterable> getCommandOptions(Command command) private static class BuildEventArtifactUploaderFactoryDelegate implements BuildEventArtifactUploaderFactory { - private volatile BuildEventArtifactUploaderFactory uploaderFactory; + @Nullable private ByteStreamBuildEventArtifactUploaderFactory uploaderFactory; - public void init(BuildEventArtifactUploaderFactory uploaderFactory) { + public void init(ByteStreamBuildEventArtifactUploaderFactory uploaderFactory) { Preconditions.checkState(this.uploaderFactory == null); this.uploaderFactory = uploaderFactory; } + @Nullable + public ByteStreamBuildEventArtifactUploader get() { + if (uploaderFactory == null) { + return null; + } + return uploaderFactory.get(); + } + public void reset() { this.uploaderFactory = null; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java index df2974c1e919ad..684a971f72a506 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java @@ -75,7 +75,12 @@ public void close() { public ListenableFuture uploadFile( RemoteActionExecutionContext context, Digest digest, Path file) { ListenableFuture future = diskCache.uploadFile(context, digest, file); - if (options.isRemoteExecutionEnabled() + + boolean uploadForSpawn = context.getSpawn() != null; + // If not upload for spawn e.g. for build event artifacts, we always upload files to remote + // cache. + if (!uploadForSpawn + || options.isRemoteExecutionEnabled() || shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { future = Futures.transformAsync( 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 5518672924d369..d7dc2ec61bdcc2 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 @@ -269,6 +269,16 @@ public String getTypeDescription() { help = "Whether to upload locally executed action results to the remote cache.") public boolean remoteUploadLocalResults; + @Option( + name = "incompatible_remote_build_event_upload_respect_no_cache", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If set to true, outputs referenced by BEP are not uploaded to remote cache if the" + + " generating action cannot be cached remotely.") + public boolean incompatibleRemoteBuildEventUploadRespectNoCache; + @Option( name = "incompatible_remote_results_ignore_disk", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index 16365bb1847d8d..8c4099a5931bd6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -615,23 +615,42 @@ public static boolean shouldUploadLocalResultsToRemoteCache( } public static boolean shouldUploadLocalResultsToDiskCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { + RemoteOptions remoteOptions, @Nullable Map executionInfo) { if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { - return spawn == null || Spawns.mayBeCached(spawn); + return executionInfo == null || Spawns.mayBeCached(executionInfo); } else { - return remoteOptions.remoteUploadLocalResults && (spawn == null || Spawns.mayBeCached(spawn)); + return remoteOptions.remoteUploadLocalResults + && (executionInfo == null || Spawns.mayBeCached(executionInfo)); } } - public static boolean shouldUploadLocalResultsToCombinedDisk( + public static boolean shouldUploadLocalResultsToDiskCache( RemoteOptions remoteOptions, @Nullable Spawn spawn) { + ImmutableMap executionInfo = null; + if (spawn != null) { + executionInfo = spawn.getExecutionInfo(); + } + return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); + } + + public static boolean shouldUploadLocalResultsToCombinedDisk( + RemoteOptions remoteOptions, @Nullable Map executionInfo) { if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { // If --incompatible_remote_results_ignore_disk is set, we treat the disk cache part as local // cache. Actions which are tagged with `no-remote-cache` can still hit the disk cache. - return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn); + return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); } else { // Otherwise, it's treated as a remote cache and disabled for `no-remote-cache`. - return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn); + return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); + } + } + + public static boolean shouldUploadLocalResultsToCombinedDisk( + RemoteOptions remoteOptions, @Nullable Spawn spawn) { + ImmutableMap executionInfo = null; + if (spawn != null) { + executionInfo = spawn.getExecutionInfo(); } + return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo); } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index e84635ce7ebc63..ad9b686382632f 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3296,4 +3296,120 @@ EOF //a:consumer >& $TEST_log || fail "Failed to build without remote cache" } +function test_uploader_respsect_no_cache() { + mkdir -p a + cat > a/BUILD < \$@", + tags = ["no-cache"], +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --incompatible_remote_build_event_upload_respect_no_cache \ + --build_event_json_file=bep.json \ + //a:foo >& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" || fail "local files are converted" + expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" +} + +function test_uploader_respsect_no_cache_trees() { + mkdir -p a + cat > a/output_dir.bzl <<'EOF' +def _gen_output_dir_impl(ctx): + output_dir = ctx.actions.declare_directory(ctx.attr.outdir) + ctx.actions.run_shell( + outputs = [output_dir], + inputs = [], + command = """ + mkdir -p $1/sub; \ + index=0; while ((index<10)); do echo $index >$1/$index.txt; index=$(($index+1)); done + echo "Shuffle, duffle, muzzle, muff" > $1/sub/bar + """, + arguments = [output_dir.path], + execution_requirements = {"no-cache": ""}, + ) + return [ + DefaultInfo(files = depset(direct = [output_dir])), + ] +gen_output_dir = rule( + implementation = _gen_output_dir_impl, + attrs = { + "outdir": attr.string(mandatory = True), + }, +) +EOF + + cat > a/BUILD <& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" || fail "local tree files are converted" + expect_not_log "a/dir/.*bytestream://" || fail "local tree files are converted" + expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" +} + +function test_uploader_respsect_no_upload_results() { + mkdir -p a + cat > a/BUILD < \$@", +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --remote_upload_local_results=false \ + --incompatible_remote_build_event_upload_respect_no_cache \ + --build_event_json_file=bep.json \ + //a:foo >& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" || fail "local files are converted" + expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" +} + +function test_uploader_respsect_no_upload_results_combined_cache() { + mkdir -p a + cat > a/BUILD < \$@", +) +EOF + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --disk_cache="${TEST_TMPDIR}/disk_cache" \ + --remote_upload_local_results=false \ + --incompatible_remote_build_event_upload_respect_no_cache \ + --build_event_json_file=bep.json \ + //a:foo >& $TEST_log || fail "Failed to build" + + cat bep.json > $TEST_log + expect_not_log "a:foo.*bytestream://" || fail "local files are converted" + expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data" + remote_cas_files="$(count_remote_cas_files)" + [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_cas_files" +} + run_suite "Remote execution and remote cache tests" diff --git a/src/test/shell/bazel/remote/remote_utils.sh b/src/test/shell/bazel/remote/remote_utils.sh index 9a0191a9ced131..876376a1dd02df 100644 --- a/src/test/shell/bazel/remote/remote_utils.sh +++ b/src/test/shell/bazel/remote/remote_utils.sh @@ -78,3 +78,11 @@ function count_remote_ac_files() { echo 0 fi } + +function count_remote_cas_files() { + if [ -d "$cas_path/cas" ]; then + expr $(find "$cas_path/cas" -type f | wc -l) + else + echo 0 + fi +}