Support basic test functionality in ActionFS.

ActionFS now allows output files to be created that do not correspond to known Artifacts.

Note that tests exercise a greater gamut of filesystem functionality (deleting files, deleting directory trees, moving files, etc.)

RELNOTES: None
PiperOrigin-RevId: 199809069

5 files changed, 102 insertions, 40 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
index a8ef2acb40..1a386ab847 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
@@ -167,6 +167,6 @@ public class TestResult {
    */
   private Collection<Pair<String, Path>> getFiles() {
     // TODO(ulfjack): Cache the set of generated files in the TestResultData.
-    return testAction.getTestOutputsMapping(execRoot);
+    return testAction.getTestOutputsMapping(null, execRoot);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index 6c4606df51..f03dcaff33 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -239,13 +239,15 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
    * file system for existence of these output files, so it must only be used after test execution.
    */
   // TODO(ulfjack): Instead of going to local disk here, use SpawnResult (add list of files there).
-  public ImmutableList<Pair<String, Path>> getTestOutputsMapping(Path execRoot) {
+  public ImmutableList<Pair<String, Path>> getTestOutputsMapping(
+      @Nullable ActionExecutionContext context, Path execRoot) {
     ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
-    if (getTestLog().getPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, getTestLog().getPath()));
+    if (convertPath(context, getTestLog()).exists()) {
+      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, convertPath(context, getTestLog())));
     }
-    if (getCoverageData() != null && getCoverageData().getPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE, getCoverageData().getPath()));
+    if (getCoverageData() != null && convertPath(context, getCoverageData()).exists()) {
+      builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE,
+          convertPath(context, getCoverageData())));
     }
     if (execRoot != null) {
       ResolvedPaths resolvedPaths = resolve(execRoot);
@@ -293,6 +295,13 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     return builder.build();
   }
 
+  private static Path convertPath(@Nullable ActionExecutionContext actionContext,
+      Artifact artifact) {
+    return actionContext == null
+        ? artifact.getPath()
+        : actionContext.getInputPath(artifact);
+  }
+
   @Override
   protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
       throws CommandLineExpansionException {
@@ -335,8 +344,10 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   }
 
   /** Saves cache status to disk. */
