diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index d1519a602fcb58..3dc59033f1c960 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -401,19 +401,17 @@ public void repr(Printer printer) { * * @param execRoot the exec root in which this action is executed * @param bulkDeleter a helper to bulk delete outputs to avoid delegating to the filesystem - * @param outputPrefixForArchivedArtifactsCleanup derived output prefix to construct archived tree - * artifacts to be cleaned up. If null, no cleanup is needed. + * @param cleanupArchivedArtifacts whether to clean up archived tree artifacts */ protected final void deleteOutputs( Path execRoot, ArtifactPathResolver pathResolver, @Nullable BulkDeleter bulkDeleter, - @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup) + boolean cleanupArchivedArtifacts) throws IOException, InterruptedException { Iterable artifactsToDelete = - outputPrefixForArchivedArtifactsCleanup != null - ? Iterables.concat( - outputs, archivedTreeArtifactOutputs(outputPrefixForArchivedArtifactsCleanup)) + cleanupArchivedArtifacts + ? Iterables.concat(outputs, archivedTreeArtifactOutputs()) : outputs; Iterable additionalPathOutputsToDelete = getAdditionalPathOutputsToDelete(); Iterable directoryOutputsToDelete = getDirectoryOutputsToDelete(); @@ -452,10 +450,10 @@ protected Iterable getDirectoryOutputsToDelete() { return ImmutableList.of(); } - private Iterable archivedTreeArtifactOutputs(PathFragment derivedPathPrefix) { + private Iterable archivedTreeArtifactOutputs() { return Iterables.transform( Iterables.filter(outputs, Artifact::isTreeArtifact), - tree -> ArchivedTreeArtifact.createForTree((SpecialArtifact) tree, derivedPathPrefix)); + tree -> ArchivedTreeArtifact.createForTree((SpecialArtifact) tree)); } /** @@ -581,9 +579,9 @@ public void prepare( Path execRoot, ArtifactPathResolver pathResolver, @Nullable BulkDeleter bulkDeleter, - @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup) + boolean cleanupArchivedArtifacts) throws IOException, InterruptedException { - deleteOutputs(execRoot, pathResolver, bulkDeleter, outputPrefixForArchivedArtifactsCleanup); + deleteOutputs(execRoot, pathResolver, bulkDeleter, cleanupArchivedArtifacts); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java index 1a4adb167b6e11..1614b2acaa898d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Action.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ConditionallyThreadCompatible; import com.google.devtools.build.lib.vfs.BulkDeleter; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.util.Map; import javax.annotation.Nullable; @@ -92,7 +91,7 @@ void prepare( Path execRoot, ArtifactPathResolver pathResolver, @Nullable BulkDeleter bulkDeleter, - @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup) + boolean cleanupArchivedArtifacts) throws IOException, InterruptedException; /** diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 48428c1741993a..312e0c325fe424 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -72,7 +73,6 @@ public class ActionCacheChecker { private final Predicate executionFilter; private final ArtifactResolver artifactResolver; private final CacheConfig cacheConfig; - private final PathFragment derivedPathPrefix; @Nullable private final ActionCache actionCache; // Null when not enabled. @@ -107,8 +107,7 @@ public ActionCacheChecker( ArtifactResolver artifactResolver, ActionKeyContext actionKeyContext, Predicate executionFilter, - @Nullable CacheConfig cacheConfig, - PathFragment derivedPathPrefix) { + @Nullable CacheConfig cacheConfig) { this.executionFilter = executionFilter; this.actionKeyContext = actionKeyContext; this.artifactResolver = artifactResolver; @@ -125,7 +124,6 @@ public ActionCacheChecker( } else { this.actionCache = null; } - this.derivedPathPrefix = derivedPathPrefix; } public boolean isActionExecutionProhibited(Action action) { @@ -310,10 +308,7 @@ private CachedOutputMetadata( } private static CachedOutputMetadata loadCachedOutputMetadata( - Action action, - ActionCache.Entry entry, - MetadataHandler metadataHandler, - PathFragment derivedPathPrefix) { + Action action, ActionCache.Entry entry, MetadataHandler metadataHandler) { ImmutableMap.Builder remoteFileMetadata = ImmutableMap.builder(); ImmutableMap.Builder mergedTreeMetadata = @@ -361,12 +356,9 @@ private static CachedOutputMetadata loadCachedOutputMetadata( cachedTreeMetadata .archivedFileValue() .map( - fileArtifactValue -> { - Artifact.ArchivedTreeArtifact archivedTreeArtifact = - Artifact.ArchivedTreeArtifact.createForTree(parent, derivedPathPrefix); - return ArchivedRepresentation.create( - archivedTreeArtifact, fileArtifactValue); - }); + fileArtifactValue -> + ArchivedRepresentation.create( + ArchivedTreeArtifact.createForTree(parent), fileArtifactValue)); } TreeArtifactValue.Builder merged = TreeArtifactValue.newBuilder(parent); @@ -422,7 +414,8 @@ public Token getTokenIfNeedToExecute( EventHandler handler, MetadataHandler metadataHandler, ArtifactExpander artifactExpander, - Map remoteDefaultPlatformProperties) + Map remoteDefaultPlatformProperties, + boolean isRemoteCacheEnabled) throws InterruptedException { // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that // produce only symlinks we should not check whether inputs are valid at all - all that matters @@ -464,10 +457,12 @@ public Token getTokenIfNeedToExecute( } ActionCache.Entry entry = getCacheEntry(action); CachedOutputMetadata cachedOutputMetadata = null; - // load remote metadata from action cache - if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) { - cachedOutputMetadata = - loadCachedOutputMetadata(action, entry, metadataHandler, derivedPathPrefix); + if (entry != null + && !entry.isCorrupted() + && cacheConfig.storeOutputMetadata() + && isRemoteCacheEnabled) { + // load remote metadata from action cache + cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler); } if (mustExecute( diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index e6c55fe85ec1fb..cb0afd2ead1632 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -41,7 +41,6 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.starlarkbuildapi.FileApi; @@ -309,7 +308,7 @@ public ArchivedTreeArtifact getArchivedTreeArtifact(SpecialArtifact treeArtifact /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */ public static final Predicate MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact(); - protected final ArtifactRoot root; + private final ArtifactRoot root; private final int hashCode; private final PathFragment execPath; @@ -978,7 +977,7 @@ boolean ownersEqual(Artifact other) { @Override public PathFragment getRootRelativePath() { - return root.isExternal() ? getExecPath().subFragment(2) : getExecPath(); + return getRoot().isExternal() ? getExecPath().subFragment(2) : getExecPath(); } @Override @@ -1168,10 +1167,10 @@ public SpecialArtifact deserialize(DeserializationContext context, CodedInputStr * TreeFileArtifact children} (and nothing else) of the tree artifact with their filesystem * structure, relative to the {@linkplain SpecialArtifact#getExecPath() tree artifact directory}. */ - @AutoCodec public static final class ArchivedTreeArtifact extends DerivedArtifact { - private static final PathFragment ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT = + private static final PathFragment DEFAULT_DERIVED_TREE_ROOT = PathFragment.create(":archived_tree_artifacts"); + private final SpecialArtifact treeArtifact; private ArchivedTreeArtifact( @@ -1180,6 +1179,8 @@ private ArchivedTreeArtifact( PathFragment execPath, Object generatingActionKey) { super(root, execPath, generatingActionKey); + Preconditions.checkArgument( + treeArtifact.isTreeArtifact(), "Not a tree artifact: %s", treeArtifact); this.treeArtifact = treeArtifact; } @@ -1188,13 +1189,6 @@ public SpecialArtifact getParent() { return treeArtifact; } - /** Creates an archived tree artifact with a given {@code root} and {@code execPath}. */ - public static ArchivedTreeArtifact create( - SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) { - return new ArchivedTreeArtifact( - treeArtifact, root, execPath, treeArtifact.getGeneratingActionKey()); - } - /** * Creates an {@link ArchivedTreeArtifact} for a given tree artifact at the path inferred from * the provided tree. @@ -1206,13 +1200,12 @@ public static ArchivedTreeArtifact create( * {@linkplain ArchivedTreeArtifact artifact} of: {@code * bazel-out/:archived_tree_artifacts/k8-fastbuild/bin/directory.zip}. */ - public static ArchivedTreeArtifact createForTree( - SpecialArtifact treeArtifact, PathFragment derivedPathPrefix) { - return createWithCustomDerivedTreeRoot( + public static ArchivedTreeArtifact createForTree(SpecialArtifact treeArtifact) { + return createInternal( treeArtifact, - derivedPathPrefix, - ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT, - treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip")); + DEFAULT_DERIVED_TREE_ROOT, + treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip"), + treeArtifact.getGeneratingActionKey()); } /** @@ -1221,31 +1214,30 @@ public static ArchivedTreeArtifact createForTree( * *

Example: for a tree artifact with root of {@code bazel-out/k8-fastbuild/bin} returns an * {@linkplain ArchivedTreeArtifact artifact} of: {@code - * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code - * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}. + * bazel-out/{derivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code + * bazel-out/{derivedTreeRoot}/k8-fastbuild/bin}. + * + *

Such artifacts should only be used as outputs of intermediate spawns. Action execution + * results must come from {@link #createForTree}. */ public static ArchivedTreeArtifact createWithCustomDerivedTreeRoot( + SpecialArtifact treeArtifact, PathFragment derivedTreeRoot, PathFragment rootRelativePath) { + return createInternal( + treeArtifact, derivedTreeRoot, rootRelativePath, treeArtifact.getGeneratingActionKey()); + } + + private static ArchivedTreeArtifact createInternal( SpecialArtifact treeArtifact, - PathFragment derivedPathPrefix, - PathFragment customDerivedTreeRoot, - PathFragment rootRelativePath) { - ArtifactRoot artifactRoot = - createRootForArchivedArtifact( - treeArtifact.getRoot(), derivedPathPrefix, customDerivedTreeRoot); - return create( - treeArtifact, artifactRoot, artifactRoot.getExecPath().getRelative(rootRelativePath)); - } - - private static ArtifactRoot createRootForArchivedArtifact( - ArtifactRoot treeArtifactRoot, - PathFragment derivedPathPrefix, - PathFragment customDerivedTreeRoot) { - return ArtifactRoot.asDerivedRoot( - getExecRoot(treeArtifactRoot), - // e.g. bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin - RootType.Output, - getExecPathWithinCustomDerivedRoot( - derivedPathPrefix, customDerivedTreeRoot, treeArtifactRoot.getExecPath())); + PathFragment derivedTreeRoot, + PathFragment rootRelativePath, + Object generatingActionKey) { + ArtifactRoot treeRoot = treeArtifact.getRoot(); + PathFragment archiveRoot = embedDerivedTreeRoot(treeRoot.getExecPath(), derivedTreeRoot); + return new ArchivedTreeArtifact( + treeArtifact, + ArtifactRoot.asDerivedRoot(getExecRoot(treeRoot), RootType.Output, archiveRoot), + archiveRoot.getRelative(rootRelativePath), + generatingActionKey); } /** @@ -1255,23 +1247,22 @@ private static ArtifactRoot createRootForArchivedArtifact( *

Example: {@code bazel-out/k8-fastbuild/bin -> * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}. */ - public static PathFragment getExecPathWithinArchivedArtifactsTree( - PathFragment derivedPathPrefix, PathFragment execPath) { - return getExecPathWithinCustomDerivedRoot( - derivedPathPrefix, ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT, execPath); + public static PathFragment getExecPathWithinArchivedArtifactsTree(PathFragment execPath) { + return embedDerivedTreeRoot(execPath, DEFAULT_DERIVED_TREE_ROOT); } /** * Translates provided output {@code execPath} to one under provided derived tree root. * *

Example: {@code bazel-out/k8-fastbuild/bin -> - * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}. + * bazel-out/{derivedTreeRoot}/k8-fastbuild/bin}. */ - private static PathFragment getExecPathWithinCustomDerivedRoot( - PathFragment derivedPathPrefix, PathFragment customDerivedTreeRoot, PathFragment execPath) { - return derivedPathPrefix - .getRelative(customDerivedTreeRoot) - .getRelative(execPath.relativeTo(derivedPathPrefix)); + private static PathFragment embedDerivedTreeRoot( + PathFragment execPath, PathFragment derivedTreeRoot) { + return execPath + .subFragment(0, 1) + .getRelative(derivedTreeRoot) + .getRelative(execPath.subFragment(1)); } private static Path getExecRoot(ArtifactRoot artifactRoot) { @@ -1284,16 +1275,42 @@ private static Path getExecRoot(ArtifactRoot artifactRoot) { 0, rootPathFragment.segmentCount() - artifactRoot.getExecPath().segmentCount()); return rootPath.getFileSystem().getPath(execRootPath); } + } - @VisibleForSerialization - @AutoCodec.Instantiator - static ArchivedTreeArtifact createForDeserialization( - SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) { + @SuppressWarnings("unused") // Codec used by reflection. + private static final class ArchivedTreeArtifactCodec + implements ObjectCodec { + + @Override + public Class getEncodedClass() { + return ArchivedTreeArtifact.class; + } + + @Override + public void serialize( + SerializationContext context, ArchivedTreeArtifact obj, CodedOutputStream codedOut) + throws SerializationException, IOException { + PathFragment derivedTreeRoot = obj.getRoot().getExecPath().subFragment(1, 2); + + context.serialize(obj.getParent(), codedOut); + context.serialize(derivedTreeRoot, codedOut); + context.serialize(obj.getRootRelativePath(), codedOut); + } + + @Override + public ArchivedTreeArtifact deserialize( + DeserializationContext context, CodedInputStream codedIn) + throws SerializationException, IOException { + SpecialArtifact treeArtifact = context.deserialize(codedIn); + PathFragment derivedTreeRoot = context.deserialize(codedIn); + PathFragment rootRelativePath = context.deserialize(codedIn); Object generatingActionKey = treeArtifact.hasGeneratingActionKey() ? treeArtifact.getGeneratingActionKey() : OMITTED_FOR_SERIALIZATION; - return new ArchivedTreeArtifact(treeArtifact, root, execPath, generatingActionKey); + + return ArchivedTreeArtifact.createInternal( + treeArtifact, derivedTreeRoot, rootRelativePath, generatingActionKey); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java index eb96d79b7051c0..964b31d107003d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java @@ -113,7 +113,7 @@ private String getAdditionalWorkspaceStatus( } catch (IOException e) { throw createExecutionException(e, Code.STDERR_IO_EXCEPTION); } - return new String(stdoutStream.toByteArray(), UTF_8); + return stdoutStream.toString(UTF_8); } } catch (BadExitStatusException e) { throw createExecutionException(e, Code.NON_ZERO_EXIT); @@ -159,7 +159,7 @@ public void prepare( Path execRoot, ArtifactPathResolver pathResolver, @Nullable BulkDeleter bulkDeleter, - @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup) + boolean cleanupArchivedArtifacts) throws IOException { // The default implementation of this method deletes all output files; override it to keep // the old stableStatus around. This way we can reuse the existing file (preserving its mtime) @@ -356,8 +356,8 @@ public com.google.devtools.build.lib.shell.Command getCommand() { @Override public Iterable> getCommandOptions(Command command) { return "build".equals(command.name()) - ? ImmutableList.>of(WorkspaceStatusAction.Options.class) - : ImmutableList.>of(); + ? ImmutableList.of(WorkspaceStatusAction.Options.class) + : ImmutableList.of(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 027084dba92579..245e08ab1b9c86 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -865,8 +865,7 @@ private Builder createBuilder( .setEnabled(options.useActionCache) .setVerboseExplanations(options.verboseExplanations) .setStoreOutputMetadata(options.actionCacheStoreOutputMetadata) - .build(), - PathFragment.create(env.getDirectories().getRelativeOutputPath())), + .build()), env.getTopDownActionCache(), request.getPackageOptions().checkOutputFiles ? modifiedOutputFiles 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 0c7f5812b8c155..5518672924d369 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 @@ -561,10 +561,14 @@ public RemoteOutputsStrategyConverter() { /** The maximum size of an outbound message sent via a gRPC channel. */ public int maxOutboundMessageSize = 1024 * 1024; - public boolean isRemoteEnabled() { - return !Strings.isNullOrEmpty(remoteCache) || isRemoteExecutionEnabled(); + /** Returns {@code true} if remote cache or disk cache is enabled. */ + public boolean isRemoteCacheEnabled() { + return !Strings.isNullOrEmpty(remoteCache) + || !(diskCache == null || diskCache.isEmpty()) + || isRemoteExecutionEnabled(); } + /** Returns {@code true} if remote execution is enabled. */ public boolean isRemoteExecutionEnabled() { return !Strings.isNullOrEmpty(remoteExecutor); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java index 7433410b5b3711..edccfe93977cc8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java @@ -37,7 +37,6 @@ import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.BulkDeleter; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import java.io.IOException; import java.util.Map; @@ -72,12 +71,12 @@ public void prepare( Path execRoot, ArtifactPathResolver pathResolver, @Nullable BulkDeleter bulkDeleter, - @Nullable PathFragment outputPrefixForArchivedArtifactsCleanup) + boolean cleanupArchivedArtifacts) throws IOException, InterruptedException { if (includePath.isDirectory(Symlinks.NOFOLLOW)) { includePath.deleteTree(); } - super.prepare(execRoot, pathResolver, bulkDeleter, outputPrefixForArchivedArtifactsCleanup); + super.prepare(execRoot, pathResolver, bulkDeleter, cleanupArchivedArtifacts); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 84fffad5602f58..63347864f01d86 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -743,7 +743,7 @@ public ActionContinuationOrResult execute() actionExecutionContext.getPathResolver(), /*bulkDeleter=*/ null, // We don't create any tree artifacts anyway. - /*outputPrefixForArchivedArtifactsCleanup=*/ null); + /*cleanupArchivedArtifacts=*/ false); } catch (IOException e) { throw new EnvironmentalExecException( e, diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index ef5d1bb51ff502..b125acd48b1a6f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -444,7 +444,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti env.getExecRoot(), ArtifactPathResolver.IDENTITY, /*bulkDeleter=*/ null, - /*outputPrefixForArchivedArtifactsCleanup=*/ null); + /*cleanupArchivedArtifacts=*/ false); } catch (IOException e) { return reportAndCreateFailureResult( env, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java index f1a5af5637e93d..c2d6286d0bb7f4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java @@ -296,10 +296,7 @@ private static TreeArtifactValue transformSharedTree( .ifPresent( archivedRepresentation -> newTree.setArchivedRepresentation( - ArchivedTreeArtifact.create( - newParent, - archivedRepresentation.archivedTreeFileArtifact().getRoot(), - archivedRepresentation.archivedTreeFileArtifact().getExecPath()), + ArchivedTreeArtifact.createForTree(newParent), archivedRepresentation.archivedFileValue())); return newTree.build(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index f6725ffa038855..52464a9d342500 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -63,12 +63,13 @@ * action's outputs for purposes of creating the final {@link ActionExecutionValue}. * *

The handler can be in one of two modes. After construction, it acts as a cache for input and - * output metadata while {@link ActionCacheChecker} determines whether the action needs to be - * executed. If the action needs to be executed (i.e. no action cache hit), {@link - * #prepareForActionExecution} is called. This call switches the handler to a mode where it accepts - * {@linkplain MetadataInjector injected output data}, or otherwise obtains metadata from the - * filesystem. Freshly created output files are set read-only and executable before - * statting them to ensure that the stat's ctime is up to date. + * output metadata while {@link com.google.devtools.build.lib.actions.ActionCacheChecker} determines + * whether the action needs to be executed. If the action needs to be executed (i.e. no action cache + * hit), {@link #prepareForActionExecution} is called. This call switches the handler to a mode + * where it accepts {@linkplain com.google.devtools.build.lib.actions.cache.MetadataInjector + * injected output data}, or otherwise obtains metadata from the filesystem. Freshly created output + * files are set read-only and executable before statting them to ensure that the stat's + * ctime is up to date. * *

After action execution, {@link #getMetadata} should be called on each of the action's outputs * (except those that were {@linkplain #artifactOmitted omitted}) to ensure that declared outputs @@ -358,8 +359,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa }); if (archivedTreeArtifactsEnabled) { - ArchivedTreeArtifact archivedTreeArtifact = - ArchivedTreeArtifact.createForTree(parent, derivedPathPrefix); + ArchivedTreeArtifact archivedTreeArtifact = ArchivedTreeArtifact.createForTree(parent); FileStatus statNoFollow = artifactPathResolver.toPath(archivedTreeArtifact).statIfFound(Symlinks.NOFOLLOW); if (statNoFollow != null) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index ab88a61b9c8dd1..acdb1b0ec53fd5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -102,7 +102,6 @@ import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.OutputService.ActionFileSystemType; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls; import com.google.devtools.build.skyframe.SkyFunction.Environment; @@ -215,7 +214,6 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { private final Supplier> sourceRootSupplier; private DiscoveredModulesPruner discoveredModulesPruner; - private final PathFragment relativeOutputPath; SkyframeActionExecutor( ActionKeyContext actionKeyContext, @@ -223,7 +221,6 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { MetadataConsumerForMetrics outputArtifactsFromActionCache, AtomicReference statusReporterRef, Supplier> sourceRootSupplier, - PathFragment relativeOutputPath, AtomicReference syscalls, Function threadStateReceiverFactory) { this.actionKeyContext = actionKeyContext; @@ -231,7 +228,6 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { this.outputArtifactsFromActionCache = outputArtifactsFromActionCache; this.statusReporterRef = statusReporterRef; this.sourceRootSupplier = sourceRootSupplier; - this.relativeOutputPath = relativeOutputPath; this.syscalls = syscalls; this.threadStateReceiverFactory = threadStateReceiverFactory; } @@ -603,6 +599,7 @@ Token checkActionCache( remoteOptions != null ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableSortedMap.of(); + boolean isRemoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled(); token = actionCacheChecker.getTokenIfNeedToExecute( action, @@ -613,7 +610,8 @@ Token checkActionCache( : null, metadataHandler, artifactExpander, - remoteDefaultProperties); + remoteDefaultProperties, + isRemoteCacheEnabled); } catch (UserExecException e) { throw e.toActionExecutionException(action); } @@ -990,7 +988,7 @@ public ActionStepOrResult run(Environment env) actionExecutionContext.getExecRoot(), actionExecutionContext.getPathResolver(), outputService != null ? outputService.bulkDeleter() : null, - useArchivedTreeArtifacts(action) ? relativeOutputPath : null); + useArchivedTreeArtifacts(action)); } catch (IOException e) { logger.atWarning().withCause(e).log( "failed to delete output files before executing action: '%s'", action); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 690c8c747c1673..4b8d460f13e71e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -376,6 +376,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur private Map lastRemoteDefaultExecProperties; private RemoteOutputsMode lastRemoteOutputsMode; + private Boolean lastRemoteCacheEnabled; class PathResolverFactoryImpl implements PathResolverFactory { @Override @@ -455,13 +456,11 @@ protected SkyframeExecutor( outputArtifactsFromActionCache, statusReporterRef, this::getPathEntries, - PathFragment.create(directories.getRelativeOutputPath()), syscalls, skyKeyStateReceiver::makeThreadStateReceiver); this.artifactFactory = new ArtifactFactory( - /* execRootParent= */ directories.getExecRootBase(), - directories.getRelativeOutputPath()); + /*execRootParent=*/ directories.getExecRootBase(), directories.getRelativeOutputPath()); this.skyframeBuildView = new SkyframeBuildView(artifactFactory, this, ruleClassProvider, actionKeyContext); this.externalFilesHelper = @@ -1731,10 +1730,25 @@ private void deleteActionsIfRemoteOptionsChanged(OptionsProvider options) lastRemoteDefaultExecProperties != null && !remoteDefaultExecProperties.equals(lastRemoteDefaultExecProperties); lastRemoteDefaultExecProperties = remoteDefaultExecProperties; + + boolean remoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled(); + // If we have remote metadata from last build, and the remote cache is not + // enabled in this build, invalidate actions since they can't download those + // remote files. + // + // TODO(chiwang): Re-evaluate this after action rewinding is implemented in + // Bazel since we can treat that case as lost inputs. + if (lastRemoteOutputsMode != RemoteOutputsMode.ALL) { + needsDeletion |= + lastRemoteCacheEnabled != null && lastRemoteCacheEnabled && !remoteCacheEnabled; + } + lastRemoteCacheEnabled = remoteCacheEnabled; + RemoteOutputsMode remoteOutputsMode = remoteOptions != null ? remoteOptions.remoteOutputsMode : RemoteOutputsMode.ALL; needsDeletion |= lastRemoteOutputsMode != null && lastRemoteOutputsMode != remoteOutputsMode; this.lastRemoteOutputsMode = remoteOutputsMode; + if (needsDeletion) { memoizingEvaluator.delete(k -> SkyFunctions.ACTION_EXECUTION.equals(k.functionName())); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index f9cfd6380907e6..536d0866e32373 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionCacheChecker.Token; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; @@ -84,7 +85,6 @@ public class ActionCacheCheckerTest { private DigestHashFunction digestHashFunction; private FileSystem fileSystem; private ArtifactRoot artifactRoot; - private PathFragment derivedPathPrefix; @Before public void setupCache() throws Exception { @@ -92,7 +92,6 @@ public void setupCache() throws Exception { Clock clock = new ManualClock(); cache = new CorruptibleActionCache(scratch.resolve("/cache/test.dat"), clock); - derivedPathPrefix = PathFragment.create("bin"); cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ false); digestHashFunction = DigestHashFunction.SHA256; fileSystem = new InMemoryFileSystem(digestHashFunction); @@ -114,8 +113,7 @@ private ActionCacheChecker createActionCacheChecker(boolean storeOutputMetadata) .setEnabled(true) .setVerboseExplanations(false) .setStoreOutputMetadata(storeOutputMetadata) - .build(), - derivedPathPrefix); + .build()); } @Before @@ -153,7 +151,7 @@ private void runAction(Action action, Map clientEnv) throws Exce private void runAction(Action action, Map clientEnv, Map platform) throws Exception { - runAction(action, clientEnv, platform, new FakeMetadataHandler(derivedPathPrefix)); + runAction(action, clientEnv, platform, new FakeMetadataHandler()); } private void runAction( @@ -185,7 +183,8 @@ private void runAction( /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - platform); + platform, + /*isRemoteCacheEnabled=*/ true); if (token != null) { // Real action execution would happen here. ActionExecutionContext context = mock(ActionExecutionContext.class); @@ -440,9 +439,10 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio /*resolvedCacheArtifacts=*/ null, /*clientEnv=*/ ImmutableMap.of(), /*handler=*/ null, - new FakeMetadataHandler(derivedPathPrefix), + new FakeMetadataHandler(), /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of())) + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true)) .isNotNull(); } @@ -451,7 +451,7 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) { return new RemoteFileArtifactValue(digest(bytes), bytes.length, 1, "action-id"); } - private TreeArtifactValue createTreeMetadata( + private static TreeArtifactValue createTreeMetadata( SpecialArtifact parent, ImmutableMap children, Optional archivedArtifactValue) { @@ -462,8 +462,7 @@ private TreeArtifactValue createTreeMetadata( } archivedArtifactValue.ifPresent( metadata -> { - Artifact.ArchivedTreeArtifact artifact = - Artifact.ArchivedTreeArtifact.createForTree(parent, derivedPathPrefix); + ArchivedTreeArtifact artifact = ArchivedTreeArtifact.createForTree(parent); builder.setArchivedRepresentation( TreeArtifactValue.ArchivedRepresentation.create(artifact, metadata)); }); @@ -549,7 +548,7 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { Artifact output = createArtifact(artifactRoot, "bin/dummy"); String content = "content"; Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); Token token = @@ -560,7 +559,8 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); assertThat(output.getPath().exists()).isFalse(); assertThat(token).isNull(); @@ -570,6 +570,33 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content)); } + @Test + public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoaded() + throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ false); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + @Test public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached() throws Exception { cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); @@ -671,7 +698,7 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception { Action action = new InjectOutputTreeMetadataAction( output, createTreeMetadata(output, ImmutableMap.of(), Optional.empty())); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); Token token = @@ -682,7 +709,8 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception { /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); assertThat(token).isNull(); assertThat(output.getPath().exists()).isFalse(); @@ -755,7 +783,7 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex Action action = new InjectOutputTreeMetadataAction( output, createTreeMetadata(output, children, Optional.empty())); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); Token token = @@ -766,7 +794,8 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); TreeArtifactValue expectedMetadata = createTreeMetadata(output, children, Optional.empty()); assertThat(token).isNull(); @@ -796,7 +825,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc output, createTreeMetadata(output, children1, Optional.empty()), createTreeMetadata(output, children2, Optional.empty())); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); writeIsoLatin1(output.getPath().getRelative("file2"), "modified_local"); @@ -836,12 +865,10 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content"))), createTreeMetadata( output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("modified")))); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); - writeIsoLatin1( - Artifact.ArchivedTreeArtifact.createForTree(output, derivedPathPrefix).getPath(), - "modified"); + writeIsoLatin1(ArchivedTreeArtifact.createForTree(output).getPath(), "modified"); // Not cached since local file changed runAction(action, metadataHandler); @@ -862,7 +889,7 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); } - private void writeContentAsLatin1(Path path, String content) throws IOException { + private static void writeContentAsLatin1(Path path, String content) throws IOException { Path parent = path.getParentDirectory(); if (parent != null) { parent.createDirectoryAndParents(); @@ -882,7 +909,7 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th Action action = new InjectOutputTreeMetadataAction( output, createTreeMetadata(output, children, Optional.empty())); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); writeContentAsLatin1(output.getPath().getRelative("file1"), "content1"); @@ -895,7 +922,8 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); assertThat(token).isNull(); assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); @@ -910,8 +938,9 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th output, ImmutableMap.of( "file1", - FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), - "file2", createRemoteFileMetadata("content2")), + FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), + "file2", + createRemoteFileMetadata("content2")), Optional.empty())); } @@ -926,12 +955,10 @@ public void saveOutputMetadata_treeMetadataWithSameLocalArchivedArtifact_cached( output, createTreeMetadata( output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); - MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); - writeContentAsLatin1( - Artifact.ArchivedTreeArtifact.createForTree(output, derivedPathPrefix).getPath(), - "content"); + writeContentAsLatin1(ArchivedTreeArtifact.createForTree(output).getPath(), "content"); // Cache hit runAction(action, metadataHandler); @@ -948,7 +975,7 @@ public void saveOutputMetadata_treeMetadataWithSameLocalArchivedArtifact_cached( } /** An {@link ActionCache} that allows injecting corruption for testing. */ - private static class CorruptibleActionCache implements ActionCache { + private static final class CorruptibleActionCache implements ActionCache { private final CompactPersistentActionCache delegate; private boolean corrupted = false; @@ -1021,14 +1048,9 @@ public MiddlemanType getActionType() { } /** A fake metadata handler that is able to obtain metadata from the file system. */ - private static class FakeMetadataHandler extends FakeMetadataHandlerBase { + private static final class FakeMetadataHandler extends FakeMetadataHandlerBase { private final Map fileMetadata = new HashMap<>(); private final Map treeMetadata = new HashMap<>(); - private final PathFragment derivedPathPrefix; - - FakeMetadataHandler(PathFragment derivedPathPrefix) { - this.derivedPathPrefix = derivedPathPrefix; - } @Override public void injectFile(Artifact output, FileArtifactValue metadata) { @@ -1086,8 +1108,7 @@ public TreeArtifactValue getTreeArtifactValue(SpecialArtifact output) throws IOE }); } - Artifact.ArchivedTreeArtifact archivedTreeArtifact = - Artifact.ArchivedTreeArtifact.createForTree(output, derivedPathPrefix); + ArchivedTreeArtifact archivedTreeArtifact = ArchivedTreeArtifact.createForTree(output); if (archivedTreeArtifact.getPath().exists()) { tree.setArchivedRepresentation( archivedTreeArtifact, @@ -1102,9 +1123,9 @@ public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {} } private static class WriteEmptyOutputAction extends NullAction { - public WriteEmptyOutputAction() {} + WriteEmptyOutputAction() {} - public WriteEmptyOutputAction(Artifact... outputs) { + WriteEmptyOutputAction(Artifact... outputs) { super(outputs); } @@ -1129,7 +1150,7 @@ private static class InjectOutputFileMetadataAction extends NullAction { private final Artifact output; private final Deque metadataDeque; - public InjectOutputFileMetadataAction(Artifact output, FileArtifactValue... metadata) { + InjectOutputFileMetadataAction(Artifact output, FileArtifactValue... metadata) { super(output); this.output = output; @@ -1143,11 +1164,11 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) { } } - private static class InjectOutputTreeMetadataAction extends NullAction { + private static final class InjectOutputTreeMetadataAction extends NullAction { private final SpecialArtifact output; private final Deque metadataDeque; - public InjectOutputTreeMetadataAction(SpecialArtifact output, TreeArtifactValue... metadata) { + InjectOutputTreeMetadataAction(SpecialArtifact output, TreeArtifactValue... metadata) { super(output); this.output = output; diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java index 6d184125aff7a5..59a67acc09b219 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java @@ -260,8 +260,10 @@ public boolean includeGeneratingActionKey(DerivedArtifact artifact) { ActionsTestUtil.createTreeArtifactWithGeneratingAction( rootDir, rootDir.getExecPath().getRelative("tree")); TreeFileArtifact treeChild = TreeFileArtifact.createTreeOutput(tree, "child"); - ArchivedTreeArtifact archivedTree = - ArchivedTreeArtifact.create(tree, rootDir, PathFragment.create("root/archived")); + ArchivedTreeArtifact archivedTree = ArchivedTreeArtifact.createForTree(tree); + ArchivedTreeArtifact customArchivedTree = + ArchivedTreeArtifact.createWithCustomDerivedTreeRoot( + tree, PathFragment.create("custom"), PathFragment.create("archived.zip")); SpecialArtifact templateExpansionTree = ActionsTestUtil.createTreeArtifactWithGeneratingAction( @@ -276,7 +278,13 @@ public boolean includeGeneratingActionKey(DerivedArtifact artifact) { SerializationTester tester = new SerializationTester( - artifact, anotherArtifact, tree, treeChild, archivedTree, expansionOutput) + artifact, + anotherArtifact, + tree, + treeChild, + archivedTree, + customArchivedTree, + expansionOutput) .addDependency(FileSystem.class, scratch.getFileSystem()) .addDependency( RootCodecDependencies.class, new RootCodecDependencies(anotherRoot.getRoot())) @@ -365,8 +373,7 @@ public void testDirnameInExecutionDir() throws Exception { @Test public void testCanConstructPathFromDirAndFilename() throws Exception { Artifact artifact = createDirNameArtifact(); - String constructed = - String.format("%s/%s", artifact.getDirname(), artifact.getFilename()); + String constructed = String.format("%s/%s", artifact.getDirname(), artifact.getFilename()); assertThat(constructed).isEqualTo("aaa/bbb/ccc/ddd"); } @@ -511,8 +518,7 @@ public void archivedTreeArtifact_create_returnsArtifactInArchivedRoot() { ArtifactRoot.asDerivedRoot(execDir, RootType.Output, "blaze-out", "fastbuild"); SpecialArtifact tree = createTreeArtifact(root, "tree"); - ArchivedTreeArtifact archivedTreeArtifact = - ArchivedTreeArtifact.createForTree(tree, PathFragment.create("blaze-out")); + ArchivedTreeArtifact archivedTreeArtifact = ArchivedTreeArtifact.createForTree(tree); assertThat(archivedTreeArtifact.getParent()).isSameInstanceAs(tree); assertThat(archivedTreeArtifact.getArtifactOwner()) @@ -529,8 +535,7 @@ public void archivedTreeArtifact_create_returnsArtifactWithGeneratingActionFromP ActionLookupData actionLookupData = ActionLookupData.create(actionLookupKey, 0); SpecialArtifact tree = createTreeArtifact(rootDir, "tree", actionLookupData); - ArchivedTreeArtifact archivedTreeArtifact = - ArchivedTreeArtifact.createForTree(tree, PathFragment.create("root")); + ArchivedTreeArtifact archivedTreeArtifact = ArchivedTreeArtifact.createForTree(tree); assertThat(archivedTreeArtifact.getExecPathString()) .isEqualTo("root/:archived_tree_artifacts/tree.zip"); @@ -538,44 +543,6 @@ public void archivedTreeArtifact_create_returnsArtifactWithGeneratingActionFromP assertThat(archivedTreeArtifact.getGeneratingActionKey()).isSameInstanceAs(actionLookupData); } - @Test - public void archivedTreeArtifact_createWithLongerDerivedPrefix_returnsArtifactWithCorrectPath() { - ArtifactRoot root = - ArtifactRoot.asDerivedRoot(execDir, RootType.Output, "dir1", "dir2", "dir3"); - SpecialArtifact tree = createTreeArtifact(root, "tree"); - - ArchivedTreeArtifact archivedTreeArtifact = - ArchivedTreeArtifact.createForTree(tree, PathFragment.create("dir1/dir2")); - - assertThat(archivedTreeArtifact.getExecPathString()) - .isEqualTo("dir1/dir2/:archived_tree_artifacts/dir3/tree.zip"); - assertThat(archivedTreeArtifact.getRoot().getExecPathString()) - .isEqualTo("dir1/dir2/:archived_tree_artifacts/dir3"); - } - - @Test - public void archivedTreeArtifact_create_failsForWrongDerivedPrefix() { - ArtifactRoot root = - ArtifactRoot.asDerivedRoot(execDir, RootType.Output, "blaze-out", "fastbuild"); - SpecialArtifact tree = createTreeArtifact(root, "tree"); - PathFragment wrongPrefix = PathFragment.create("notAPrefix"); - - assertThrows( - IllegalArgumentException.class, - () -> ArchivedTreeArtifact.createForTree(tree, wrongPrefix)); - } - - @Test - public void archivedTreeArtifact_create_failsForDerivedPrefixOutsideOfArtifactRoot() { - ArtifactRoot root = ArtifactRoot.asDerivedRoot(execDir, RootType.Output, "dir1", "dir2"); - SpecialArtifact tree = createTreeArtifact(root, "dir3/tree"); - PathFragment prefixOutsideOfRoot = PathFragment.create("dir1/dir2/dir3"); - - assertThrows( - IllegalArgumentException.class, - () -> ArchivedTreeArtifact.createForTree(tree, prefixOutsideOfRoot)); - } - @Test public void archivedTreeArtifact_createWithCustomDerivedTreeRoot_returnsArtifactWithCustomRoot() { ArtifactRoot root = @@ -584,10 +551,7 @@ public void archivedTreeArtifact_createWithCustomDerivedTreeRoot_returnsArtifact ArchivedTreeArtifact archivedTreeArtifact = ArchivedTreeArtifact.createWithCustomDerivedTreeRoot( - tree, - PathFragment.create("blaze-out"), - PathFragment.create("custom/custom2"), - PathFragment.create("treePath/file.xyz")); + tree, PathFragment.create("custom/custom2"), PathFragment.create("treePath/file.xyz")); assertThat(archivedTreeArtifact.getParent()).isSameInstanceAs(tree); assertThat(archivedTreeArtifact.getExecPathString()) @@ -620,22 +584,11 @@ RootCodecDependencies.class, new RootCodecDependencies(anotherRoot.getRoot())) public void archivedTreeArtifact_getExecPathWithinArchivedArtifactsTree_returnsCorrectPath() { assertThat( ArchivedTreeArtifact.getExecPathWithinArchivedArtifactsTree( - PathFragment.create("bazel-out"), PathFragment.create("bazel-out/k8-fastbuild/bin/dir/subdir"))) .isEqualTo( PathFragment.create("bazel-out/:archived_tree_artifacts/k8-fastbuild/bin/dir/subdir")); } - @Test - public void archivedTreeArtifact_getExecPathWithinArchivedArtifactsTree_wrongPrefix_fails() { - assertThrows( - IllegalArgumentException.class, - () -> - ArchivedTreeArtifact.getExecPathWithinArchivedArtifactsTree( - PathFragment.create("wrongPrefix"), - PathFragment.create("bazel-out/k8-fastbuild/bin/dir/subdir"))); - } - private static SpecialArtifact createTreeArtifact(ArtifactRoot root, String relativePath) { return createTreeArtifact(root, relativePath, ActionsTestUtil.NULL_ACTION_LOOKUP_DATA); } @@ -654,7 +607,6 @@ private static SpecialArtifact createTreeArtifact( private static ArchivedTreeArtifact createArchivedTreeArtifact( ArtifactRoot root, String treeRelativePath) { - return ArchivedTreeArtifact.createForTree( - createTreeArtifact(root, treeRelativePath), root.getExecPath().subFragment(0, 1)); + return ArchivedTreeArtifact.createForTree(createTreeArtifact(root, treeRelativePath)); } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index 040264d16ce292..7e8be67a8ee5b6 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -50,7 +51,6 @@ public class CompactPersistentActionCacheTest { private Path journalFile; private final ManualClock clock = new ManualClock(); private CompactPersistentActionCache cache; - private PathFragment derivedPathPrefix; private ArtifactRoot artifactRoot; @Before @@ -59,7 +59,6 @@ public final void createFiles() throws Exception { cache = CompactPersistentActionCache.create(dataRoot, clock, NullEventHandler.INSTANCE); mapFile = CompactPersistentActionCache.cacheFile(dataRoot); journalFile = CompactPersistentActionCache.journalFile(dataRoot); - derivedPathPrefix = PathFragment.create("bin"); artifactRoot = ArtifactRoot.asDerivedRoot( scratch.getFileSystem().getPath("/output"), ArtifactRoot.RootType.Output, "bin"); @@ -219,8 +218,7 @@ private TreeArtifactValue createTreeMetadata( } archivedArtifactValue.ifPresent( metadata -> { - Artifact.ArchivedTreeArtifact artifact = - Artifact.ArchivedTreeArtifact.createForTree(parent, derivedPathPrefix); + ArchivedTreeArtifact artifact = ArchivedTreeArtifact.createForTree(parent); builder.setArchivedRepresentation( TreeArtifactValue.ArchivedRepresentation.create(artifact, metadata)); }); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java index 75a3af46e703d2..261c28bfbf89eb 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java @@ -166,7 +166,7 @@ public void testFileRemoved() throws Exception { rootDirectory, ArtifactPathResolver.IDENTITY, /*bulkDeleter=*/ null, - /*outputPrefixForArchivedArtifactsCleanup=*/ null); + /*cleanupArchivedArtifacts=*/ false); assertThat(extra.exists()).isFalse(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java index 63c1d7751335a5..a9c1d8a368acbf 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java @@ -223,8 +223,7 @@ private TreeArtifactValue createTreeArtifactValue( } if (includeArchivedTreeArtifacts) { - ArchivedTreeArtifact archivedArtifact = - ArchivedTreeArtifact.createForTree(treeArtifact, DERIVED_PATH_PREFIX); + ArchivedTreeArtifact archivedArtifact = ArchivedTreeArtifact.createForTree(treeArtifact); createFile(archivedArtifact.getPath()); builder.setArchivedRepresentation( archivedArtifact, FileArtifactValue.createForTesting(archivedArtifact)); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java index 2df56fc29467e2..35240c8bfc0d99 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java @@ -218,8 +218,7 @@ private TreeFileArtifact createTreeFileArtifactWithContent( private ArchivedTreeArtifact createArchivedTreeArtifactWithContent( SpecialArtifact treeArtifact, String... contentLines) throws IOException { - ArchivedTreeArtifact artifact = - ArchivedTreeArtifact.createForTree(treeArtifact, PathFragment.create("bin")); + ArchivedTreeArtifact artifact = ArchivedTreeArtifact.createForTree(treeArtifact); writeFile(artifact.getPath(), contentLines); return artifact; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index 4a1fa99fadecd3..7766cb3716407c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -89,7 +89,6 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.events.Reporter; -import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Target; @@ -183,7 +182,7 @@ public final class SequencedSkyframeExecutorTest extends BuildViewTestCase { private OptionsParser options; @Before - public final void createSkyframeExecutorAndVisitor() throws Exception { + public void createSkyframeExecutorAndVisitor() throws Exception { skyframeExecutor = getSkyframeExecutor(); visitor = skyframeExecutor.getPackageManager().transitiveLoader(); options = @@ -694,8 +693,7 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) new ActionsTestUtil.FakeArtifactResolverBase(), new ActionKeyContext(), Predicates.alwaysTrue(), - null, - PathFragment.create("bazel-out")); + /*cacheConfig=*/ null); private static final ProgressSupplier EMPTY_PROGRESS_SUPPLIER = new ProgressSupplier() { @Override @@ -791,7 +789,7 @@ public void testSharedActionsRacing() throws Exception { Path root = getExecRoot(); PathFragment execPath = PathFragment.create("out").getRelative("file"); Path sourcePath = rootDirectory.getRelative("foo/src"); - FileSystemUtils.createDirectoryAndParents(sourcePath.getParentDirectory()); + sourcePath.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.createEmptyFile(sourcePath); // We create two "configured targets" and two copies of the same artifact, each generated by @@ -1340,7 +1338,7 @@ public void incrementalSharedActions() throws Exception { PathFragment relativeOut = PathFragment.create("out"); PathFragment execPath = relativeOut.getRelative("file"); Path sourcePath = rootDirectory.getRelative("foo/src"); - FileSystemUtils.createDirectoryAndParents(sourcePath.getParentDirectory()); + sourcePath.getParentDirectory().createDirectoryAndParents(); FileSystemUtils.createEmptyFile(sourcePath); // We create two "configured targets" and two copies of the same artifact, each generated by @@ -1609,14 +1607,13 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) } } - private BinTools setupEmbeddedArtifacts() throws IOException { + private void setupEmbeddedArtifacts() throws IOException { List embeddedTools = analysisMock.getEmbeddedTools(); directories.getEmbeddedBinariesRoot().createDirectoryAndParents(); for (String embeddedToolName : embeddedTools) { Path toolPath = directories.getEmbeddedBinariesRoot().getRelative(embeddedToolName); FileSystemUtils.touchFile(toolPath); } - return BinTools.forIntegrationTesting(directories, embeddedTools); } /** Test appropriate behavior when an action halts the build with a catastrophic failure. */ diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 88e3072c8b99b6..c8bbdff70d55d5 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -85,7 +85,6 @@ import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionCompletedReceiver; -import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ProgressSupplier; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.testutil.FoundationTestCase; import com.google.devtools.build.lib.testutil.TestConstants; @@ -247,7 +246,6 @@ protected BuilderWithResult createBuilder( MetadataConsumerForMetrics.NO_OP, new AtomicReference<>(statusReporter), /*sourceRootSupplier=*/ ImmutableList::of, - PathFragment.create(directories.getRelativeOutputPath()), new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS), k -> ThreadStateReceiver.NULL_INSTANCE); @@ -332,7 +330,7 @@ protected BuilderWithResult createBuilder( /*keepEdges=*/ true); final SequentialBuildDriver driver = new SequentialBuildDriver(evaluator); PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID()); - PrecomputedValue.ACTION_ENV.set(differencer, ImmutableMap.of()); + PrecomputedValue.ACTION_ENV.set(differencer, ImmutableMap.of()); PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); return new BuilderWithResult() { @@ -380,17 +378,12 @@ public void buildArtifacts( executor, options, new ActionCacheChecker( - actionCache, - null, - actionKeyContext, - ALWAYS_EXECUTE_FILTER, - null, - PathFragment.create("bazel-out")), + actionCache, null, actionKeyContext, ALWAYS_EXECUTE_FILTER, null), topDownActionCache, /*outputService=*/ null, /*incrementalAnalysis=*/ true); skyframeActionExecutor.setActionExecutionProgressReportingObjects( - EMPTY_PROGRESS_SUPPLIER, EMPTY_COMPLETION_RECEIVER); + () -> "", EMPTY_COMPLETION_RECEIVER); List keys = new ArrayList<>(); for (Artifact artifact : artifacts) { @@ -659,14 +652,6 @@ public String extractTag(SkyKey skyKey) { } } - private static final ProgressSupplier EMPTY_PROGRESS_SUPPLIER = - new ProgressSupplier() { - @Override - public String getProgressString() { - return ""; - } - }; - private static final ActionCompletedReceiver EMPTY_COMPLETION_RECEIVER = new ActionCompletedReceiver() { @Override diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index 53435c6d590e17..6223c04c4f00cf 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -54,11 +54,10 @@ @RunWith(TestParameterInjector.class) public final class TreeArtifactValueTest { - private static final PathFragment BIN_PATH = PathFragment.create("bin"); - private final Scratch scratch = new Scratch(); private final ArtifactRoot root = - ArtifactRoot.asDerivedRoot(scratch.resolve("root"), RootType.Output, BIN_PATH); + ArtifactRoot.asDerivedRoot( + scratch.resolve("root"), RootType.Output, PathFragment.create("bin")); @Test public void createsCorrectValue() { @@ -687,7 +686,7 @@ public void multiBuilder_removeAndRecreateValue_injectsValueAfterRemove() { } private static ArchivedTreeArtifact createArchivedTreeArtifact(SpecialArtifact specialArtifact) { - return ArchivedTreeArtifact.createForTree(specialArtifact, BIN_PATH); + return ArchivedTreeArtifact.createForTree(specialArtifact); } private SpecialArtifact createTreeArtifact(String execPath) { diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 3a3218cb79f66f..286e508c07c3f0 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3204,4 +3204,60 @@ EOF expect_not_log "WARNING: Writing to Remote Cache:" } +function test_download_toplevel_when_turn_remote_cache_off() { + download_toplevel_when_turn_remote_cache_off +} + +function test_download_toplevel_when_turn_remote_cache_off_with_metadata() { + download_toplevel_when_turn_remote_cache_off --experimental_action_cache_store_output_metadata +} + +function download_toplevel_when_turn_remote_cache_off() { + # Test that BwtB doesn't cause build failure if remote cache is disabled in a following build. + # See https://github.com/bazelbuild/bazel/issues/13882. + + cat > .bazelrc < a/BUILD <<'EOF' +genrule( + name = "producer", + outs = ["a.txt", "b.txt"], + cmd = "touch $(OUTS)", +) +genrule( + name = "consumer", + outs = ["out.txt"], + srcs = [":b.txt", "in.txt"], + cmd = "cat $(SRCS) > $@", +) +EOF + echo 'foo' > a/in.txt + + # populate the cache + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --remote_download_toplevel \ + //a:consumer >& $TEST_log || fail "Failed to populate the cache" + + bazel clean >& $TEST_log || fail "Failed to clean" + + # download top level outputs + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + --remote_download_toplevel \ + "$@" \ + //a:consumer >& $TEST_log || fail "Failed to download outputs" + [[ -f bazel-bin/a/a.txt ]] || [[ -f bazel-bin/a/b.txt ]] \ + && fail "Expected outputs of producer are not downloaded" + + # build without remote cache + echo 'bar' > a/in.txt + bazel build \ + --remote_download_toplevel \ + "$@" \ + //a:consumer >& $TEST_log || fail "Failed to build without remote cache" +} + run_suite "Remote execution and remote cache tests"