Unify path resolution codepaths.

RELNOTES: None
PiperOrigin-RevId: 199880252

8 files changed, 124 insertions, 78 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
index 1db7c23e43..2aff66330f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
@@ -59,6 +59,8 @@ public class ActionExecutionContext implements Closeable {
 
   @Nullable private ImmutableList<FilesetOutputSymlink> outputSymlinks;
 
+  private final ArtifactPathResolver pathResolver;
+
   private ActionExecutionContext(
       Executor executor,
       MetadataProvider actionInputFileCache,
@@ -82,6 +84,9 @@ public class ActionExecutionContext implements Closeable {
     this.artifactExpander = artifactExpander;
     this.env = env;
     this.actionFileSystem = actionFileSystem;
+    this.pathResolver = createPathResolver(actionFileSystem,
+        // executor is only ever null in testing.
+        executor == null ? null : executor.getExecRoot());
   }
 
   public ActionExecutionContext(
@@ -168,22 +173,25 @@ public class ActionExecutionContext implements Closeable {
    * {@link Artifact.getRoot}.
    */
   public Path getInputPath(ActionInput input) {
-    if (input instanceof Artifact) {
-      Artifact artifact = (Artifact) input;
-      if (actionFileSystem != null) {
-        return actionFileSystem.getPath(artifact.getPath().getPathString());
-      }
-      return artifact.getPath();
-    }
-    return getExecRoot().getRelative(input.getExecPath());
+    return pathResolver.toPath(input);
   }
 
   public Root getRoot(Artifact artifact) {
-    if (actionFileSystem != null) {
-      return Root.fromPath(
-          actionFileSystem.getPath(artifact.getRoot().getRoot().asPath().getPathString()));
+    return pathResolver.transformRoot(artifact.getRoot().getRoot());
+  }
+
+  private static ArtifactPathResolver createPathResolver(FileSystem actionFileSystem,
+      Path execRoot) {
+    if (actionFileSystem == null) {
+      return ArtifactPathResolver.forExecRoot(execRoot);
+    } else {
+      return ArtifactPathResolver.withTransformedFileSystem(
+          actionFileSystem.getPath(execRoot.asFragment()));
     }
-    return artifact.getRoot().getRoot();
+  }
+
+  public ArtifactPathResolver getPathResolver() {
+    return pathResolver;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java
new file mode 100644
index 0000000000..7c28d1e72d
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java
@@ -0,0 +1,88 @@
+// Copyright 2018 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.actions;
+
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Root;
+
+/**
+ * An indirection layer on Path resolution of {@link Artifact} and {@link Root}.
+ */
+public interface ArtifactPathResolver {
+  Path toPath(ActionInput actionInput);
+  Root transformRoot(Root root);
+
+  ArtifactPathResolver IDENTITY = new IdentityResolver(null);
+
+  static ArtifactPathResolver forExecRoot(Path execRoot) {
+    return new IdentityResolver(execRoot);
+  }
+
+  static ArtifactPathResolver withTransformedFileSystem(Path execRoot) {
+    return new TransformResolver(execRoot);
+  }
+
+  /**
+   * Path resolution that uses an Artifact's path directly, or looks up the input execPath relative
+   * to the given execRoot.
+   */
+  class IdentityResolver implements ArtifactPathResolver {
+    private final Path execRoot;
+
+    private IdentityResolver(Path execRoot) {
+      this.execRoot = execRoot;
+    }
+
+    @Override
+    public Path toPath(ActionInput actionInput) {
+      if (actionInput instanceof Artifact) {
+        return ((Artifact) actionInput).getPath();
+      }
+      return execRoot.getRelative(actionInput.getExecPath());
+    }
+
+    @Override
+    public Root transformRoot(Root root) {
+      return Preconditions.checkNotNull(root);
+    }
+  };
+
+  /**
+   * A resolver that transforms all results to the same filesystem as the given execRoot.
+   */
+  class TransformResolver implements ArtifactPathResolver {
+    private final FileSystem fileSystem;
+    private final Path execRoot;
+
+    private TransformResolver(Path execRoot) {
+      this.execRoot = execRoot;
+      this.fileSystem = Preconditions.checkNotNull(execRoot.getFileSystem());
+    }
+
+    @Override
+    public Path toPath(ActionInput input) {
+      if (input instanceof Artifact) {
+        return fileSystem.getPath(((Artifact) input).getPath().getPathString());
+      }
+      return execRoot.getRelative(input.getExecPath());
+    }
+
+    @Override
+    public Root transformRoot(Root root) {
+      return Root.toFileSystem(Preconditions.checkNotNull(root), fileSystem);
+    }
+  }
+}
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 1a386ab847..c6a46e0b4e 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
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.analysis.test;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -167,6 +168,6 @@ public class TestResult {
    */
   private Collection<Pair<String, Path>> getFiles() {
     // TODO(ulfjack): Cache the set of generated files in the TestResultData.
-    return testAction.getTestOutputsMapping(null, execRoot);
+    return testAction.getTestOutputsMapping(ArtifactPathResolver.forExecRoot(execRoot), 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 f03dcaff33..3548b9aafc 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
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.ActionResult;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
@@ -240,14 +241,14 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
    */
   // TODO(ulfjack): Instead of going to local disk here, use SpawnResult (add list of files there).
   public ImmutableList<Pair<String, Path>> getTestOutputsMapping(
-      @Nullable ActionExecutionContext context, Path execRoot) {
+      ArtifactPathResolver resolver, Path execRoot) {
     ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
-    if (convertPath(context, getTestLog()).exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, convertPath(context, getTestLog())));
+    if (resolver.toPath(getTestLog()).exists()) {
+      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog())));
     }
-    if (getCoverageData() != null && convertPath(context, getCoverageData()).exists()) {
+    if (getCoverageData() != null && resolver.toPath(getCoverageData()).exists()) {
       builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE,
-          convertPath(context, getCoverageData())));
+          resolver.toPath(getCoverageData())));
     }
     if (execRoot != null) {
       ResolvedPaths resolvedPaths = resolve(execRoot);
@@ -295,13 +296,6 @@ 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 {
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 5f714bd281..6c034d97b9 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
@@ -162,7 +162,7 @@ public class StandaloneTestStrategy extends TestStrategy {
       }
       processLastTestAttempt(attempt, dataBuilder, standaloneTestResult.testResultData());
       ImmutableList<Pair<String, Path>> testOutputs =
-          action.getTestOutputsMapping(actionExecutionContext, execRoot);
+          action.getTestOutputsMapping(actionExecutionContext.getPathResolver(), execRoot);
       actionExecutionContext
           .getEventHandler()
           .post(
@@ -211,7 +211,8 @@ 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, actionExecutionContext.getExecRoot());
+        action.getTestOutputsMapping(actionExecutionContext.getPathResolver(),
+            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/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 2ffe103975..63ddb692d6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
@@ -46,7 +47,6 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.rules.cpp.IncludeScannable;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
-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.skyframe.SkyFunction;
@@ -550,19 +550,10 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
   private ArtifactPathResolver pathResolver(ActionFileSystem actionFileSystem) {
     if (actionFileSystem != null) {
-      return new ArtifactPathResolver() {
-        @Override
-        public Path toPath(Artifact artifact) {
-          return actionFileSystem.getPath(artifact.getPath().getPathString());
-        }
-
-        @Override
-        public Root transformRoot(Root root) {
-          return Root.toFileSystem(root, actionFileSystem);
-        }
-      };
+      return ArtifactPathResolver.withTransformedFileSystem(
+          actionFileSystem.getPath(skyframeActionExecutor.getExecRoot().asFragment()));
     }
-    return ArtifactPathResolver.IDENTITY;
+    return ArtifactPathResolver.forExecRoot(skyframeActionExecutor.getExecRoot());
   }
 
   private static final Function<Artifact, SkyKey> TO_NONMANDATORY_SKYKEY =
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 af2a14347b..1d4012c60e 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
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileStateValue;
 import com.google.devtools.build.lib.actions.FileValue;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java
deleted file mode 100644
index 9d0bf7651b..0000000000
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright 2018 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.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.Root;
-
-/**
- * An indirection layer on Path resolution of {@link Artifact} and {@link Root}.
- */
-interface ArtifactPathResolver {
-  Path toPath(Artifact artifact);
-  Root transformRoot(Root root);
-
-  ArtifactPathResolver IDENTITY = new ArtifactPathResolver() {
-    @Override
-    public Path toPath(Artifact artifact) {
-      return artifact.getPath();
-    }
-
-    @Override
-    public Root transformRoot(Root root) {
-      return root;
-    }
-  };
-}