-  public void saveCacheStatus(TestResultData data) throws IOException {
-    try (OutputStream out = cacheStatus.getPath().getOutputStream()) {
+  public void saveCacheStatus(
+      ActionExecutionContext actionExecutionContext,
+      TestResultData data) throws IOException {
+    try (OutputStream out = actionExecutionContext.getInputPath(cacheStatus).getOutputStream()) {
       data.writeTo(out);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 7b347c46e4..5f714bd281 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -161,7 +161,8 @@ public class StandaloneTestStrategy extends TestStrategy {
                 workingDirectory);
       }
       processLastTestAttempt(attempt, dataBuilder, standaloneTestResult.testResultData());
-      ImmutableList<Pair<String, Path>> testOutputs = action.getTestOutputsMapping(execRoot);
+      ImmutableList<Pair<String, Path>> testOutputs =
+          action.getTestOutputsMapping(actionExecutionContext, execRoot);
       actionExecutionContext
           .getEventHandler()
           .post(
@@ -210,7 +211,7 @@ public class StandaloneTestStrategy extends TestStrategy {
     // Get the normal test output paths, and then update them to use "attempt_N" names, and
     // attemptDir, before adding them to the outputs.
     ImmutableList<Pair<String, Path>> testOutputs =
-        action.getTestOutputsMapping(actionExecutionContext.getExecRoot());
+        action.getTestOutputsMapping(actionExecutionContext, actionExecutionContext.getExecRoot());
     for (Pair<String, Path> testOutput : testOutputs) {
       // e.g. /testRoot/test.dir/file, an example we follow throughout this loop's comments.
       Path testOutputPath = testOutput.getSecond();
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index fcd21ea316..26a24b1e85 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -246,7 +246,7 @@ public abstract class TestStrategy implements TestActionContext {
    */
   protected void postTestResult(ActionExecutionContext actionExecutionContext, TestResult result)
       throws IOException {
-    result.getTestAction().saveCacheStatus(result.getData());
+    result.getTestAction().saveCacheStatus(actionExecutionContext, result.getData());
     actionExecutionContext.getEventBus().post(result);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionFileSystem.java
index 92f51c4ee7..ab02229b2a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionFileSystem.java
@@ -16,6 +16,9 @@ package com.google.devtools.build.lib.skyframe;
 import static java.nio.charset.StandardCharsets.US_ASCII;
 
 import com.google.common.base.Preconditions;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Streams;
@@ -30,6 +33,7 @@ import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.actions.MetadataProvider;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
+import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -73,7 +77,7 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
   private final HashMap<PathFragment, OptionalInputMetadata> optionalInputs;
 
   /** exec path → artifact and metadata */
-  private final ImmutableMap<PathFragment, OutputMetadata> outputs;
+  private final LoadingCache<PathFragment, OutputMetadata> outputs;
 
   /** Used to lookup metadata for optional inputs. */
   private SkyFunction.Environment env = null;
@@ -122,10 +126,10 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
             input.getExecPath(), unused -> new OptionalInputMetadata(input));
       }
 
-      this.outputs =
-          Streams.stream(outputArtifacts)
-              .collect(
-                  ImmutableMap.toImmutableMap(a -> a.getExecPath(), a -> new OutputMetadata(a)));
+      ImmutableMap<PathFragment, Artifact> outputsMapping = Streams.stream(outputArtifacts)
+          .collect(ImmutableMap.toImmutableMap(Artifact::getExecPath, a -> a));
+      this.outputs = CacheBuilder.newBuilder().build(
+          CacheLoader.from(path -> new OutputMetadata(outputsMapping.get(path))));
     } finally {
       Profiler.instance().completeTask(ProfilerTask.ACTION_FS_STAGING);
     }
@@ -162,7 +166,7 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
   @Override
   public void onInsert(ActionInput dest, byte[] digest, long size, int backendIndex)
       throws IOException {
-    OutputMetadata output = outputs.get(dest.getExecPath());
+    OutputMetadata output = outputs.getUnchecked(dest.getExecPath());
     if (output != null) {
       output.set(new RemoteFileArtifactValue(digest, size, backendIndex),
           /*notifyConsumer=*/ false);
@@ -191,6 +195,52 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
     return true;
   }
 
+  @Override
+  protected FileStatus stat(Path path, boolean followSymlinks) throws IOException {
+    FileArtifactValue metadata = getMetadataOrThrowFileNotFound(path);
+    return new FileStatus() {
+      @Override
+      public boolean isFile() {
+        return metadata.getType() == FileStateType.REGULAR_FILE;
+      }
+
+      @Override
+      public boolean isDirectory() {
+        return false;
+      }
+
+      @Override
+      public boolean isSymbolicLink() {
+        return false;
+      }
+
+      @Override
+      public boolean isSpecialFile() {
+        return metadata.getType() == FileStateType.SPECIAL_FILE;
+      }
+
+      @Override
+      public long getSize() {
+        return metadata.getSize();
+      }
+
+      @Override
+      public long getLastModifiedTime() {
+        return metadata.getModifiedTime();
+      }
+
+      @Override
+      public long getLastChangeTime() {
+        return metadata.getModifiedTime();
+      }
+
+      @Override
+      public long getNodeId() {
+        throw new UnsupportedOperationException();
+      }
+    };
+  }
+
   /** ActionFileSystem currently doesn't track directories. */
   @Override
   public boolean createDirectory(Path path) throws IOException {
@@ -207,7 +257,8 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
 
   @Override
   public boolean delete(Path path) throws IOException {
-    throw new UnsupportedOperationException(path.getPathString());
+    // TODO(felly): Support file deletion.
+    return false;
   }
 
   @Override
@@ -230,29 +281,32 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
 
   @Override
   protected boolean isSymbolicLink(Path path) {
-    throw new UnsupportedOperationException(path.getPathString());
+    // TODO(felly): We should have minimal support for symlink awareness when looking at
+    // output --> src and src --> src symlinks.
+    return false;
   }
 
   @Override
   protected boolean isDirectory(Path path, boolean followSymlinks) {
-    Preconditions.checkArgument(
-        followSymlinks, "ActionFileSystem doesn't support no-follow: %s", path);
-    FileArtifactValue metadata = getMetadataUnchecked(path);
-    return metadata == null ? false : metadata.getType() == FileStateType.DIRECTORY;
+    // TODO(felly): Support directory awareness.
+    return true;
+  }
+
+  @Override
+  protected Collection<String> getDirectoryEntries(Path path) throws IOException {
+    // TODO(felly): Support directory traversal.
+    return ImmutableList.of();
   }
 
   @Override
   protected boolean isFile(Path path, boolean followSymlinks) {
-    Preconditions.checkArgument(
-        followSymlinks, "ActionFileSystem doesn't support no-follow: %s", path);
+    // TODO(felly): Unify is* methods with the stat() operation.
     FileArtifactValue metadata = getMetadataUnchecked(path);
     return metadata == null ? false : metadata.getType() == FileStateType.REGULAR_FILE;
   }
 
   @Override
   protected boolean isSpecialFile(Path path, boolean followSymlinks) {
-    Preconditions.checkArgument(
-        followSymlinks, "ActionFileSystem doesn't support no-follow: %s", path);
     FileArtifactValue metadata = getMetadataUnchecked(path);
     return metadata == null ? false : metadata.getType() == FileStateType.SPECIAL_FILE;
   }
@@ -277,7 +331,7 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
           createSymbolicLinkErrorMessage(
               linkPath, targetFragment, targetFragment + " is not an input."));
     }
-    OutputMetadata outputHolder = outputs.get(asExecPath(linkPath));
+    OutputMetadata outputHolder = outputs.getUnchecked(asExecPath(linkPath));
     if (outputHolder == null) {
       throw new FileNotFoundException(
           createSymbolicLinkErrorMessage(
@@ -298,10 +352,6 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
     return getMetadataUnchecked(path) != null;
   }
 
-  @Override
-  protected Collection<String> getDirectoryEntries(Path path) throws IOException {
-    throw new UnsupportedOperationException(path.getPathString());
-  }
 
   @Override
   protected boolean isReadable(Path path) throws IOException {
@@ -341,11 +391,9 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
   }
 
   @Override
-  protected OutputStream getOutputStream(Path path, boolean append) throws IOException {
+  protected OutputStream getOutputStream(Path path, boolean append) {
     Preconditions.checkArgument(!append, "ActionFileSystem doesn't support append.");
-    return Preconditions.checkNotNull(
-            outputs.get(asExecPath(path)), "getOutputStream called for non-output: %s", path)
-        .getOutputStream();
+    return outputs.getUnchecked(asExecPath(path)).getOutputStream();
   }
 
   @Override
@@ -393,7 +441,7 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
       }
     }
     {
-      OutputMetadata metadataHolder = outputs.get(execPath);
+      OutputMetadata metadataHolder = outputs.getIfPresent(execPath);
       if (metadataHolder != null) {
         FileArtifactValue metadata = metadataHolder.get();
         if (metadata != null) {
@@ -423,11 +471,13 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
   }
 
   private boolean isOutput(Path path) {
+    // TODO(felly): This method should instead just refer to potential output paths, which are
+    // anything under the output tree.
     PathFragment fragment = path.asFragment();
     if (!fragment.startsWith(execRootFragment)) {
       return false;
     }
-    return outputs.containsKey(fragment.relativeTo(execRootFragment));
+    return outputs.getIfPresent(fragment.relativeTo(execRootFragment)) != null;
   }
 
   /**
@@ -497,7 +547,7 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
   }
 
   private class OutputMetadata {
-    private final Artifact artifact;
+    private final @Nullable Artifact artifact;
     @Nullable private volatile FileArtifactValue metadata = null;
 
     private OutputMetadata(Artifact artifact) {
@@ -517,7 +567,7 @@ final class ActionFileSystem extends FileSystem implements MetadataProvider, Inj
      * metadataConsumer if it will be notified separately at the Spawn level.
      */
     public void set(FileArtifactValue metadata, boolean notifyConsumer) throws IOException {
-      if (notifyConsumer) {
+      if (notifyConsumer && artifact != null) {
         metadataConsumer.accept(artifact, metadata);
       }
       this.metadata = metadata;