Pass the source path -> Artifact mapping into FdoSupport from a provider instead of special-casing it.

This removes the need to deserialize artifacts in FdoSupport, which in turn removes the need to support artifact deserialization at all, which makes our lives simpler and Thoreauvian programming is good.

--
MOS_MIGRATED_REVID=115660698

17 files changed, 79 insertions, 348 deletions

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 9d1c2910e0..5c69ce5cfd 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
@@ -155,8 +155,7 @@ public class Artifact
   private final Root root;
   private final PathFragment execPath;
   private final PathFragment rootRelativePath;
-  // Non-final only for use when dealing with deserialized artifacts.
-  private ArtifactOwner owner;
+  private final ArtifactOwner owner;
 
   /**
    * Constructs an artifact for the specified path, root and execPath. The root must be an ancestor
@@ -291,27 +290,12 @@ public class Artifact
    * ArtifactOwner#NULL_OWNER} or a dummy owner set in tests. Such a dummy value should only occur
    * for source artifacts if created without specifying the owner, or for special derived artifacts,
    * such as target completion middleman artifacts, build info artifacts, and the like.
-   *
-   * <p>When deserializing artifacts we end up with a dummy owner. In that case,
-   * it must be set using {@link #setArtifactOwner} before this method is called.
    */
   public final ArtifactOwner getArtifactOwner() {
-    Preconditions.checkState(owner != DESERIALIZED_MARKER_OWNER, this);
     return owner;
   }
 
   /**
-   * Sets the artifact owner of this artifact. Should only be called for artifacts that were created
-   * through deserialization, and so their owner was unknown at the time of creation.
-   */
-  public final void setArtifactOwner(ArtifactOwner owner) {
-    if (this.owner == DESERIALIZED_MARKER_OWNER) {
-      // We tolerate multiple calls of this method to accommodate shared actions.
-      this.owner = Preconditions.checkNotNull(owner, this);
-    }
-  }
-
-  /**
    * Returns the root beneath which this Artifact resides, if any. This may be one of the
    * package-path entries (for source Artifacts), or one of the bin, genfiles or includes dirs
    * (for derived Artifacts). It will always be an ancestor of getPath().
@@ -807,12 +791,6 @@ public class Artifact
   }
 
 
-  static final ArtifactOwner DESERIALIZED_MARKER_OWNER = new ArtifactOwner() {
-    @Override
-    public Label getLabel() {
-      return null;
-    }};
-
   @Override
   public boolean isImmutable() {
     return true;
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 45c195edac..4509092cca 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
@@ -18,7 +18,6 @@ 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.common.collect.Lists;
 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;
@@ -33,8 +32,6 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 
 import javax.annotation.Nullable;
 
@@ -150,7 +147,6 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
     derivedRoots = ImmutableList.of();
     artifactIdRegistry = new ArtifactIdRegistry();
     sourceArtifactCache.clear();
-    clearDeserializedArtifacts();
   }
 
   /**
@@ -440,59 +436,6 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
     return null;
   }
 
-  // Non-final only because clear()ing a map does not actually free the memory it took up, so we
-  // assign it to a new map in lieu of clearing.
-  private ConcurrentMap<PathFragment, Artifact> deserializedArtifacts =
-      new ConcurrentHashMap<>();
-
-  /**
-   * Returns the map of all artifacts that were deserialized this build. The caller should process
-   * them and then call {@link #clearDeserializedArtifacts}.
-   */
-  public Map<PathFragment, Artifact> getDeserializedArtifacts() {
-    return deserializedArtifacts;
-  }
-
-  /** Clears the map of deserialized artifacts. */
-  public void clearDeserializedArtifacts() {
-    deserializedArtifacts = new ConcurrentHashMap<>();
-  }
-
-  /**
-   * Resolves an artifact based on its deserialized representation. The artifact can be either a
-   * source or a derived one.
-   *
-   * <p>Note: this method represents a hole in the usual contract that artifacts with a random path
-   * cannot be created. Unfortunately, we currently need this in some cases.
-   *
-   * @param execPath the exec path of the artifact
-   * @throws PackageRootResolutionException on failure to determine the package roots of
-   *    {@code execPath}
-   */
-  public Artifact deserializeArtifact(PathFragment execPath, PackageRootResolver resolver)
-      throws PackageRootResolutionException {
-    Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
-    Path path = execRoot.getRelative(execPath);
-    Root root = findDerivedRoot(path);
-
-    if (root != null) {
-      Artifact result = getDerivedArtifact(path.relativeTo(root.getPath()), root,
-          Artifact.DESERIALIZED_MARKER_OWNER);
-      Artifact oldResult = deserializedArtifacts.putIfAbsent(execPath, result);
-      if (oldResult != null) {
-        result = oldResult;
-      }
-      return result;
-    } else {
-      Map<PathFragment, Root> sourceRoots = resolver.findPackageRootsForFiles(
-          Lists.newArrayList(execPath));
-      if (sourceRoots == null || sourceRoots.get(execPath) == null) {
-        return null;
-      }
-      return getSourceArtifact(execPath, sourceRoots.get(execPath), ArtifactOwner.NULL_OWNER);
-    }
-  }
-
   @Override
   public Artifact lookupArtifactById(int artifactId) {
     return artifactIdRegistry.lookupArtifactById(artifactId);
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 f278d3fc31..e48a1b19c1 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
@@ -486,12 +486,6 @@ public class BuildView {
       }
     }
 
