Move creating directories for action outputs from `SkyframeActionExecutor` to a
separate class.
The logic creating output directories for actions is complicated enough to
deserve a class of its own. Move the logic to the new class and add proper unit
tests now that we have a clean-cut on its API.
Add test helpers for easy listing of leaf entries in directories.
PiperOrigin-RevId: 375115886
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java
new file mode 100644
index 0000000..59984af
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelper.java
@@ -0,0 +1,246 @@
+// Copyright 2014 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheBuilderSpec;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
+import com.google.common.util.concurrent.Striped;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.vfs.FileStatus;
+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.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.locks.Lock;
+
+/**
+ * Helper class to create directories for {@linkplain com.google.devtools.build.lib.actions.Action
+ * action} outputs.
+ */
+public final class ActionOutputDirectoryHelper {
+
+ // Used to prevent check-then-act races in #createOutputDirectories. See the comment there for
+ // more detail.
+ private static final Striped<Lock> outputDirectoryDeletionLock = Striped.lock(64);
+
+ // Directories which are known to be created as regular directories within this invocation. This
+ // implies parent directories are also regular directories.
+ private final Map<PathFragment, DirectoryState> knownDirectories;
+
+ private enum DirectoryState {
+ FOUND,
+ CREATED
+ }
+
+ ActionOutputDirectoryHelper(CacheBuilderSpec cacheBuilderSpec) {
+ knownDirectories =
+ CacheBuilder.from(cacheBuilderSpec)
+ .concurrencyLevel(Runtime.getRuntime().availableProcessors())
+ .<PathFragment, DirectoryState>build()
+ .asMap();
+ }
+
+ /**
+ * Create output directories for an ActionFS. The action-local filesystem starts empty, so we
+ * expect the output directory creation to always succeed. There can be no interference from state
+ * left behind by prior builds or other actions intra-build.
+ */
+ void createActionFsOutputDirectories(
+ ImmutableSet<Artifact> actionOutputs, ArtifactPathResolver artifactPathResolver)
+ throws CreateOutputDirectoryException {
+ Set<Path> done = new HashSet<>(); // avoid redundant calls for the same directory.
+ for (Artifact outputFile : actionOutputs) {
+ Path outputDir;
+ if (outputFile.isTreeArtifact()) {
+ outputDir = artifactPathResolver.toPath(outputFile);
+ } else {
+ outputDir = artifactPathResolver.toPath(outputFile).getParentDirectory();
+ }
+
+ if (done.add(outputDir)) {
+ try {
+ outputDir.createDirectoryAndParents();
+ } catch (IOException e) {
+ throw new CreateOutputDirectoryException(outputDir.asFragment(), e);
+ }
+ }
+ }
+ }
+
+ /**
+ * Creates output directories, with ancestors, for all action outputs.
+ *
+ * <p>For regular file outputs, creates the parent directories, for tree outputs creates the tree
+ * directory (with ancestors in both cases).
+ *
+ * <p>It is only valid to use this method when not using an action file system, in which case
+ * please use {@link #createActionFsOutputDirectories} instead. In particular, {@linkplain
+ * #knownDirectories the cache of directories}, shared across actions will cause common
+ * directories to be created in action file system for only one of the actions as opposed to all
+ * of them.
+ */
+ void createOutputDirectories(ImmutableSet<Artifact> actionOutputs)
+ throws CreateOutputDirectoryException {
+ Set<Path> done = new HashSet<>(); // avoid redundant calls for the same directory.
+ for (Artifact outputFile : actionOutputs) {
+ Path outputDir;
+ // Given we know that we are not using action file system, we can get safely get paths
+ // directly from the artifacts.
+ if (outputFile.isTreeArtifact()) {
+ outputDir = outputFile.getPath();
+ } else {
+ outputDir = outputFile.getPath().getParentDirectory();
+ }
+
+ if (done.add(outputDir)) {
+ try {
+ createAndCheckForSymlinks(outputDir, outputFile);
+ continue;
+ } catch (IOException e) {
+ /* Fall through to plan B. */
+ }
+
+ // Possibly some direct ancestors are not directories. In that case, we traverse the
+ // ancestors downward, deleting any non-directories. This handles the case where a file
+ // becomes a directory. The traversal is done downward because otherwise we may delete
+ // files through a symlink in a parent directory. Since Blaze never creates such
+ // directories within a build, we have no idea where on disk we're actually deleting.
+ //
+ // Symlinks should not be followed so in order to clean up symlinks pointing to Fileset
+ // outputs from previous builds. See bug [incremental build of Fileset fails if
+ // Fileset.out was changed to be a subdirectory of the old value].
+ try {
+ Path p = outputFile.getRoot().getRoot().asPath();
+ for (String segment : outputDir.relativeTo(p).segments()) {
+ p = p.getRelative(segment);
+
+ // This lock ensures that the only thread that observes a filesystem transition in
+ // which the path p first exists and then does not is the thread that calls
+ // p.delete() and causes the transition.
+ //
+ // If it were otherwise, then some thread A could test p.exists(), see that it does,
+ // then test p.isDirectory(), see that p isn't a directory (because, say, thread
+ // B deleted it), and then call p.delete(). That could result in two different kinds
+ // of failures:
+ //
+ // 1) In the time between when thread A sees that p is not a directory and when thread
+ // A calls p.delete(), thread B may reach the call to createDirectoryAndParents
+ // and create a directory at p, which thread A then deletes. Thread B would then try
+ // adding outputs to the directory it thought was there, and fail.
+ //
+ // 2) In the time between when thread A sees that p is not a directory and when thread
+ // A calls p.delete(), thread B may create a directory at p, and then either create a
+ // subdirectory beneath it or add outputs to it. Then when thread A tries to delete p,
+ // it would fail.
+ Lock lock = outputDirectoryDeletionLock.get(p);
+ lock.lock();
+ try {
+ FileStatus stat = p.statIfFound(Symlinks.NOFOLLOW);
+ if (stat == null) {
+ // Missing entry: Break out and create expected directories.
+ break;
+ }
+ if (stat.isDirectory()) {
+ // If this directory used to be a tree artifact it won't be writable.
+ p.setWritable(true);
+ knownDirectories.put(p.asFragment(), DirectoryState.FOUND);
+ } else {
+ // p may be a file or symlink (possibly from a Fileset in a previous build).
+ p.delete(); // throws IOException
+ break;
+ }
+ } finally {
+ lock.unlock();
+ }
+ }
+ outputDir.createDirectoryAndParents();
+ } catch (IOException e) {
+ throw new CreateOutputDirectoryException(outputDir.asFragment(), e);
+ }
+ }
+ }
+ }
+
+ /**
+ * Create an output directory and ensure that no symlinks exists between the output root and the
+ * output file. These are all expected to be regular directories. Violations of this expectations
+ * can only come from state left behind by previous invocations or external filesystem mutation.
+ */
+ private void createAndCheckForSymlinks(Path dir, Artifact outputFile) throws IOException {
+ Path rootPath = outputFile.getRoot().getRoot().asPath();
+ PathFragment root = rootPath.asFragment();
+
+ // If the output root has not been created yet, do so now.
+ if (!knownDirectories.containsKey(root)) {
+ FileStatus stat = rootPath.statNullable(Symlinks.NOFOLLOW);
+ if (stat == null) {
+ rootPath.createDirectoryAndParents();
+ knownDirectories.put(root, DirectoryState.CREATED);
+ } else {
+ knownDirectories.put(root, DirectoryState.FOUND);
+ }
+ }
+
+ // Walk up until the first known directory is found (must be root or below).
+ List<Path> checkDirs = new ArrayList<>();
+ while (!knownDirectories.containsKey(dir.asFragment())) {
+ checkDirs.add(dir);
+ dir = dir.getParentDirectory();
+ }
+
+ // Check in reverse order (parent directory first):
+ // - If symlink -> Exception.
+ // - If non-existent -> Create directory and all children.
+ boolean parentCreated = knownDirectories.get(dir.asFragment()) == DirectoryState.CREATED;
+ for (Path path : Lists.reverse(checkDirs)) {
+ if (parentCreated) {
+ // If we have created this directory's parent, we know that it doesn't exist or else we
+ // would know about it already. Even if a parallel thread has created it in the meantime,
+ // createDirectory() will return normally and we can assume that a regular directory exists
+ // afterwards.
+ path.createDirectory();
+ knownDirectories.put(path.asFragment(), DirectoryState.CREATED);
+ continue;
+ }
+ FileStatus stat = path.statNullable(Symlinks.NOFOLLOW);
+ if (stat != null && !stat.isDirectory()) {
+ throw new IOException(dir + " is not a regular directory");
+ }
+ if (stat == null) {
+ parentCreated = true;
+ path.createDirectory();
+ knownDirectories.put(path.asFragment(), DirectoryState.CREATED);
+ } else {
+ knownDirectories.put(path.asFragment(), DirectoryState.FOUND);
+ }
+ }
+ }
+
+ static final class CreateOutputDirectoryException extends IOException {
+ final PathFragment directoryPath;
+
+ private CreateOutputDirectoryException(PathFragment directoryPath, IOException cause) {
+ super(cause.getMessage(), cause);
+ this.directoryPath = directoryPath;
+ }
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 5f54fee..89bcca6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -19,6 +19,7 @@
"ActionArtifactCycleReporter.java",
"ActionExecutionFunction.java",
"ActionLookupConflictFindingFunction.java",
+ "ActionOutputDirectoryHelper.java",
"AspectCompletionValue.java",
"AspectCompletor.java",
"AspectFunction.java",
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 1a024fc..7c9fd4e 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
@@ -14,17 +14,13 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Preconditions;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
-import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.flogger.GoogleLogger;
import com.google.common.util.concurrent.ListenableFuture;
-import com.google.common.util.concurrent.Striped;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionCacheChecker;
@@ -97,17 +93,16 @@
import com.google.devtools.build.lib.skyframe.ActionExecutionState.ActionStep;
import com.google.devtools.build.lib.skyframe.ActionExecutionState.ActionStepOrResult;
import com.google.devtools.build.lib.skyframe.ActionExecutionState.SharedActionCallback;
+import com.google.devtools.build.lib.skyframe.ActionOutputDirectoryHelper.CreateOutputDirectoryException;
import com.google.devtools.build.lib.util.CrashFailureDetails;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.io.FileOutErr;
-import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystem;
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.Symlinks;
import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
@@ -115,15 +110,12 @@
import java.io.Closeable;
import java.io.FileNotFoundException;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicReference;
-import java.util.concurrent.locks.Lock;
import java.util.function.Supplier;
import javax.annotation.Nullable;
@@ -135,10 +127,6 @@
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
- // Used to prevent check-then-act races in #createOutputDirectories. See the comment there for
- // more detail.
- private static final Striped<Lock> outputDirectoryDeletionLock = Striped.lock(64);
-
private static final MetadataInjector THROWING_METADATA_INJECTOR_FOR_ACTIONFS =
new MetadataInjector() {
@Override
@@ -204,14 +192,7 @@
// at the start of its next attempt,
private ConcurrentMap<OwnerlessArtifactWrapper, ImmutableList<SkyKey>> lostDiscoveredInputsMap;
- // Directories which are known to be created as regular directories within this invocation. This
- // implies parent directories are also regular directories.
- private Map<PathFragment, DirectoryState> knownDirectories;
-
- private enum DirectoryState {
- FOUND,
- CREATED
- }
+ private ActionOutputDirectoryHelper outputDirectoryHelper;
private OptionsProvider options;
private boolean useAsyncExecution;
@@ -296,11 +277,9 @@
this.replayActionOutErr = options.getOptions(BuildRequestOptions.class).replayActionOutErr;
this.outputService = outputService;
- Cache<PathFragment, DirectoryState> cache =
- CacheBuilder.from(options.getOptions(BuildRequestOptions.class).directoryCreationCacheSpec)
- .concurrencyLevel(Runtime.getRuntime().availableProcessors())
- .build();
- this.knownDirectories = cache.asMap();
+ this.outputDirectoryHelper =
+ new ActionOutputDirectoryHelper(
+ options.getOptions(BuildRequestOptions.class).directoryCreationCacheSpec);
// Retaining discovered inputs is only worthwhile for incremental builds or builds with extra
// actions, which consume their shadowed action's discovered inputs.
@@ -376,7 +355,7 @@
this.lostDiscoveredInputsMap = null;
this.actionCacheChecker = null;
this.topDownActionCache = null;
- this.knownDirectories = null;
+ this.outputDirectoryHelper = null;
}
/**
@@ -1014,9 +993,9 @@
setupActionFsFileOutErr(actionExecutionContext.getFileOutErr(), action);
}
if (actionFileSystemType().inMemoryFileSystem()) {
- createActionFsOutputDirectories(action, actionExecutionContext);
+ createActionFsOutputDirectories(action, actionExecutionContext.getPathResolver());
} else {
- createOutputDirectories(action, actionExecutionContext);
+ createOutputDirectories(action);
}
} catch (ActionExecutionException e) {
// This try-catch block cannot trigger rewinding, so it is safe to notify the status
@@ -1322,25 +1301,14 @@
* expect the output directory creation to always succeed. There can be no interference from state
* left behind by prior builds or other actions intra-build.
*/
- private void createActionFsOutputDirectories(Action action, ActionExecutionContext context)
- throws ActionExecutionException {
+ private void createActionFsOutputDirectories(
+ Action action, ArtifactPathResolver artifactPathResolver) throws ActionExecutionException {
try {
- Set<Path> done = new HashSet<>(); // avoid redundant calls for the same directory.
- for (Artifact outputFile : action.getOutputs()) {
- Path outputDir;
- if (outputFile.isTreeArtifact()) {
- outputDir = context.getPathResolver().toPath(outputFile);
- } else {
- outputDir = context.getPathResolver().toPath(outputFile).getParentDirectory();
- }
-
- if (done.add(outputDir)) {
- outputDir.createDirectoryAndParents();
- }
- }
- } catch (IOException e) {
+ outputDirectoryHelper.createActionFsOutputDirectories(
+ action.getOutputs(), artifactPathResolver);
+ } catch (CreateOutputDirectoryException e) {
throw toActionExecutionException(
- "failed to create output directory",
+ String.format("failed to create output directory '%s'", e.directoryPath),
e,
action,
null,
@@ -1348,152 +1316,17 @@
}
}
- /**
- * Create an output directory and ensure that no symlinks exists between the output root and the
- * output file. These are all expected to be regular directories. Violations of this expectations
- * can only come from state left behind by previous invocations or external filesystem mutation.
- */
- private void createAndCheckForSymlinks(
- final Path dir, final Artifact outputFile, ActionExecutionContext context)
- throws IOException {
- Path rootPath = outputFile.getRoot().getRoot().asPath();
- PathFragment root = rootPath.asFragment();
- Path curDir = context.getPathResolver().convertPath(dir);
-
- // If the output root has not been created yet, do so now.
- if (!knownDirectories.containsKey(root)) {
- FileStatus stat = rootPath.statNullable(Symlinks.NOFOLLOW);
- if (stat == null) {
- outputFile.getRoot().getRoot().asPath().createDirectoryAndParents();
- knownDirectories.put(root, DirectoryState.CREATED);
- } else {
- knownDirectories.put(root, DirectoryState.FOUND);
- }
- }
-
- // Walk up until the first known directory is found (must be root or below).
- List<Path> checkDirs = new ArrayList<>();
- while (!knownDirectories.containsKey(curDir.asFragment())) {
- checkDirs.add(curDir);
- curDir = curDir.getParentDirectory();
- }
-
- // Check in reverse order (parent directory first):
- // - If symlink -> Exception.
- // - If non-existent -> Create directory and all children.
- boolean parentCreated = knownDirectories.get(curDir.asFragment()) == DirectoryState.CREATED;
- for (Path path : Lists.reverse(checkDirs)) {
- if (parentCreated) {
- // If we have created this directory's parent, we know that it doesn't exist or else we
- // would know about it already. Even if a parallel thread has created it in the meantime,
- // createDirectory() will return normally and we can assume that a regular directory exists
- // afterwards.
- path.createDirectory();
- knownDirectories.put(path.asFragment(), DirectoryState.CREATED);
- continue;
- }
- FileStatus stat = path.statNullable(Symlinks.NOFOLLOW);
- if (stat != null && !stat.isDirectory()) {
- throw new IOException(curDir + " is not a regular directory");
- }
- if (stat == null) {
- parentCreated = true;
- path.createDirectory();
- knownDirectories.put(path.asFragment(), DirectoryState.CREATED);
- } else {
- knownDirectories.put(path.asFragment(), DirectoryState.FOUND);
- }
- }
- }
-
- private void createOutputDirectories(Action action, ActionExecutionContext context)
- throws ActionExecutionException {
+ private void createOutputDirectories(Action action) throws ActionExecutionException {
try {
- Set<Path> done = new HashSet<>(); // avoid redundant calls for the same directory.
- for (Artifact outputFile : action.getOutputs()) {
- Path outputDir;
- if (outputFile.isTreeArtifact()) {
- outputDir = context.getPathResolver().toPath(outputFile);
- } else {
- outputDir = context.getPathResolver().toPath(outputFile).getParentDirectory();
- }
-
- if (done.add(outputDir)) {
- try {
- createAndCheckForSymlinks(outputDir, outputFile, context);
- continue;
- } catch (IOException e) {
- /* Fall through to plan B. */
- }
-
- // Possibly some direct ancestors are not directories. In that case, we traverse the
- // ancestors downward, deleting any non-directories. This handles the case where a file
- // becomes a directory. The traversal is done downward because otherwise we may delete
- // files through a symlink in a parent directory. Since Blaze never creates such
- // directories within a build, we have no idea where on disk we're actually deleting.
- //
- // Symlinks should not be followed so in order to clean up symlinks pointing to Fileset
- // outputs from previous builds. See bug [incremental build of Fileset fails if
- // Fileset.out was changed to be a subdirectory of the old value].
- try {
- Path p =
- context.getPathResolver().transformRoot(outputFile.getRoot().getRoot()).asPath();
- for (String segment : outputDir.relativeTo(p).segments()) {
- p = p.getRelative(segment);
-
- // This lock ensures that the only thread that observes a filesystem transition in
- // which the path p first exists and then does not is the thread that calls
- // p.delete() and causes the transition.
- //
- // If it were otherwise, then some thread A could test p.exists(), see that it does,
- // then test p.isDirectory(), see that p isn't a directory (because, say, thread
- // B deleted it), and then call p.delete(). That could result in two different kinds
- // of failures:
- //
- // 1) In the time between when thread A sees that p is not a directory and when thread
- // A calls p.delete(), thread B may reach the call to createDirectoryAndParents
- // and create a directory at p, which thread A then deletes. Thread B would then try
- // adding outputs to the directory it thought was there, and fail.
- //
- // 2) In the time between when thread A sees that p is not a directory and when thread
- // A calls p.delete(), thread B may create a directory at p, and then either create a
- // subdirectory beneath it or add outputs to it. Then when thread A tries to delete p,
- // it would fail.
- Lock lock = outputDirectoryDeletionLock.get(p);
- lock.lock();
- try {
- FileStatus stat = p.statIfFound(Symlinks.NOFOLLOW);
- if (stat == null) {
- // Missing entry: Break out and create expected directories.
- break;
- }
- if (stat.isDirectory()) {
- // If this directory used to be a tree artifact it won't be writable.
- p.setWritable(true);
- knownDirectories.put(p.asFragment(), DirectoryState.FOUND);
- } else {
- // p may be a file or symlink (possibly from a Fileset in a previous build).
- p.delete(); // throws IOException
- break;
- }
- } finally {
- lock.unlock();
- }
- }
- outputDir.createDirectoryAndParents();
- } catch (IOException e) {
- String message =
- String.format(
- "failed to create output directory '%s': %s", outputDir, e.getMessage());
- DetailedExitCode code =
- createDetailedExitCode(message, Code.ACTION_OUTPUT_DIRECTORY_CREATION_FAILURE);
- throw new ActionExecutionException(message, e, action, false, code);
- }
- }
- }
- } catch (ActionExecutionException ex) {
- printError(ex.getMessage(), action, null);
- throw ex;
+ outputDirectoryHelper.createOutputDirectories(action.getOutputs());
+ } catch (CreateOutputDirectoryException e) {
+ throw toActionExecutionException(
+ String.format(
+ "failed to create output directory '%s': %s", e.directoryPath, e.getMessage()),
+ e,
+ action,
+ /*actionOutput=*/ null,
+ Code.ACTION_OUTPUT_DIRECTORY_CREATION_FAILURE);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/testing/common/BUILD b/src/main/java/com/google/devtools/build/lib/testing/common/BUILD
index 747a907..0055b03 100644
--- a/src/main/java/com/google/devtools/build/lib/testing/common/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/testing/common/BUILD
@@ -1,6 +1,7 @@
load("@rules_java//java:defs.bzl", "java_library")
package(
+ default_testonly = True,
default_visibility = ["//src:__subpackages__"],
)
@@ -8,13 +9,13 @@
filegroup(
name = "srcs",
+ testonly = False,
srcs = glob(["**"]),
visibility = ["//src:__subpackages__"],
)
java_library(
name = "fake-options",
- testonly = True,
srcs = ["FakeOptions.java"],
deps = [
"//src/main/java/com/google/devtools/common/options",
@@ -22,3 +23,12 @@
"//third_party:jsr305",
],
)
+
+java_library(
+ name = "directory_listing_helper",
+ srcs = ["DirectoryListingHelper.java"],
+ deps = [
+ "//src/main/java/com/google/devtools/build/lib/vfs",
+ "//third_party:guava",
+ ],
+)
diff --git a/src/main/java/com/google/devtools/build/lib/testing/common/DirectoryListingHelper.java b/src/main/java/com/google/devtools/build/lib/testing/common/DirectoryListingHelper.java
new file mode 100644
index 0000000..459572e
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/testing/common/DirectoryListingHelper.java
@@ -0,0 +1,68 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.testing.common;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
+import java.io.IOException;
+
+/** Namespace for helpers to test recursive directory listings. */
+public final class DirectoryListingHelper {
+
+ private DirectoryListingHelper() {}
+
+ /** Shorthand for {@link Dirent} of {@link Dirent.Type#DIRECTORY} type with a given name. */
+ public static Dirent directory(String name) {
+ return new Dirent(name, Dirent.Type.DIRECTORY);
+ }
+
+ /**
+ * Returns all of the leaf {@linkplain Dirent dirents} under a given directory.
+ *
+ * <p>For directory structure of:
+ *
+ * <pre>
+ * dir/dir2
+ * dir/file1
+ * dir/subdir/file2
+ * </pre>
+ *
+ * will return: {@code FILE(dir/file1), FILE(dir/subdir/file2), DIRECTORY(dir/dir2)}.
+ */
+ public static ImmutableList<Dirent> leafDirectoryEntries(Path path) throws IOException {
+ ImmutableList.Builder<Dirent> entries = ImmutableList.builder();
+ leafDirectoryEntriesInternal(path, "", entries);
+ return entries.build();
+ }
+
+ private static void leafDirectoryEntriesInternal(
+ Path path, String prefix, ImmutableList.Builder<Dirent> entries) throws IOException {
+ boolean isEmpty = true;
+ for (Dirent dirent : path.readdir(Symlinks.NOFOLLOW)) {
+ if (dirent.getType() == Dirent.Type.DIRECTORY) {
+ leafDirectoryEntriesInternal(
+ path.getChild(dirent.getName()),
+ prefix.isEmpty() ? dirent.getName() : prefix + "/" + dirent.getName(),
+ entries);
+ }
+ isEmpty = false;
+ }
+
+ if (isEmpty) {
+ entries.add(new Dirent(prefix, Dirent.Type.DIRECTORY));
+ }
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelperTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelperTest.java
new file mode 100644
index 0000000..b2bdcc2
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputDirectoryHelperTest.java
@@ -0,0 +1,218 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testing.common.DirectoryListingHelper.leafDirectoryEntries;
+import static org.junit.Assert.assertThrows;
+
+import com.google.common.cache.CacheBuilderSpec;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.skyframe.ActionOutputDirectoryHelper.CreateOutputDirectoryException;
+import com.google.devtools.build.lib.testing.common.DirectoryListingHelper;
+import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.vfs.DelegateFileSystem;
+import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import java.io.IOException;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+/** Tests for {@link ActionOutputDirectoryHelper}. */
+@RunWith(TestParameterInjector.class)
+public class ActionOutputDirectoryHelperTest {
+
+ private static final CacheBuilderSpec CACHE_ENABLED =
+ CacheBuilderSpec.parse("maximumSize=100000");
+
+ private Path execRoot;
+ private ArtifactRoot outputRoot;
+
+ @Before
+ public void createArtifactRootAndOutputDirectoryHelper() throws IOException {
+ Scratch scratch = new Scratch();
+ execRoot = scratch.dir("/execroot");
+ outputRoot = createOutputRoot(execRoot);
+ }
+
+ private enum OutputSet {
+ SINGLE_FILE(
+ /*fileOutputs=*/ ImmutableSet.of("a/b"),
+ /*treeOutputs=*/ ImmutableSet.of(),
+ /*expectedDirectories=*/ ImmutableList.of("a")),
+ DEEP_DIRECTORY_STRUCTURE(
+ /*fileOutputs=*/ ImmutableSet.of("a/b/c/d/e/f/g/h/i/j/k/l/m"),
+ /*treeOutputs=*/ ImmutableSet.of(),
+ /*expectedDirectories=*/ ImmutableList.of("a/b/c/d/e/f/g/h/i/j/k/l")),
+ MULTIPLE_FILES(
+ /*fileOutputs=*/ ImmutableSet.of("a/b/c", "a/c", "a/d/1", "a/d/2"),
+ /*treeOutputs=*/ ImmutableSet.of(),
+ /*expectedDirectories=*/ ImmutableList.of("a/b", "a/d")),
+ TREE_OUTPUT(
+ /*fileOutputs=*/ ImmutableSet.of(),
+ /*treeOutputs=*/ ImmutableSet.of("a/b"),
+ /*expectedDirectories=*/ ImmutableList.of("a/b"));
+
+ OutputSet(
+ ImmutableSet<String> fileOutputs,
+ ImmutableSet<String> treeOutputs,
+ ImmutableList<String> expectedDirectories) {
+ this.fileOutputs = fileOutputs;
+ this.treeOutputs = treeOutputs;
+ this.expectedDirectories = expectedDirectories;
+ }
+
+ ImmutableSet<Artifact> actionOutputs(ActionOutputDirectoryHelperTest test) {
+ ImmutableSet.Builder<Artifact> outs =
+ ImmutableSet.builderWithExpectedSize(fileOutputs.size() + treeOutputs.size());
+ fileOutputs.stream().map(test::createOutput).forEach(outs::add);
+ treeOutputs.stream().map(test::createTreeOutput).forEach(outs::add);
+ return outs.build();
+ }
+
+ ImmutableList<Dirent> expectedDirectoryEntries() {
+ return expectedDirectories.stream()
+ .map(DirectoryListingHelper::directory)
+ .collect(toImmutableList());
+ }
+
+ private final ImmutableSet<String> fileOutputs;
+ private final ImmutableSet<String> treeOutputs;
+ private final ImmutableList<String> expectedDirectories;
+ }
+
+ @Test
+ public void createOutputDirectories_createsExpectedDirectories(@TestParameter OutputSet outputSet)
+ throws Exception {
+ ActionOutputDirectoryHelper outputDirectoryHelper = createActionOutputDirectoryHelper();
+
+ outputDirectoryHelper.createOutputDirectories(outputSet.actionOutputs(this));
+
+ assertThat(leafDirectoryEntries(outputRoot.getRoot().asPath()))
+ .containsExactlyElementsIn(outputSet.expectedDirectoryEntries());
+ }
+
+ @Test
+ public void createActionFsOutputDirectories_createsExpectedDirectoriesInActionFs(
+ @TestParameter OutputSet outputSet) throws Exception {
+ ActionOutputDirectoryHelper outputDirectoryHelper = createActionOutputDirectoryHelper();
+ FileSystem actionFileSystem = new Scratch().getFileSystem();
+ ArtifactPathResolver resolver =
+ ArtifactPathResolver.createPathResolver(actionFileSystem, execRoot);
+
+ outputDirectoryHelper.createActionFsOutputDirectories(outputSet.actionOutputs(this), resolver);
+
+ Path outputRootPath = outputRoot.getRoot().asPath();
+ assertThat(outputRootPath.exists()).isFalse();
+ assertThat(leafDirectoryEntries(actionFileSystem.getPath(outputRootPath.asFragment())))
+ .containsExactlyElementsIn(outputSet.expectedDirectoryEntries());
+ }
+
+ @Test
+ public void createOutputDirectories_ioExceptionWhenCreatingDirectory_fails() {
+ ActionOutputDirectoryHelper outputDirectoryHelper = createActionOutputDirectoryHelper();
+ IOException injectedException = new IOException("oh no!");
+ PathFragment outputRootPath = outputRoot.getRoot().asPath().asFragment();
+ FileSystem fsWithFailures =
+ createFileSystemInjectingException(outputRootPath.getRelative("dir"), injectedException);
+ ArtifactRoot rootWithFailure = createOutputRoot(fsWithFailures.getPath(execRoot.asFragment()));
+ ImmutableSet<Artifact> outputs = ImmutableSet.of(createOutput(rootWithFailure, "dir/file"));
+
+ CreateOutputDirectoryException e =
+ assertThrows(
+ CreateOutputDirectoryException.class,
+ () -> outputDirectoryHelper.createOutputDirectories(outputs));
+
+ assertThat(e.directoryPath).isEqualTo(outputRootPath.getRelative("dir"));
+ assertThat(e).hasCauseThat().isSameInstanceAs(injectedException);
+ }
+
+ @Test
+ public void createActionFsOutputDirectories_ioExceptionWhenCreatingDirectory_fails() {
+ ActionOutputDirectoryHelper outputDirectoryHelper = createActionOutputDirectoryHelper();
+ IOException injectedException = new IOException("oh no!");
+ PathFragment outputRootPath = outputRoot.getRoot().asPath().asFragment();
+ FileSystem fsWithFailures =
+ createFileSystemInjectingException(outputRootPath.getRelative("dir"), injectedException);
+ ArtifactRoot rootWithFailure = createOutputRoot(fsWithFailures.getPath(execRoot.asFragment()));
+ ImmutableSet<Artifact> outputs = ImmutableSet.of(createOutput(rootWithFailure, "dir/file"));
+ FileSystem actionFileSystem = new DelegateFileSystem(fsWithFailures) {};
+ ArtifactPathResolver pathResolver =
+ ArtifactPathResolver.createPathResolver(actionFileSystem, execRoot);
+
+ CreateOutputDirectoryException e =
+ assertThrows(
+ CreateOutputDirectoryException.class,
+ () -> outputDirectoryHelper.createActionFsOutputDirectories(outputs, pathResolver));
+
+ assertThat(e.directoryPath).isEqualTo(outputRootPath.getRelative("dir"));
+ assertThat(e).hasCauseThat().isSameInstanceAs(injectedException);
+ }
+
+ private FileSystem createFileSystemInjectingException(
+ PathFragment failingPath, IOException injectedException) {
+ return new DelegateFileSystem(execRoot.getFileSystem()) {
+ @Override
+ public boolean createDirectory(PathFragment path) throws IOException {
+ if (path.equals(failingPath)) {
+ throw injectedException;
+ }
+ return super.createDirectory(path);
+ }
+
+ @Override
+ public void createDirectoryAndParents(PathFragment path) throws IOException {
+ if (path.equals(failingPath)) {
+ throw injectedException;
+ }
+ super.createDirectoryAndParents(path);
+ }
+ };
+ }
+
+ private SpecialArtifact createTreeOutput(String relativeExecPath) {
+ return ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ outputRoot, outputRoot.getExecPath().getRelative(relativeExecPath));
+ }
+
+ private Artifact createOutput(String relativeExecPath) {
+ return createOutput(outputRoot, relativeExecPath);
+ }
+
+ private static Artifact createOutput(ArtifactRoot outputRoot, String relativeExecPath) {
+ return ActionsTestUtil.createArtifactWithRootRelativePath(
+ outputRoot, PathFragment.create(relativeExecPath));
+ }
+
+ private static ArtifactRoot createOutputRoot(Path execRoot) {
+ return ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "out");
+ }
+
+ private static ActionOutputDirectoryHelper createActionOutputDirectoryHelper() {
+ return new ActionOutputDirectoryHelper(CACHE_ENABLED);
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
index 9f6795d..701cb3e 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -124,7 +124,6 @@
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/compilation_mode",
- "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set",
@@ -152,7 +151,6 @@
"//src/main/java/com/google/devtools/build/lib/bazel:main",
"//src/main/java/com/google/devtools/build/lib/bazel/rules",
"//src/main/java/com/google/devtools/build/lib/bugreport",
- "//src/main/java/com/google/devtools/build/lib/causes",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect",
@@ -252,6 +250,7 @@
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils:depsutils",
+ "//src/main/java/com/google/devtools/build/lib/testing/common:directory_listing_helper",
"//src/main/java/net/starlark/java/annot",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
@@ -273,7 +272,6 @@
"//src/test/java/com/google/devtools/build/lib/actions/util",
"//src/test/java/com/google/devtools/build/lib/analysis/testing",
"//src/test/java/com/google/devtools/build/lib/analysis/util",
- "//src/test/java/com/google/devtools/build/lib/analysis/util:test-build-options",
"//src/test/java/com/google/devtools/build/lib/events:testutil",
"//src/test/java/com/google/devtools/build/lib/packages:testutil",
"//src/test/java/com/google/devtools/build/lib/rules/platform:testutil",
@@ -294,6 +292,7 @@
"//third_party:mockito",
"//third_party:truth",
"//third_party/protobuf:protobuf_java",
+ "@com_google_testparameterinjector//:testparameterinjector",
],
)
