From 53f189973e1372d53c7cc2a041a3e34adcff4fbe Mon Sep 17 00:00:00 2001 From: jhorvitz Date: Fri, 29 Oct 2021 10:00:35 -0700 Subject: [PATCH] In `ArchivedTreeArtifact`, make the assumption that the relative output path is always a single segment, and use this to serialize less data. This assumption holds because the origin of the relative output path (i.e. `bazel-out`) is `BlazeDirectories#getRelativeOutputPath`, which always returns a single-segment string. Instead of passing that around, just extract it from the tree artifact's exec path. Additionally, the public `ArchivedTreeArtifact#create` method is removed in order to enforce a consistent naming pattern for all instances. The codec supports custom derived tree roots even though there is currently no such serialization in skyframe (all serialized instances have the default `:archived_tree_artifacts`, but it was easy enough to be flexible). PiperOrigin-RevId: 406382340 --- .../build/lib/actions/AbstractAction.java | 18 ++- .../devtools/build/lib/actions/Action.java | 3 +- .../build/lib/actions/ActionCacheChecker.java | 23 +--- .../devtools/build/lib/actions/Artifact.java | 127 ++++++++++-------- .../lib/bazel/BazelWorkspaceStatusModule.java | 8 +- .../build/lib/buildtool/ExecutionTool.java | 3 +- .../lib/rules/cpp/CreateIncSymlinkAction.java | 5 +- .../lib/rules/java/JavaCompileAction.java | 2 +- .../lib/runtime/commands/RunCommand.java | 2 +- .../lib/skyframe/ActionExecutionValue.java | 5 +- .../lib/skyframe/ActionMetadataHandler.java | 16 +-- .../lib/skyframe/SkyframeActionExecutor.java | 6 +- .../build/lib/skyframe/SkyframeExecutor.java | 4 +- .../lib/actions/ActionCacheCheckerTest.java | 61 ++++----- .../build/lib/actions/ArtifactTest.java | 80 +++-------- .../CompactPersistentActionCacheTest.java | 6 +- .../rules/cpp/CreateIncSymlinkActionTest.java | 2 +- ...ValueTransformSharedTreeArtifactsTest.java | 3 +- ...lesystemValueCheckerParameterizedTest.java | 3 +- .../SequencedSkyframeExecutorTest.java | 13 +- .../skyframe/TimestampBuilderTestCase.java | 21 +-- .../lib/skyframe/TreeArtifactValueTest.java | 7 +- 22 files changed, 164 insertions(+), 254 deletions(-) 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 b286e9d995aae7..5a737cf25caa71 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..8dfe7478077222 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); @@ -466,8 +458,7 @@ public Token getTokenIfNeedToExecute( CachedOutputMetadata cachedOutputMetadata = null; // load remote metadata from action cache if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) { - cachedOutputMetadata = - loadCachedOutputMetadata(action, entry, metadataHandler, derivedPathPrefix); + 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 e5e927c2456470..121307784b5254 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 4ca0d4491e4746..84915e6d93ee8f 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/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 9b2fb11cc22b5d..56d20e9d76d15c 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 a38cb9c3dd1f64..cf677050061405 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..f943f3aad1d93e 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; } @@ -990,7 +986,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 2bb0fd1fc721fb..f7644e8ef83321 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 @@ -457,13 +457,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 = 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..00e67ad0957a6a 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( @@ -440,7 +438,7 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio /*resolvedCacheArtifacts=*/ null, /*clientEnv=*/ ImmutableMap.of(), /*handler=*/ null, - new FakeMetadataHandler(derivedPathPrefix), + new FakeMetadataHandler(), /*artifactExpander=*/ null, /*remoteDefaultPlatformProperties=*/ ImmutableMap.of())) .isNotNull(); @@ -451,7 +449,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 +460,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 +546,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 = @@ -671,7 +668,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 = @@ -755,7 +752,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 = @@ -796,7 +793,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 +833,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 +857,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 +877,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"); @@ -926,12 +921,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 +941,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 +1014,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 +1074,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 +1089,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 +1116,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 +1130,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) {