-    // Configuration of some BuildConfiguration.Fragments may require information about
-    // artifactRoots, so we need to set them before calling prepareToBuild. In that case loading
-    // phase has to be enabled.
-    if (loadingEnabled) {
-      setArtifactRoots(loadingResult.getPackageRoots(), configurations);
-    }
     prepareToBuild(configurations, new SkyframePackageRootResolver(skyframeExecutor, eventHandler));
     skyframeExecutor.injectWorkspaceStatusData();
     SkyframeAnalysisResult skyframeAnalysisResult;
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 6d68c47778..09698e6176 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.pkgcache;
 
 import com.google.common.base.Stopwatch;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
@@ -30,13 +29,11 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
-import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TestTargetUtils;
 import com.google.devtools.build.lib.syntax.Type;
 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.Collection;
@@ -229,7 +226,7 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
 
     postLoadingLogging(eventBus, targetsToLoad, expandedResult.getTargets(), timer);
     return new LoadingResult(targets.hasError(), expandedResult.hasError(),
-        expandedResult.getTargets(), testsToRun, ImmutableMap.<PackageIdentifier, Path>of());
+        expandedResult.getTargets(), testsToRun);
   }
 
   /**
@@ -259,7 +256,7 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     postLoadingLogging(eventBus, baseResult.getTargets(), expandedResult.getTargets(), timer);
     LoadingResult loadingResult = new LoadingResult(targets.hasError(),
         !baseResult.isSuccesful() || expandedResult.hasError(),
-        expandedResult.getTargets(), testsToRun, baseResult.roots);
+        expandedResult.getTargets(), testsToRun);
     return loadingResult;
   }
 
@@ -293,7 +290,6 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     // packages/targets have been visited since the last sync/clear.
     boolean loadingSuccessful = pkgLoader.sync(eventHandler, targetsToLoad, labelsToLoad,
           keepGoing, loadingPhaseThreads, Integer.MAX_VALUE);
-    Set<Package> errorFreePackages = pkgLoader.getErrorFreeVisitedPackages(eventHandler);
 
     ImmutableSet<Target> targetsToAnalyze;
     if (loadingSuccessful) {
@@ -307,9 +303,7 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     } else {
       throw new LoadingFailedException("Loading failed; build aborted");
     }
-    return new BaseLoadingResult(
-        targetsToAnalyze, loadingSuccessful,
-        LoadingPhaseRunner.collectPackageRoots(errorFreePackages));
+    return new BaseLoadingResult(targetsToAnalyze, loadingSuccessful);
   }
 
   private void reportAboutPartiallySuccesfulLoading(ImmutableSet<Target> requestedTargets,
@@ -340,13 +334,10 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
   private static class BaseLoadingResult {
     private final ImmutableSet<Target> targets;
     private final boolean succesful;
-    private final ImmutableMap<PackageIdentifier, Path> roots;
 
-    BaseLoadingResult(ImmutableSet<Target> targets, boolean succesful,
-        ImmutableMap<PackageIdentifier, Path> roots) {
+    BaseLoadingResult(ImmutableSet<Target> targets, boolean succesful) {
       this.targets = targets;
       this.succesful = succesful;
-      this.roots = roots;
     }
 
     ImmutableSet<Target> getTargets() {
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
index 80c6fbe4f1..94f597649e 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
@@ -13,11 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.pkgcache;
 
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.vfs.Path;
 
 import java.util.Collection;
 
@@ -30,17 +27,14 @@ public final class LoadingResult {
   private final boolean hasLoadingError;
   private final ImmutableSet<Target> targetsToAnalyze;
   private final ImmutableSet<Target> testsToRun;
-  private final ImmutableMap<PackageIdentifier, Path> packageRoots;
 
   public LoadingResult(boolean hasTargetPatternError, boolean hasLoadingError,
-      Collection<Target> targetsToAnalyze, Collection<Target> testsToRun,
-      ImmutableMap<PackageIdentifier, Path> packageRoots) {
+      Collection<Target> targetsToAnalyze, Collection<Target> testsToRun) {
     this.hasTargetPatternError = hasTargetPatternError;
     this.hasLoadingError = hasLoadingError;
     this.targetsToAnalyze =
         targetsToAnalyze == null ? null : ImmutableSet.copyOf(targetsToAnalyze);
     this.testsToRun = testsToRun == null ? null : ImmutableSet.copyOf(testsToRun);
-    this.packageRoots = packageRoots;
   }
 
   /** Whether there were errors during target pattern evaluation. */
@@ -62,12 +56,4 @@ public final class LoadingResult {
   public Collection<Target> getTestsToRun() {
     return testsToRun;
   }
-
-  /**
-   * The map from package names to the package root where each package was found; this is used to
-   * set up the symlink tree.
-   */
-  public ImmutableMap<PackageIdentifier, Path> getPackageRoots() {
-    return packageRoots;
-  }
 }
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TransitivePackageLoader.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TransitivePackageLoader.java
index 65582247d1..45c7c931b1 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/TransitivePackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TransitivePackageLoader.java
@@ -17,7 +17,6 @@ import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
 
 import java.util.Set;
