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
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 9d1c291..5c69ce5 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 @@ 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 @@ * 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 @@ } - 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 45c195e..4509092 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.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.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 @@ derivedRoots = ImmutableList.of(); artifactIdRegistry = new ArtifactIdRegistry(); sourceArtifactCache.clear(); - clearDeserializedArtifacts(); } /** @@ -440,59 +436,6 @@ 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 f278d3f..e48a1b1 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 @@ } } - // 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 6d68c47..09698e6 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.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 @@ 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 @@ 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 @@ // 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 @@ } 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 @@ 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 80c6fbe..94f5976 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 @@ 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 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 6558224..45c7c93 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.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 @@ 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 4df20e4..5d4bb91 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 @@ 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 @@ Artifact source = Iterables.getOnlyElement(scannable.getIncludeScannerSources()); scannableMap.put(source, scannable); + sourceFileMap.put(source.getExecPath(), source); } } @@ -339,7 +341,8 @@ 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 5a5e17e..e46d1f7 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 @@ /** * 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 @@ } } 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 f07f7eb..b182c8c 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.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 @@ * * <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 @@ * 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 @@ 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 @@ "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 @@ * * @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 @@ * 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 @@ 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 @@ 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 @@ * * @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 @@ * @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 @@ 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 @@ 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 4b3b2e8..0edb009 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.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 @@ 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 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 07adcce..894a653 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.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.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.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.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 @@ 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 @@ 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 @@ } } } - 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 4be534d..de7ddb2 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.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 @@ 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 f23edea..ec0defd 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.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 @@ 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 @@ 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 @@ } @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 6b702e7..f319e77 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 @@ 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 LoadingResult toLoadingResult() { - return new LoadingResult(hasError(), hasPostExpansionError(), getTargets(), getTestsToRun(), - ImmutableMap.<PackageIdentifier, Path>of()); + return new LoadingResult(hasError(), hasPostExpansionError(), getTargets(), getTestsToRun()); } @SuppressWarnings("unused")