diff options
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( |