@@ -55,16 +54,6 @@ public interface TransitivePackageLoader {
   Set<PackageIdentifier> getVisitedPackageNames();
 
   /**
-   * Returns a read-only view of the set of the actual packages visited without error since this
-   * visitor was constructed.
-   *
-   * <p>Use {@link #getVisitedPackageNames()} instead when possible.
-   *
-   * <p>Not thread-safe; do not call during visitation.
-   */
-  Set<Package> getErrorFreeVisitedPackages(EventHandler eventHandler);
-
-  /**
    * Return a mapping between the specified top-level targets and root causes. Note that targets in
    * the input that are transitively error free will not be in the output map. "Top-level" targets
    * are the targetsToVisit and labelsToVisit specified in the last sync.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 4df20e40ac..5d4bb91a40 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -321,6 +321,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
         fake);
 
     Map<Artifact, IncludeScannable> scannableMap = new LinkedHashMap<>();
+    Map<PathFragment, Artifact> sourceFileMap = new LinkedHashMap<>();
     if (cppConfiguration.isLipoContextCollector()) {
       for (IncludeScannable scannable : transitiveLipoInfo.getTransitiveIncludeScannables()) {
         // These should all be CppCompileActions, which should have only one source file.
@@ -328,6 +329,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
         Artifact source =
             Iterables.getOnlyElement(scannable.getIncludeScannerSources());
         scannableMap.put(source, scannable);
+        sourceFileMap.put(source.getExecPath(), source);
       }
     }
 
@@ -339,7 +341,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
                 ruleContext.getLabel(), strippedFile, executable, explicitDwpFile))
         .setRunfilesSupport(runfilesSupport, executable)
         .addProvider(LipoContextProvider.class, new LipoContextProvider(
-            cppCompilationContext, ImmutableMap.copyOf(scannableMap)))
+            cppCompilationContext, ImmutableMap.copyOf(scannableMap),
+            ImmutableMap.copyOf(sourceFileMap)))
         .addProvider(CppLinkAction.Context.class, linkContext)
         .addSkylarkTransitiveInfo(CcSkylarkApiProvider.NAME, new CcSkylarkApiProvider())
         .build();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 5a5e17e05c..e46d1f76d6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -139,7 +139,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   /**
    * This enumeration is used for the --strip option.
    */
-  public static enum StripMode {
+  public enum StripMode {
 
     ALWAYS("always"),       // Always strip.
     SOMETIMES("sometimes"), // Strip iff compilationMode == FASTBUILD.
@@ -1923,10 +1923,10 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       }
     }
     try {
-      getFdoSupport().prepareToBuild(execRoot, artifactFactory, resolver);
+      getFdoSupport().prepareToBuild(execRoot);
     } catch (ZipException e) {
       throw new ViewCreationFailedException("Error reading provided FDO zip file", e);
-    } catch (FdoException | IOException | PackageRootResolutionException e) {
+    } catch (FdoException | IOException e) {
       throw new ViewCreationFailedException("Error while initializing FDO support", e);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
index f07f7eb38f..b182c8cf04 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
@@ -21,9 +21,6 @@ import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactFactory;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -204,7 +201,7 @@ public class FdoSupport {
    *
    * <p>Set only in {@link #prepareToBuild}.
    */
-  private ImmutableMultimap<PathFragment, Artifact> imports;
+  private ImmutableMultimap<PathFragment, PathFragment> imports;
 
   /**
    * Creates an FDO support object.
@@ -266,12 +263,10 @@ public class FdoSupport {
    * FDO gcda zip file into a clean working directory under execRoot.
    *
    * @throws FdoException if the FDO ZIP contains a file of unknown type
-   * @throws PackageRootResolutionException
    */
   @ThreadHostile // must be called before starting the build
-  public void prepareToBuild(Path execRoot, ArtifactFactory artifactDeserializer,
-      PackageRootResolver resolver)
-          throws IOException, FdoException, PackageRootResolutionException {
+  public void prepareToBuild(Path execRoot)
+      throws IOException, FdoException {
     // The execRoot != null case is only there for testing. We cannot provide a real ZIP file in
     // tests because ZipFileSystem does not work with a ZIP on an in-memory file system.
     // IMPORTANT: Keep in sync with #declareSkyframeDependencies to avoid incrementality issues.
@@ -285,7 +280,7 @@ public class FdoSupport {
         Path fdoImports = fdoProfile.getParentDirectory().getRelative(
             fdoProfile.getBaseName() + ".imports");
         if (isLipoEnabled()) {
-          imports = readAutoFdoImports(artifactDeserializer, fdoImports, resolver);
+          imports = readAutoFdoImports(fdoImports);
         }
         FileSystemUtils.ensureSymbolicLink(
             execRoot.getRelative(getAutoProfilePath()), fdoProfile);
@@ -299,10 +294,9 @@ public class FdoSupport {
                                  "for the compiler to find the profile");
         }
         ImmutableSet.Builder<PathFragment> gcdaFilesBuilder = ImmutableSet.builder();
-        ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder =
+        ImmutableMultimap.Builder<PathFragment, PathFragment> importsBuilder =
             ImmutableMultimap.builder();
-        extractFdoZip(artifactDeserializer, zipFilePath, fdoDirPath,
-            gcdaFilesBuilder, importsBuilder, resolver);
+        extractFdoZip(zipFilePath, fdoDirPath, gcdaFilesBuilder, importsBuilder);
         gcdaFiles = gcdaFilesBuilder.build();
         imports = importsBuilder.build();
       }
@@ -325,29 +319,25 @@ public class FdoSupport {
    *
    * @throws IOException if any of the I/O operations failed
    * @throws FdoException if the FDO ZIP contains a file of unknown type
-   * @throws PackageRootResolutionException on exception resolving package roots for imports
    */
-  private void extractFdoZip(ArtifactFactory artifactFactory, Path sourceDir,
-      Path targetDir, ImmutableSet.Builder<PathFragment> gcdaFilesBuilder,
-      ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder,
-      PackageRootResolver resolver)
-          throws IOException, FdoException, PackageRootResolutionException {
+  private void extractFdoZip(Path sourceDir, Path targetDir,
+      ImmutableSet.Builder<PathFragment> gcdaFilesBuilder,
+      ImmutableMultimap.Builder<PathFragment, PathFragment> importsBuilder)
+          throws IOException, FdoException {
     for (Path sourceFile : sourceDir.getDirectoryEntries()) {
       Path targetFile = targetDir.getRelative(sourceFile.getBaseName());
       if (sourceFile.isDirectory()) {
         targetFile.createDirectory();
-        extractFdoZip(artifactFactory, sourceFile, targetFile, gcdaFilesBuilder, importsBuilder,
-            resolver);
+        extractFdoZip(sourceFile, targetFile, gcdaFilesBuilder, importsBuilder);
       } else {
         if (CppFileTypes.COVERAGE_DATA.matches(sourceFile)) {
           FileSystemUtils.copyFile(sourceFile, targetFile);
           gcdaFilesBuilder.add(
               sourceFile.relativeTo(sourceFile.getFileSystem().getRootDirectory()));
         } else if (CppFileTypes.COVERAGE_DATA_IMPORTS.matches(sourceFile)) {
-          readCoverageImports(artifactFactory, sourceFile, importsBuilder, resolver);
+          readCoverageImports(sourceFile, importsBuilder);
         } else {
-            throw new FdoException("FDO ZIP file contained a file of unknown type: "
-                + sourceFile);
+            throw new FdoException("FDO ZIP file contained a file of unknown type: " + sourceFile);
         }
       }
     }
@@ -357,12 +347,10 @@ public class FdoSupport {
    * Reads a .gcda.imports file and stores the imports information.
    *
    * @throws FdoException if an auxiliary LIPO input was not found
-   * @throws PackageRootResolutionException on exception resolving package roots for imports
    */
-  private void readCoverageImports(ArtifactFactory artifactFactory, Path importsFile,
-      ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder,
-      PackageRootResolver resolver)
-          throws IOException, FdoException, PackageRootResolutionException {
+  private void readCoverageImports(Path importsFile,
+      ImmutableMultimap.Builder<PathFragment, PathFragment> importsBuilder)
+          throws IOException, FdoException {
     PathFragment key = importsFile.asFragment().relativeTo(ZIP_ROOT);
     String baseName = key.getBaseName();
     String ext = Iterables.getOnlyElement(CppFileTypes.COVERAGE_DATA_IMPORTS.getExtensions());
@@ -377,24 +365,18 @@ public class FdoSupport {
           throw new FdoException("Absolute paths not allowed in gcda imports file " + importsFile
               + ": " + execPath);
         }
-        Artifact artifact = artifactFactory.deserializeArtifact(new PathFragment(line), resolver);
-        if (artifact == null) {
-          throw new FdoException("Auxiliary LIPO input not found: " + line);
-        }
-
-        importsBuilder.put(key, artifact);
+        importsBuilder.put(key, new PathFragment(line));
       }
     }
   }
 
   /**
    * Reads a .afdo.imports file and stores the imports information.
-   * @throws PackageRootResolutionException
    */
-  private ImmutableMultimap<PathFragment, Artifact> readAutoFdoImports(
-      ArtifactFactory artifactFactory, Path importsFile, PackageRootResolver resolver)
-          throws IOException, FdoException, PackageRootResolutionException {
-    ImmutableMultimap.Builder<PathFragment, Artifact> importBuilder = ImmutableMultimap.builder();
+  private ImmutableMultimap<PathFragment, PathFragment> readAutoFdoImports(Path importsFile)
+          throws IOException, FdoException {
+    ImmutableMultimap.Builder<PathFragment, PathFragment> importBuilder =
+        ImmutableMultimap.builder();
     for (String line : FileSystemUtils.iterateLinesAsLatin1(importsFile)) {
       if (!line.isEmpty()) {
         PathFragment key = new PathFragment(line.substring(0, line.indexOf(':')));
@@ -406,12 +388,8 @@ public class FdoSupport {
           if (auxFile.length() == 0) {
             continue;
           }
-          Artifact artifact = artifactFactory.deserializeArtifact(new PathFragment(auxFile),
-              resolver);
-          if (artifact == null) {
-            throw new FdoException("Auxiliary LIPO input not found: " + auxFile);
-          }
-          importBuilder.put(key, artifact);
+
+          importBuilder.put(key, new PathFragment(auxFile));
         }
       }
     }
@@ -423,12 +401,23 @@ public class FdoSupport {
    *
    * @param sourceExecPath the source file
    */
-  private Collection<Artifact> getAutoFdoImports(PathFragment sourceExecPath) {
+  private Collection<Artifact> getAutoFdoImports(RuleContext ruleContext,
+      PathFragment sourceExecPath, LipoContextProvider lipoContextProvider) {
     Preconditions.checkState(isLipoEnabled());
-    ImmutableCollection<Artifact> afdoImports = imports.get(sourceExecPath);
+    ImmutableCollection<PathFragment> afdoImports = imports.get(sourceExecPath);
     Preconditions.checkState(afdoImports != null,
         "AutoFDO import data missing for %s", sourceExecPath);
-    return afdoImports;
+    ImmutableList.Builder<Artifact> result = ImmutableList.builder();
+    for (PathFragment afdoImport : afdoImports) {
+      Artifact afdoArtifact = lipoContextProvider.getSourceArtifactMap().get(afdoImport);
+      if (afdoArtifact != null) {
+        result.add(afdoArtifact);
+      } else {
+        ruleContext.ruleError(String.format(
+            "cannot find source file '%s' referenced from '%s'", afdoImport, sourceExecPath));
+      }
+    }
+    return result.build();
   }
 
   /**
@@ -437,12 +426,12 @@ public class FdoSupport {
    * @param objDirectory the object directory of the object file's target
    * @param objectName the object file
    */
-  private Iterable<Artifact> getImports(PathFragment objDirectory, PathFragment objectName) {
+  private Iterable<PathFragment> getImports(PathFragment objDirectory, PathFragment objectName) {
     Preconditions.checkState(isLipoEnabled());
     Preconditions.checkState(imports != null,
         "Tried to look up imports of uninitialized FDOSupport");
     PathFragment key = objDirectory.getRelative(FileSystemUtils.removeExtension(objectName));
-    ImmutableCollection<Artifact> importsForObject = imports.get(key);
+    ImmutableCollection<PathFragment> importsForObject = imports.get(key);
     Preconditions.checkState(importsForObject != null, "Import data missing for %s", key);
     return importsForObject;
   }
@@ -520,7 +509,7 @@ public class FdoSupport {
       env.registerAction(new FdoStubAction(ruleContext.getActionOwner(), artifact));
       auxiliaryInputs.add(artifact);
       if (lipoContextProvider != null) {
-        auxiliaryInputs.addAll(getAutoFdoImports(sourceExecPath));
+        auxiliaryInputs.addAll(getAutoFdoImports(ruleContext, sourceExecPath, lipoContextProvider));
       }
       return auxiliaryInputs.build();
     } else {
@@ -534,20 +523,26 @@ public class FdoSupport {
           getGcdaArtifactsForObjectFileName(ruleContext, env, objectName, lipoLabel));
 
       if (lipoContextProvider != null) {
-        for (Artifact importedFile : getImports(
+        for (PathFragment importedFile : getImports(
             getNonLipoObjDir(ruleContext, lipoLabel), objectName)) {
-          if (CppFileTypes.COVERAGE_DATA.matches(importedFile.getFilename())) {
-            Artifact gcdaArtifact = getGcdaArtifactsForGcdaPath(
-                ruleContext, env, importedFile.getExecPath());
+          if (CppFileTypes.COVERAGE_DATA.matches(importedFile.getBaseName())) {
+            Artifact gcdaArtifact = getGcdaArtifactsForGcdaPath(ruleContext, env, importedFile);
             if (gcdaArtifact == null) {
               ruleContext.ruleError(String.format(
                   ".gcda file %s is not in the FDO zip (referenced by source file %s)",
-                  importedFile.getExecPath(), sourceName));
+                  importedFile, sourceName));
             } else {
               auxiliaryInputs.add(gcdaArtifact);
             }
           } else {
-            auxiliaryInputs.add(importedFile);
+            Artifact importedArtifact = lipoContextProvider.getSourceArtifactMap()
+                .get(importedFile);
+            if (importedArtifact != null) {
+              auxiliaryInputs.add(importedArtifact);
+            } else {
+              ruleContext.ruleError(String.format(
+                  "cannot find source file '%s' referenced from '%s'", importedFile, objectName));
+            }
           }
         }
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LipoContextProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LipoContextProvider.java
index 4b3b2e89d1..0edb0094ea 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LipoContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LipoContextProvider.java
@@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.util.Map;
 
@@ -35,10 +36,14 @@ public final class LipoContextProvider implements TransitiveInfoProvider {
   private final CppCompilationContext cppCompilationContext;
 
   private final ImmutableMap<Artifact, IncludeScannable> includeScannables;
+  private final ImmutableMap<PathFragment, Artifact> sourceArtifactMap;
+
   public LipoContextProvider(CppCompilationContext cppCompilationContext,
-      Map<Artifact, IncludeScannable> scannables) {
+      Map<Artifact, IncludeScannable> scannables,
+      Map<PathFragment, Artifact> sourceArtifactMap) {
     this.cppCompilationContext = cppCompilationContext;
     this.includeScannables = ImmutableMap.copyOf(scannables);
+    this.sourceArtifactMap = ImmutableMap.copyOf(sourceArtifactMap);
   }
 
   /**
@@ -55,4 +60,8 @@ public final class LipoContextProvider implements TransitiveInfoProvider {
   public ImmutableMap<Artifact, IncludeScannable> getIncludeScannables() {
     return includeScannables;
   }
+
+  public ImmutableMap<PathFragment, Artifact> getSourceArtifactMap() {
+    return sourceArtifactMap;
+  }
 }
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 07adcce1e3..894a653acc 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
@@ -25,7 +25,6 @@ import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.Action;
-import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
@@ -59,7 +58,6 @@ import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
-import com.google.devtools.build.lib.skyframe.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
 import com.google.devtools.build.lib.skyframe.BuildInfoCollectionValue.BuildInfoKeyAndConfig;
@@ -68,7 +66,6 @@ import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ConflictExc
 import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
 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 com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.ErrorInfo;
 import com.google.devtools.build.skyframe.EvaluationProgressReceiver;
@@ -78,7 +75,6 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -198,43 +194,6 @@ public final class SkyframeBuildView {
     skyframeExecutor.clearAnalysisCache(topLevelTargets);
   }
 
-  private void setDeserializedArtifactOwners() throws ViewCreationFailedException {
-    Map<PathFragment, Artifact> deserializedArtifactMap =
-        artifactFactory.getDeserializedArtifacts();
-    Set<Artifact> deserializedArtifacts = new HashSet<>();
-    for (Artifact artifact : deserializedArtifactMap.values()) {
-      if (!artifact.getExecPath().getBaseName().endsWith(".gcda")) {
-        // gcda files are classified as generated artifacts, but are not actually generated. All
-        // others need owners.
-        deserializedArtifacts.add(artifact);
-      }
-    }
-    if (deserializedArtifacts.isEmpty()) {
-      // If there are no deserialized artifacts to process, don't pay the price of iterating over
-      // the graph.
-      return;
-    }
-    for (Map.Entry<SkyKey, ActionLookupValue> entry :
-      skyframeExecutor.getActionLookupValueMap().entrySet()) {
-      for (Action action : entry.getValue().getActionsForFindingArtifactOwners()) {
-        for (Artifact output : action.getOutputs()) {
-          Artifact deserializedArtifact = deserializedArtifactMap.get(output.getExecPath());
-          if (deserializedArtifact != null) {
-            deserializedArtifact.setArtifactOwner((ActionLookupKey) entry.getKey().argument());
-            deserializedArtifacts.remove(deserializedArtifact);
-          }
-        }
-      }
-    }
-    if (!deserializedArtifacts.isEmpty()) {
-      throw new ViewCreationFailedException("These artifacts were read in from the FDO profile but"
-      + " have no generating action that could be found. If you are confident that your profile was"
-      + " collected from the same source state at which you're building, please report this:\n"
-      + Artifact.asExecPaths(deserializedArtifacts));
-    }
-    artifactFactory.clearDeserializedArtifacts();
-  }
-
   /**
    * Analyzes the specified targets using Skyframe as the driving framework.
    *
@@ -285,7 +244,6 @@ public final class SkyframeBuildView {
         LoadingPhaseRunner.collectPackageRoots(packages.build().toCollection());
 
     if (!result.hasError() && badActions.isEmpty()) {
-      setDeserializedArtifactOwners();
       return new SkyframeAnalysisResult(
           /*hasLoadingError=*/false, /*hasAnalysisError=*/false,
           ImmutableList.copyOf(goodCts),
@@ -412,7 +370,7 @@ public final class SkyframeBuildView {
         }
       }
     }
-    setDeserializedArtifactOwners();
+
     return new SkyframeAnalysisResult(
         hasLoadingError,
         result.hasError() || !badActions.isEmpty(),
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 4be534db58..de7ddb2a61 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
@@ -32,7 +32,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Range;
-import com.google.common.collect.Sets;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionCacheChecker;
@@ -1411,40 +1410,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
       return buildDriver.evaluate(valueNames, keepGoing, parallelThreads, eventHandler);
     }
-
-    public Set<Package> retrievePackages(
-        final EventHandler eventHandler, Set<PackageIdentifier> packageIds) {
-      final List<SkyKey> valueNames = new ArrayList<>();
-      for (PackageIdentifier pkgId : packageIds) {
-        valueNames.add(PackageValue.key(pkgId));
-      }
-
-      try {
-        return callUninterruptibly(
-            new Callable<Set<Package>>() {
-              @Override
-              public Set<Package> call() throws Exception {
-                EvaluationResult<PackageValue> result =
-                    buildDriver.evaluate(
-                        valueNames,
-                        false,
-                        ResourceUsage.getAvailableProcessors(),
-                        eventHandler);
-                Preconditions.checkState(
-                    !result.hasError(), "unexpected errors: %s", result.errorMap());
-                Set<Package> packages = Sets.newHashSet();
-                for (PackageValue value : result.values()) {
-                  Package pkg = value.getPackage();
-                  Preconditions.checkState(!pkg.containsErrors(), pkg.getName());
-                  packages.add(pkg);
-                }
-                return packages;
-              }
-            });
-      } catch (Exception e) {
-        throw new IllegalStateException(e);
-      }
-    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
index f23edea999..ec0defdf55 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor.SkyframeTransitivePackageLoader;
@@ -51,7 +50,6 @@ final class SkyframeLabelVisitor implements TransitivePackageLoader {
   private final AtomicReference<CyclesReporter> skyframeCyclesReporter;
 
   private Set<PackageIdentifier> allVisitedPackages;
-  private Set<PackageIdentifier> errorFreeVisitedPackages;
   private Set<TransitiveTargetValue> previousBuildTargetValueSet = null;
   private boolean lastBuildKeepGoing;
   private final Multimap<Label, Label> rootCauses = HashMultimap.create();
@@ -212,14 +210,11 @@ final class SkyframeLabelVisitor implements TransitivePackageLoader {
       return;
     }
     NestedSetBuilder<PackageIdentifier> nestedAllPkgsBuilder = NestedSetBuilder.stableOrder();
-    NestedSetBuilder<PackageIdentifier> nestedErrorFreePkgsBuilder = NestedSetBuilder.stableOrder();
     for (TransitiveTargetValue value : targetValues) {
       nestedAllPkgsBuilder.addTransitive(value.getTransitiveSuccessfulPackages());
       nestedAllPkgsBuilder.addTransitive(value.getTransitiveUnsuccessfulPackages());
-      nestedErrorFreePkgsBuilder.addTransitive(value.getTransitiveSuccessfulPackages());
     }
     allVisitedPackages = nestedAllPkgsBuilder.build().toSet();
-    errorFreeVisitedPackages = nestedErrorFreePkgsBuilder.build().toSet();
     previousBuildTargetValueSet = currentBuildTargetValueSet;
   }
 
@@ -229,11 +224,6 @@ final class SkyframeLabelVisitor implements TransitivePackageLoader {
   }
 
   @Override
-  public Set<Package> getErrorFreeVisitedPackages(EventHandler eventHandler) {
-    return transitivePackageLoader.retrievePackages(eventHandler, errorFreeVisitedPackages);
-  }
-
-  @Override
   public Multimap<Label, Label> getRootCauses() {
     Preconditions.checkState(lastBuildKeepGoing);
     return rootCauses;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
index 6b702e7e60..f319e77fb2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
@@ -15,16 +15,13 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.util.Preconditions;
-import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
@@ -105,8 +102,7 @@ public final class TargetPatternPhaseValue implements SkyValue {
   }
 
   public LoadingResult toLoadingResult() {
-    return new LoadingResult(hasError(), hasPostExpansionError(), getTargets(), getTestsToRun(),
-        ImmutableMap.<PackageIdentifier, Path>of());
+    return new LoadingResult(hasError(), hasPostExpansionError(), getTargets(), getTestsToRun());
   }
 
   @SuppressWarnings("unused")
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
index 1979df4e2f..2e491b17e0 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
@@ -251,37 +251,4 @@ public class ArtifactFactoryTest {
       return result;
     }
   }
-
-  @Test
-  public void testArtifactDeserializationWithoutReusedArtifacts() throws Exception {
-    PathFragment derivedPath = outRoot.getExecPath().getRelative("fruit/banana");
-    artifactFactory.clear();
-    artifactFactory.setDerivedArtifactRoots(ImmutableList.of(outRoot));
-    MockPackageRootResolver rootResolver = new MockPackageRootResolver();
-    rootResolver.setPackageRoots(
-        ImmutableMap.of(PackageIdentifier.createInDefaultRepo(""), clientRoot));
-    Artifact artifact1 = artifactFactory.deserializeArtifact(derivedPath, rootResolver);
-    Artifact artifact2 = artifactFactory.deserializeArtifact(derivedPath, rootResolver);
-    assertEquals(artifact1, artifact2);
-    assertNull(artifact1.getOwner());
-    assertNull(artifact2.getOwner());
-    assertEquals(derivedPath, artifact1.getExecPath());
-    assertEquals(derivedPath, artifact2.getExecPath());
-
-    // Source artifacts are always reused
-    PathFragment sourcePath = clientRoot.getExecPath().getRelative("fruit/mango");
-    artifact1 = artifactFactory.deserializeArtifact(sourcePath, rootResolver);
-    artifact2 = artifactFactory.deserializeArtifact(sourcePath, rootResolver);
-    assertSame(artifact1, artifact2);
-    assertEquals(sourcePath, artifact1.getExecPath());
-  }
-
-  @Test
-  public void testDeserializationWithInvalidPath() throws Exception {
-    artifactFactory.clear();
-    PathFragment randomPath = new PathFragment("maracuja/lemon/kiwi");
-    Artifact artifact = artifactFactory.deserializeArtifact(randomPath,
-        new MockPackageRootResolver());
-    assertNull(artifact);
-  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index 77b9ad81ec..5e9dcd41f8 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -26,7 +26,6 @@ import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.eventbus.EventBus;
@@ -116,12 +115,6 @@ public class LoadingPhaseRunnerTest {
     LoadingResult loadingResult = assertNoErrors(tester.load("//base:hello"));
     assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//base:hello"));
     assertNull(loadingResult.getTestsToRun());
-    // TODO(ulfjack): We don't collect package roots if we don't run the loading phase.
-    if (runsLoadingPhase()) {
-      assertEquals(
-          ImmutableMap.of(PackageIdentifier.createInDefaultRepo("base"), tester.getWorkspace()),
-          loadingResult.getPackageRoots());
-    }
   }
 
   @Test
@@ -150,7 +143,6 @@ public class LoadingPhaseRunnerTest {
     assertThat(loadingResult.hasLoadingError()).isFalse();
     assertThat(loadingResult.getTargets()).isEmpty();
     assertThat(loadingResult.getTestsToRun()).isNull();
-    assertThat(loadingResult.getPackageRoots()).isEmpty();
     tester.assertContainsError("Skipping '//base:missing': no such package 'base'");
     tester.assertContainsWarning("Target pattern parsing failed.");
   }
@@ -163,7 +155,6 @@ public class LoadingPhaseRunnerTest {
     assertThat(loadingResult.hasLoadingError()).isFalse();
     assertThat(loadingResult.getTargets()).isEmpty();
     assertThat(loadingResult.getTestsToRun()).isNull();
-    assertThat(loadingResult.getPackageRoots()).isEmpty();
     tester.assertContainsError("Skipping '//base:missing': no such target '//base:missing'");
     tester.assertContainsWarning("Target pattern parsing failed.");
   }
@@ -193,7 +184,6 @@ public class LoadingPhaseRunnerTest {
     assertTrue(loadingResult.hasLoadingError());
     assertThat(loadingResult.getTargets()).containsExactlyElementsIn(ImmutableList.<Target>of());
     assertNull(loadingResult.getTestsToRun());
-    assertTrue(loadingResult.getPackageRoots().size() <= 1);
     tester.assertContainsError("no such package 'nonexistent': BUILD file not found");
   }
 
@@ -321,10 +311,6 @@ public class LoadingPhaseRunnerTest {
     LoadingResult loadingResult = assertNoErrors(tester.loadTests("//cc:tests"));
     assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//cc:my_test"));
     assertThat(loadingResult.getTestsToRun()).containsExactlyElementsIn(getTargets("//cc:my_test"));
-    if (runsLoadingPhase()) {
-      assertThat(loadingResult.getPackageRoots().entrySet())
-          .contains(entryFor(PackageIdentifier.createInDefaultRepo("cc"), tester.getWorkspace()));
-    }
     assertThat(tester.getOriginalTargets())
         .containsExactlyElementsIn(getTargets("//cc:tests", "//cc:my_test"));
     assertThat(tester.getTestSuiteTargets())
@@ -450,7 +436,6 @@ public class LoadingPhaseRunnerTest {
     LoadingResult secondResult = assertNoErrors(tester.load("//base:hello"));
     assertEquals(firstResult.getTargets(), secondResult.getTargets());
     assertEquals(firstResult.getTestsToRun(), secondResult.getTestsToRun());
-    assertEquals(firstResult.getPackageRoots(), secondResult.getPackageRoots());
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java
index 5771f522d9..13951591c1 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitorTestCase.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.events.EventCollector;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
-import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
@@ -121,8 +120,7 @@ abstract public class SkyframeLabelVisitorTestCase extends PackageLoadingTestCas
    * Check that the expected targets were exactly those visited, and that the packages of these
    * expected targets were exactly those packages visited.
    */
-  protected void assertExpectedTargets(
-      Set<String> expectedLabels, boolean expectError, Set<Target> startingTargets)
+  protected void assertExpectedTargets(Set<String> expectedLabels, Set<Target> startingTargets)
       throws Exception {
     Set<Label> visitedLabels =
         getVisitedLabels(
@@ -143,13 +141,6 @@ abstract public class SkyframeLabelVisitorTestCase extends PackageLoadingTestCas
     }
 
     assertEquals(expectedPkgs, getVisitedPackageNames(startingTargets));
-    if (!expectError) {
-      Set<PathFragment> visitedPkgs = new HashSet<>();
-      for (Package pkg : getErrorFreeVisitedPackages(startingTargets)) {
-        visitedPkgs.add(pkg.getNameFragment());
-      }
-      assertEquals(expectedPkgs, visitedPkgs);
-    }
   }
 
   /**
@@ -176,7 +167,7 @@ abstract public class SkyframeLabelVisitorTestCase extends PackageLoadingTestCas
             reporter, startingTargets, ImmutableSet.<Label>of(), keepGoing, 200, Integer.MAX_VALUE);
 
     assertNotSame(expectError, result);
-    assertExpectedTargets(expectedLabels, expectError, startingTargets);
+    assertExpectedTargets(expectedLabels, startingTargets);
   }
 
   /**
@@ -286,15 +277,6 @@ abstract public class SkyframeLabelVisitorTestCase extends PackageLoadingTestCas
     return builder.build();
   }
 
-  protected Set<Package> getErrorFreeVisitedPackages(Set<Target> startingTargets) {
-    ImmutableSet.Builder<Package> builder = ImmutableSet.builder();
-    builder.addAll(visitor.getErrorFreeVisitedPackages(reporter));
-    for (Target target : startingTargets) {
-      builder.add(target.getPackage());
-    }
-    return builder.build();
-  }
-
   @Before
   public final void initializeVisitor() throws Exception {
     skyframeExecutor = super.createSkyframeExecutor(