Remove ArtifactFactory dependency on incorrect exec root

Somewhat trickily, this changes the execRoot field from referring to
[output_base]/execroot/wsname (not really the exec root) to
[output_base]/execroot (actually the execroot).

Progress on #1681.

--
MOS_MIGRATED_REVID=131286181

6 files changed, 50 insertions, 70 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
index 941ea1554c..8ac643b6be 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
@@ -14,10 +14,8 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -27,7 +25,6 @@ import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
@@ -39,7 +36,8 @@ import javax.annotation.Nullable;
 @ThreadSafe
 public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, ArtifactDeserializer {
 
-  private final Path execRoot;
+  private final Path execRootParent;
+  private final PathFragment derivedPathPrefix;
 
   /**
    * Cache of source artifacts.
@@ -52,13 +50,6 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    */
   private ImmutableMap<PackageIdentifier, Root> packageRoots;
 
-  /**
-   * Reverse-ordered list of derived roots for use in looking up or (in rare cases) creating
-   * derived artifacts from execPaths. The reverse order is only significant for overlapping roots
-   * so that the longest is found first.
-   */
-  private ImmutableCollection<Root> derivedRoots = ImmutableList.of();
-
   private ArtifactIdRegistry artifactIdRegistry = new ArtifactIdRegistry();
 
   private static class SourceArtifactCache {
@@ -131,10 +122,12 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    * Constructs a new artifact factory that will use a given execution root when
    * creating artifacts.
    *
-   * @param execRoot the execution root Path to use
+   * @param execRootParent the execution root Path to use. This will be [output_base]/execroot if
+   * deep_execroot is set, [output_base] otherwise.
    */
-  public ArtifactFactory(Path execRoot) {
-    this.execRoot = execRoot;
+  public ArtifactFactory(Path execRootParent, String derivedPathPrefix) {
+    this.execRootParent = execRootParent;
+    this.derivedPathPrefix = new PathFragment(derivedPathPrefix);
   }
 
   /**
@@ -142,7 +135,6 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    */
   public synchronized void clear() {
     packageRoots = null;
-    derivedRoots = ImmutableList.of();
     artifactIdRegistry = new ArtifactIdRegistry();
     sourceArtifactCache.clear();
   }
@@ -159,16 +151,6 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
     sourceArtifactCache.newBuild();
   }
 
-  /**
-   * Set the set of known derived artifact roots. Must be called exactly once
-   * after construction or clear().
-   *
-   * @param roots the set of derived artifact roots to use
-   */
-  public synchronized void setDerivedArtifactRoots(Collection<Root> roots) {
-    derivedRoots = ImmutableSortedSet.<Root>reverseOrder().addAll(roots).build();
-  }
-
   @Override
   public Artifact getSourceArtifact(PathFragment execPath, Root root, ArtifactOwner owner) {
     Preconditions.checkArgument(!execPath.isAbsolute(), "%s %s %s", execPath, root, owner);
@@ -187,7 +169,7 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    * fragment, relative to the exec root, creating it if not found. This method
    * only works for normalized, relative paths.
    */
-  public Artifact getDerivedArtifact(PathFragment execPath) {
+  public Artifact getDerivedArtifact(PathFragment execPath, Path execRoot) {
     Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
     Preconditions.checkArgument(execPath.isNormalized(), execPath);
     // TODO(bazel-team): Check that either BinTools do not change over the life of the Blaze server,
@@ -197,10 +179,13 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
   }
 
   private void validatePath(PathFragment rootRelativePath, Root root) {
+    Preconditions.checkArgument(!root.isSourceRoot());
     Preconditions.checkArgument(!rootRelativePath.isAbsolute(), rootRelativePath);
     Preconditions.checkArgument(rootRelativePath.isNormalized(), rootRelativePath);
-    Preconditions.checkArgument(root.getPath().startsWith(execRoot), "%s %s", root, execRoot);
-    Preconditions.checkArgument(!root.getPath().equals(execRoot), "%s %s", root, execRoot);
+    Preconditions.checkArgument(root.getPath().startsWith(execRootParent), "%s %s", root,
+        execRootParent);
+    Preconditions.checkArgument(!root.getPath().equals(execRootParent), "%s %s", root,
+        execRootParent);
     // TODO(bazel-team): this should only accept roots from derivedRoots.
     //Preconditions.checkArgument(derivedRoots.contains(root), "%s not in %s", root, derivedRoots);
   }
@@ -209,15 +194,15 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    * Returns an artifact for a tool at the given root-relative path under the given root, creating
    * it if not found. This method only works for normalized, relative paths.
    *
-   * <p>The root must be below the execRoot, and the execPath of the resulting Artifact is computed
-   * as {@code root.getRelative(rootRelativePath).relativeTo(execRoot)}.
+   * <p>The root must be below the execRootParent, and the execPath of the resulting Artifact is
+   * computed as {@code root.getRelative(rootRelativePath).relativeTo(root.execRoot)}.
    */
-  // TODO(bazel-team): Don't allow root == execRoot.
+  // TODO(bazel-team): Don't allow root == execRootParent.
   public Artifact getDerivedArtifact(PathFragment rootRelativePath, Root root,
       ArtifactOwner owner) {
     validatePath(rootRelativePath, root);
     Path path = root.getPath().getRelative(rootRelativePath);
-    return getArtifact(path, root, path.relativeTo(execRoot), owner, null);
+    return getArtifact(path, root, path.relativeTo(root.getExecRoot()), owner, null);
   }
 
   /**
@@ -225,28 +210,30 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
    * root-relative path under the given root, creating it if not found. This method only works for
    * normalized, relative paths.
    *
-   * <p>The root must be below the execRoot, and the execPath of the resulting Artifact is computed
-   * as {@code root.getRelative(rootRelativePath).relativeTo(execRoot)}.
+   * <p>The root must be below the execRootParent, and the execPath of the resulting Artifact is
+   * computed as {@code root.getRelative(rootRelativePath).relativeTo(root.execRoot)}.
    */
   public Artifact getFilesetArtifact(PathFragment rootRelativePath, Root root,
       ArtifactOwner owner) {
     validatePath(rootRelativePath, root);
     Path path = root.getPath().getRelative(rootRelativePath);
-    return getArtifact(path, root, path.relativeTo(execRoot), owner, SpecialArtifactType.FILESET);
+    return getArtifact(
+        path, root, path.relativeTo(root.getExecRoot()), owner, SpecialArtifactType.FILESET);
   }
 
   /**
    * Returns an artifact that represents a TreeArtifact; that is, a directory containing some
    * tree of ArtifactFiles unknown at analysis time.
    *
-   * <p>The root must be below the execRoot, and the execPath of the resulting Artifact is computed
-   * as {@code root.getRelative(rootRelativePath).relativeTo(execRoot)}.
+   * <p>The root must be below the execRootParent, and the execPath of the resulting Artifact is
+   * computed as {@code root.getRelative(rootRelativePath).relativeTo(root.execRoot)}.
    */
   public Artifact getTreeArtifact(PathFragment rootRelativePath, Root root,
       ArtifactOwner owner) {
     validatePath(rootRelativePath, root);
     Path path = root.getPath().getRelative(rootRelativePath);
-    return getArtifact(path, root, path.relativeTo(execRoot), owner, SpecialArtifactType.TREE);
+    return getArtifact(
+        path, root, path.relativeTo(root.getExecRoot()), owner, SpecialArtifactType.TREE);
   }
 
   public Artifact getConstantMetadataArtifact(PathFragment rootRelativePath, Root root,
@@ -254,7 +241,8 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
     validatePath(rootRelativePath, root);
     Path path = root.getPath().getRelative(rootRelativePath);
     return getArtifact(
-        path, root, path.relativeTo(execRoot), owner, SpecialArtifactType.CONSTANT_METADATA);
+        path, root, path.relativeTo(root.getExecRoot()), owner,
+        SpecialArtifactType.CONSTANT_METADATA);
   }
 
   /**
@@ -321,7 +309,7 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
       return artifact;
     }
     // Don't create an artifact if it's derived.
-    if (findDerivedRoot(execRoot.getRelative(execPath)) != null) {
+    if (isDerivedArtifact(execPath)) {
       return null;
     }
 
@@ -382,7 +370,7 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
       Artifact a = sourceArtifactCache.getArtifactIfValid(execPathNormalized);
       if (a != null) {
         result.put(execPath, a);
-      } else if (findDerivedRoot(execRoot.getRelative(execPathNormalized)) != null) {
+      } else if (isDerivedArtifact(execPathNormalized)) {
         // Don't create an artifact if it's derived.
         result.put(execPath, null);
       } else {
@@ -418,20 +406,14 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
   }
 
   /**
-   * Finds the derived root for a full path by comparing against the known
-   * derived artifact roots.
+   * Determines if an artifact is derived, that is, its root is a derived root or its exec path
+   * starts with the bazel-out prefix.
    *
-   * @param path a Path to resolve the root for
-   * @return the root for the path or null if no root can be determined
+   * @param execPath The artifact's exec path.
    */
   @VisibleForTesting  // for our own unit tests only.
-  synchronized Root findDerivedRoot(Path path) {
-    for (Root prefix : derivedRoots) {
-      if (path.startsWith(prefix.getPath())) {
-        return prefix;
-      }
-    }
-    return null;
+  synchronized boolean isDerivedArtifact(PathFragment execPath) {
+    return execPath.startsWith(derivedPathPrefix);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Root.java b/src/main/java/com/google/devtools/build/lib/actions/Root.java
index a85e95a66a..2ca8bb65b1 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Root.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Root.java
@@ -130,6 +130,11 @@ public final class Root implements Comparable<Root>, Serializable {
     return getExecPath().getPathString();
   }
 
+  @Nullable
+  public Path getExecRoot() {
+    return execRoot;
+  }
+
   public boolean isSourceRoot() {
     return execRoot == null;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java
index 9e3a377e06..140d3c8488 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BlazeDirectories.java
@@ -59,6 +59,7 @@ public final class BlazeDirectories {
   // in a profiler.
   private final Path outputPath;
   private final Path localOutputPath;
+  private final String productName;
 
   public BlazeDirectories(
       ServerDirectories serverDirectories,
@@ -80,6 +81,7 @@ public final class BlazeDirectories {
     String relativeOutputPath = getRelativeOutputPath(productName);
     this.outputPath = execRoot.getRelative(relativeOutputPath);
     this.localOutputPath = outputBase.getRelative(relativeOutputPath);
+    this.productName = productName;
   }
 
   @VisibleForTesting
@@ -190,6 +192,10 @@ public final class BlazeDirectories {
     return serverDirectories.getInstallMD5();
   }
 
+  public String getRelativeOutputPath() {
+    return BlazeDirectories.getRelativeOutputPath(productName);
+  }
+
   /**
    * Returns the output directory name, relative to the execRoot.
    * TODO(bazel-team): (2011) make this private?
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index fed47ee868..9078f96528 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -483,7 +483,7 @@ public class BuildView {
       skyframeAnalysisResult =
           skyframeBuildView.configureTargets(
               eventHandler, topLevelCtKeys, aspectKeys, eventBus, viewOptions.keepGoing);
-      setArtifactRoots(skyframeAnalysisResult.getPackageRoots(), configurations);
+      setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
     } finally {
       skyframeBuildView.clearInvalidatedConfiguredTargets();
     }
@@ -818,8 +818,7 @@ public class BuildView {
    * paths with unknown roots to artifacts.
    */
   @VisibleForTesting // for BuildViewTestCase
-  public void setArtifactRoots(ImmutableMap<PackageIdentifier, Path> packageRoots,
-      BuildConfigurationCollection configurations) {
+  public void setArtifactRoots(ImmutableMap<PackageIdentifier, Path> packageRoots) {
     Map<Path, Root> rootMap = new HashMap<>();
     Map<PackageIdentifier, Root> realPackageRoots = new HashMap<>();
     for (Map.Entry<PackageIdentifier, Path> entry : packageRoots.entrySet()) {
@@ -832,19 +831,6 @@ public class BuildView {
     }
     // Source Artifact roots:
     getArtifactFactory().setPackageRoots(realPackageRoots);
-
-    // Derived Artifact roots:
-    ImmutableList.Builder<Root> roots = ImmutableList.builder();
-
-    // build-info.txt and friends; this root is not configuration specific.
-    roots.add(directories.getBuildDataDirectory());
-
-    // The roots for each configuration - duplicates are automatically removed in the call below.
-    for (BuildConfiguration cfg : configurations.getAllConfigurations()) {
-      roots.addAll(cfg.getRoots());
-    }
-
-    getArtifactFactory().setDerivedArtifactRoots(roots.build());
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java
index eb8af1801f..6ff3203be1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BinTools.java
@@ -137,7 +137,7 @@ public final class BinTools {
   public Artifact getEmbeddedArtifact(String embedPath, ArtifactFactory artifactFactory) {
     PathFragment path = getExecPath(embedPath);
     Preconditions.checkNotNull(path, embedPath + " not found in embedded tools");
-    return artifactFactory.getDerivedArtifact(path);
+    return artifactFactory.getDerivedArtifact(path, binDir.getParentDirectory());
   }
 
   public ImmutableList<Artifact> getAllEmbeddedArtifacts(ArtifactFactory artifactFactory) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 2d80c522f5..2d95ca3c11 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -129,7 +129,8 @@ public final class SkyframeBuildView {
       SkyframeExecutor skyframeExecutor, BinTools binTools,
       ConfiguredRuleClassProvider ruleClassProvider) {
     this.factory = new ConfiguredTargetFactory(ruleClassProvider);
-    this.artifactFactory = new ArtifactFactory(directories.getExecRoot());
+    this.artifactFactory = new ArtifactFactory(
+        directories.getExecRoot().getParentDirectory(), directories.getRelativeOutputPath());
     this.skyframeExecutor = skyframeExecutor;
     this.binTools = binTools;
     this.ruleClassProvider = ruleClassProvider;