diff options
Diffstat (limited to 'src/main/java')
13 files changed, 147 insertions, 217 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 4a3e3561ce..d9e324b357 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.bazel; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; @@ -60,13 +59,11 @@ import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.util.Clock; -import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.common.options.OptionsProvider; import java.util.Map.Entry; -import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; @@ -75,7 +72,6 @@ import java.util.concurrent.atomic.AtomicBoolean; */ public class BazelRepositoryModule extends BlazeModule { - private BlazeDirectories directories; // A map of repository handlers that can be looked up by rule class name. private final ImmutableMap<String, RepositoryFunction> repositoryHandlers; private final AtomicBoolean isFetch = new AtomicBoolean(false); @@ -109,18 +105,12 @@ public class BazelRepositoryModule extends BlazeModule { public void blazeStartup(OptionsProvider startupOptions, BlazeVersionInfo versionInfo, UUID instanceId, BlazeDirectories directories, Clock clock) { - this.directories = directories; for (RepositoryFunction handler : repositoryHandlers.values()) { handler.setDirectories(directories); } } @Override - public Set<Path> getImmutableDirectories() { - return ImmutableSet.of(RepositoryFunction.getExternalRepositoryDirectory(directories)); - } - - @Override public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) { for (Entry<String, RepositoryFunction> handler : repositoryHandlers.entrySet()) { // TODO(bazel-team): Migrate away from Class<?> diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java index e6bff64ef5..f14b954978 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.pkgcache; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Objects; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -33,6 +32,7 @@ import java.io.Serializable; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; @@ -93,23 +93,23 @@ public class PathPackageLocator implements Serializable { /** * Like #getPackageBuildFile(), but returns null instead of throwing. - * - * @param packageName the name of the package. + * @param packageIdentifier the name of the package. * @param cache a filesystem-level cache of stat() calls. */ - public Path getPackageBuildFileNullable(PackageIdentifier packageName, + public Path getPackageBuildFileNullable(PackageIdentifier packageIdentifier, AtomicReference<? extends UnixGlob.FilesystemCalls> cache) { - if (packageName.getRepository().isDefault()) { - return getFilePath(packageName.getPackageFragment().getRelative("BUILD"), cache); - } else if (!packageName.getRepository().isDefault()) { + if (packageIdentifier.getRepository().isDefault()) { + return getFilePath(packageIdentifier.getPackageFragment().getRelative("BUILD"), cache); + } else if (!packageIdentifier.getRepository().isDefault()) { Verify.verify(outputBase != null, String.format( "External package '%s' needs to be loaded but this PathPackageLocator instance does not " - + "support external packages", packageName)); + + "support external packages", packageIdentifier)); // This works only to some degree, because it relies on the presence of the repository under // $OUTPUT_BASE/external, which is created by the appropriate RepositoryValue. This is true // for the invocation in GlobCache, but not for the locator.getBuildFileForPackage() // invocation in Parser#include(). - Path buildFile = outputBase.getRelative(packageName.getPathFragment()).getRelative("BUILD"); + Path buildFile = outputBase.getRelative( + packageIdentifier.getPathFragment()).getRelative("BUILD"); FileStatus stat = cache.get().statNullable(buildFile, Symlinks.FOLLOW); if (stat != null && stat.isFile()) { return buildFile; @@ -182,6 +182,7 @@ public class PathPackageLocator implements Serializable { resolvedPaths.add(rootPath); } } + return new PathPackageLocator(outputBase, resolvedPaths); } @@ -235,7 +236,7 @@ public class PathPackageLocator implements Serializable { @Override public int hashCode() { - return Objects.hashCode(pathEntries, outputBase); + return Objects.hash(pathEntries, outputBase); } @Override @@ -246,7 +247,12 @@ public class PathPackageLocator implements Serializable { if (!(other instanceof PathPackageLocator)) { return false; } - return this.getPathEntries().equals(((PathPackageLocator) other).getPathEntries()) - && Objects.equal(this.outputBase, ((PathPackageLocator) other).outputBase); + PathPackageLocator pathPackageLocator = (PathPackageLocator) other; + return Objects.equals(getPathEntries(), pathPackageLocator.getPathEntries()) + && Objects.equals(outputBase, pathPackageLocator.outputBase); + } + + public Path getOutputBase() { + return outputBase; } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java index 9f661bec71..2c23469f1a 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java @@ -17,7 +17,6 @@ import com.google.common.base.Predicate; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionContextConsumer; import com.google.devtools.build.lib.actions.ActionContextProvider; import com.google.devtools.build.lib.actions.ActionInputFileCache; @@ -51,7 +50,6 @@ import com.google.devtools.common.options.OptionsProvider; import java.io.IOException; import java.util.Map; -import java.util.Set; import java.util.UUID; import javax.annotation.Nullable; @@ -105,13 +103,6 @@ public abstract class BlazeModule { } /** - * Returns the set of directories under which blaze may assume all files are immutable. - */ - public Set<Path> getImmutableDirectories() { - return ImmutableSet.<Path>of(); - } - - /** * May yield a supplier that provides factories for the Preprocessor to apply. Only one of the * configured modules may return non-null. * diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index a0ab1bc88b..d804e11d67 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -24,7 +24,6 @@ import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Range; @@ -1341,16 +1340,7 @@ public final class BlazeRuntime { } } - Set<Path> immutableDirectories = null; - { - ImmutableSet.Builder<Path> builder = new ImmutableSet.Builder<>(); - for (BlazeModule module : blazeModules) { - builder.addAll(module.getImmutableDirectories()); - } - immutableDirectories = builder.build(); - } - - Iterable<DiffAwareness.Factory> diffAwarenessFactories = null; + Iterable<DiffAwareness.Factory> diffAwarenessFactories; { ImmutableList.Builder<DiffAwareness.Factory> builder = new ImmutableList.Builder<>(); boolean watchFS = startupOptionsProvider != null @@ -1419,7 +1409,6 @@ public final class BlazeRuntime { binTools, workspaceStatusActionFactory, ruleClassProvider.getBuildInfoFactories(), - immutableDirectories, diffAwarenessFactories, allowedMissingInputs, preprocessorFactorySupplier, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java index eddbd9e3f7..fc53e8d23e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java @@ -16,8 +16,10 @@ package com.google.devtools.build.lib.skyframe; import static com.google.devtools.build.lib.skyframe.SkyFunctions.DIRECTORY_LISTING_STATE; import static com.google.devtools.build.lib.skyframe.SkyFunctions.FILE_STATE; +import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.RootedPath; @@ -30,7 +32,7 @@ import java.util.Set; import javax.annotation.Nullable; /** Utilities for checking dirtiness of keys (mainly filesystem keys) in the graph. */ -class DirtinessCheckerUtils { +public class DirtinessCheckerUtils { private DirtinessCheckerUtils() {} static class FileDirtinessChecker extends SkyValueDirtinessChecker { @@ -91,16 +93,53 @@ class DirtinessCheckerUtils { } static final class MissingDiffDirtinessChecker extends BasicFilesystemDirtinessChecker { - private final Set<Path> missingDiffPaths; + private final Set<Path> missingDiffPackageRoots; - MissingDiffDirtinessChecker(final Set<Path> missingDiffPaths) { - this.missingDiffPaths = missingDiffPaths; + MissingDiffDirtinessChecker(final Set<Path> missingDiffPackageRoots) { + this.missingDiffPackageRoots = missingDiffPackageRoots; } @Override public boolean applies(SkyKey key) { return super.applies(key) - && missingDiffPaths.contains(((RootedPath) key.argument()).getRoot()); + && missingDiffPackageRoots.contains(((RootedPath) key.argument()).getRoot()); + } + } + + /** Checks files outside of the package roots for changes. */ + static final class ExternalDirtinessChecker extends BasicFilesystemDirtinessChecker { + private final PathPackageLocator packageLocator; + + ExternalDirtinessChecker(PathPackageLocator packageLocator) { + this.packageLocator = packageLocator; + } + + @Override + public boolean applies(SkyKey key) { + return super.applies(key) + && !ExternalFilesHelper.isInternal((RootedPath) key.argument(), packageLocator); + } + + /** + * Files under output_base/external have a dependency on the WORKSPACE file, so we don't add a + * new SkyValue to the graph yet because it might change once the WORKSPACE file has been + * parsed. + * + * <p>This dirtiness checker is a bit conservative: files that are outside the package roots + * but aren't under output_base/external/ could just be stat-ed here (but they aren't).</p> + */ + @Nullable + @Override + public SkyValue createNewValue(SkyKey key, @Nullable TimestampGranularityMonitor tsgm) { + throw new UnsupportedOperationException(); + } + + @Override + public SkyValueDirtinessChecker.DirtyResult check( + SkyKey skyKey, SkyValue oldValue, @Nullable TimestampGranularityMonitor tsgm) { + return Objects.equal(super.createNewValue(skyKey, tsgm), oldValue) + ? SkyValueDirtinessChecker.DirtyResult.notDirty(oldValue) + : SkyValueDirtinessChecker.DirtyResult.dirty(oldValue); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java index 17d29ed818..100858aaab 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java @@ -13,129 +13,91 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; -import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; -import java.util.Set; import java.util.concurrent.atomic.AtomicReference; /** Common utilities for dealing with files outside the package roots. */ public class ExternalFilesHelper { private final AtomicReference<PathPackageLocator> pkgLocator; - private final Set<Path> immutableDirs; - private final boolean errorOnExternalFiles; - - public ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator) { - this(pkgLocator, ImmutableSet.<Path>of(), /*errorOnExternalFiles=*/false); - } - - @VisibleForTesting - ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, - boolean errorOnExternalFiles) { - this(pkgLocator, ImmutableSet.<Path>of(), errorOnExternalFiles); - } + private final ExternalFileAction externalFileAction; /** * @param pkgLocator an {@link AtomicReference} to a {@link PathPackageLocator} used to - * determine what files are internal - * @param immutableDirs directories whose contents may be considered unchangeable - * @param errorOnExternalFiles whether or not to allow references to files outside of - * the directories provided by pkgLocator or immutableDirs. See - * {@link #maybeHandleExternalFile(RootedPath, SkyFunction.Environment)} for more details. + * determine what files are internal. + * @param errorOnExternalFiles If files outside of package paths should be allowed. */ - ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, Set<Path> immutableDirs, - boolean errorOnExternalFiles) { + public ExternalFilesHelper( + AtomicReference<PathPackageLocator> pkgLocator, boolean errorOnExternalFiles) { this.pkgLocator = pkgLocator; - this.immutableDirs = immutableDirs; - this.errorOnExternalFiles = errorOnExternalFiles; + if (errorOnExternalFiles) { + this.externalFileAction = ExternalFileAction.ERROR_OUT; + } else { + this.externalFileAction = ExternalFileAction.DEPEND_ON_EXTERNAL_PKG; + } + } + + private enum ExternalFileAction { + // Re-check the files when the WORKSPACE file changes. + DEPEND_ON_EXTERNAL_PKG, + + // Throw an exception if there is an external file. + ERROR_OUT, } private enum FileType { - // A file inside the package roots. + // A file inside the package roots or in an external repository. INTERNAL_FILE, - // A file outside the package roots that users of ExternalFilesHelper may pretend is immutable. - EXTERNAL_IMMUTABLE_FILE, - // A file outside the package roots about which we may make no other assumptions. EXTERNAL_MUTABLE_FILE, } - private FileType getFileType(RootedPath rootedPath) { + public static boolean isInternal(RootedPath rootedPath, PathPackageLocator packageLocator) { // TODO(bazel-team): This is inefficient when there are a lot of package roots or there are a - // lot of immutable directories. Consider either explicitly preventing this case or using a more - // efficient approach here (e.g. use a trie for determing if a file is under an immutable + // lot of external directories. Consider either explicitly preventing this case or using a more + // efficient approach here (e.g. use a trie for determining if a file is under an external // directory). - if (!pkgLocator.get().getPathEntries().contains(rootedPath.getRoot())) { - Path path = rootedPath.asPath(); - for (Path immutableDir : immutableDirs) { - if (path.startsWith(immutableDir)) { - return FileType.EXTERNAL_IMMUTABLE_FILE; - } - } - return FileType.EXTERNAL_MUTABLE_FILE; - } - return FileType.INTERNAL_FILE; - } - - public boolean shouldAssumeImmutable(RootedPath rootedPath) { - return getFileType(rootedPath) == FileType.EXTERNAL_IMMUTABLE_FILE; + return packageLocator.getPathEntries().contains(rootedPath.getRoot()); } /** - * Potentially adds a dependency on build_id to env if this instance is configured - * with errorOnExternalFiles=false and rootedPath is an external mutable file. - * If errorOnExternalFiles=true and rootedPath is an external mutable file then - * a FileOutsidePackageRootsException is thrown. If the file is an external file that is - * referenced by the WORKSPACE, it gets a dependency on the //external package (and, thus, - * WORKSPACE file changes). This method is a no-op for any rootedPaths that fall within the known - * package roots. - * - * @param rootedPath - * @param env - * @throws FileOutsidePackageRootsException + * If this instance is configured with DEPEND_ON_EXTERNAL_PKG and rootedPath is a file that isn't + * under a package root then this adds a dependency on the //external package. If the action is + * ERROR_OUT, it will throw an error instead. */ public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env) throws FileOutsidePackageRootsException { - if (getFileType(rootedPath) == FileType.EXTERNAL_MUTABLE_FILE) { - if (!errorOnExternalFiles) { - // For files outside the package roots that are not assumed to be immutable, add a - // dependency on the build_id. This is sufficient for correctness; all other files - // will be handled by diff awareness of their respective package path, but these - // files need to be addressed separately. - // - // Using the build_id here seems to introduce a performance concern because the upward - // transitive closure of these external files will get eagerly invalidated on each - // incremental build (e.g. if every file had a transitive dependency on the filesystem root, - // then we'd have a big performance problem). But this a non-issue by design: - // - We don't add a dependency on the parent directory at the package root boundary, so the - // only transitive dependencies from files inside the package roots to external files are - // through symlinks. So the upwards transitive closure of external files is small. - // - The only way external source files get into the skyframe graph in the first place is - // through symlinks outside the package roots, which we neither want to encourage nor - // optimize for since it is not common. So the set of external files is small. - // - // The above reasoning doesn't hold for bazel, because external repositories - // (e.g. new_local_repository) cause lots of external symlinks to be present in the build. - // So bazel pretends that these external repositories are immutable to avoid the performance - // penalty described above. - PrecomputedValue.dependOnBuildId(env); - } else { - throw new FileOutsidePackageRootsException(rootedPath); + if (isInternal(rootedPath, pkgLocator.get())) { + return; + } + + if (externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG) { + // For files outside the package roots, add a dependency on the //external package so that if + // the WORKSPACE file changes, the File/DirectoryStateValue will be re-evaluated. + // + // Note that: + // - We don't add a dependency on the parent directory at the package root boundary, so + // the only transitive dependencies from files inside the package roots to external files + // are through symlinks. So the upwards transitive closure of external files is small. + // - The only way other than external repositories for external source files to get into the + // skyframe graph in the first place is through symlinks outside the package roots, which we + // neither want to encourage nor optimize for since it is not common. So the set of external + // files is small. + // TODO(kchodorow): check that the path is under output_base/external before adding the dep. + PackageValue pkgValue = (PackageValue) env.getValue(PackageValue.key( + PackageIdentifier.createInDefaultRepo(PackageIdentifier.EXTERNAL_PREFIX))); + if (pkgValue == null) { + return; } - } else if (getFileType(rootedPath) == FileType.EXTERNAL_IMMUTABLE_FILE) { - PackageValue pkgValue = - (PackageValue) - Preconditions.checkNotNull( - env.getValue(PackageValue.key(Package.EXTERNAL_PACKAGE_IDENTIFIER))); Preconditions.checkState(!pkgValue.getPackage().containsErrors()); + } else { + throw new FileOutsidePackageRootsException(rootedPath); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java index 6adf32f03d..e270f41aff 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java @@ -20,7 +20,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.util.Pair; -import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; @@ -30,7 +29,6 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import java.io.IOException; import java.util.ArrayList; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicReference; @@ -46,15 +44,9 @@ import javax.annotation.Nullable; */ public class FileFunction implements SkyFunction { private final AtomicReference<PathPackageLocator> pkgLocator; - private final TimestampGranularityMonitor tsgm; - private final ExternalFilesHelper externalFilesHelper; - public FileFunction(AtomicReference<PathPackageLocator> pkgLocator, - TimestampGranularityMonitor tsgm, - ExternalFilesHelper externalFilesHelper) { + public FileFunction(AtomicReference<PathPackageLocator> pkgLocator) { this.pkgLocator = pkgLocator; - this.tsgm = tsgm; - this.externalFilesHelper = externalFilesHelper; } @Override @@ -101,33 +93,13 @@ public class FileFunction implements SkyFunction { while (realFileStateValue.getType().equals(FileStateValue.Type.SYMLINK)) { symlinkChain.add(realRootedPath); orderedSeenPaths.add(realRootedPath.asPath()); - if (externalFilesHelper.shouldAssumeImmutable(realRootedPath)) { - // If the file is assumed to be immutable, we want to resolve the symlink chain without - // adding dependencies since we don't care about incremental correctness. - try { - Path realPath = rootedPath.asPath().resolveSymbolicLinks(); - realRootedPath = RootedPath.toRootedPathMaybeUnderRoot(realPath, - pkgLocator.get().getPathEntries()); - realFileStateValue = FileStateValue.create(realRootedPath, tsgm); - } catch (IOException e) { - RootedPath root = RootedPath.toRootedPath( - rootedPath.asPath().getFileSystem().getRootDirectory(), - rootedPath.asPath().getFileSystem().getRootDirectory()); - return FileValue.value( - rootedPath, fileStateValue, - root, FileStateValue.NONEXISTENT_FILE_STATE_NODE); - } catch (InconsistentFilesystemException e) { - throw new FileFunctionException(e, Transience.TRANSIENT); - } - } else { - Pair<RootedPath, FileStateValue> resolvedState = getSymlinkTargetRootedPath(realRootedPath, - realFileStateValue.getSymlinkTarget(), orderedSeenPaths, symlinkChain, env); - if (resolvedState == null) { - return null; - } - realRootedPath = resolvedState.getFirst(); - realFileStateValue = resolvedState.getSecond(); + Pair<RootedPath, FileStateValue> resolvedState = getSymlinkTargetRootedPath(realRootedPath, + realFileStateValue.getSymlinkTarget(), orderedSeenPaths, symlinkChain, env); + if (resolvedState == null) { + return null; } + realRootedPath = resolvedState.getFirst(); + realFileStateValue = resolvedState.getSecond(); } return FileValue.value(rootedPath, fileStateValue, realRootedPath, realFileStateValue); } @@ -142,10 +114,7 @@ public class FileFunction implements SkyFunction { PathFragment relativePath = rootedPath.getRelativePath(); RootedPath realRootedPath = rootedPath; FileValue parentFileValue = null; - // We only resolve ancestors if the file is not assumed to be immutable (handling ancestors - // would be too aggressive). - if (!externalFilesHelper.shouldAssumeImmutable(rootedPath) - && !relativePath.equals(PathFragment.EMPTY_FRAGMENT)) { + if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) { RootedPath parentRootedPath = RootedPath.toRootedPath(rootedPath.getRoot(), relativePath.getParentDirectory()); parentFileValue = (FileValue) env.getValue(FileValue.key(parentRootedPath)); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java index 804171d0e3..5abc3c318c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java @@ -126,7 +126,7 @@ public abstract class FileValue implements SkyValue { * Only intended to be used by {@link FileFunction}. Should not be used for symlink cycles. */ static FileValue value(RootedPath rootedPath, FileStateValue fileStateValue, - RootedPath realRootedPath, FileStateValue realFileStateValue) { + RootedPath realRootedPath, FileStateValue realFileStateValue) { if (rootedPath.equals(realRootedPath)) { Preconditions.checkState(fileStateValue.getType() != FileStateValue.Type.SYMLINK, "rootedPath: %s, fileStateValue: %s, realRootedPath: %s, realFileStateValue: %s", @@ -137,7 +137,8 @@ public abstract class FileValue implements SkyValue { return new SymlinkFileValue(realRootedPath, realFileStateValue, fileStateValue.getSymlinkTarget()); } else { - return new DifferentRealPathFileValue(realRootedPath, realFileStateValue); + return new DifferentRealPathFileValue( + realRootedPath, realFileStateValue); } } } @@ -201,7 +202,7 @@ public abstract class FileValue implements SkyValue { protected final FileStateValue realFileStateValue; public DifferentRealPathFileValue(RootedPath realRootedPath, - FileStateValue realFileStateValue) { + FileStateValue realFileStateValue) { this.realRootedPath = Preconditions.checkNotNull(realRootedPath); this.realFileStateValue = Preconditions.checkNotNull(realFileStateValue); } @@ -245,7 +246,7 @@ public abstract class FileValue implements SkyValue { private final PathFragment linkValue; public SymlinkFileValue(RootedPath realRootedPath, FileStateValue realFileState, - PathFragment linkTarget) { + PathFragment linkTarget) { super(realRootedPath, realFileState); this.linkValue = linkTarget; } @@ -276,7 +277,8 @@ public abstract class FileValue implements SkyValue { @Override public int hashCode() { - return Objects.hash(realRootedPath, realFileStateValue, linkValue, Boolean.TRUE); + return Objects.hash( + realRootedPath, realFileStateValue, linkValue, Boolean.TRUE); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index c4b11d22bf..b7f3757dcb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.pkgcache.PackageCacheOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.profiler.AutoProfiler; import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.BasicFilesystemDirtinessChecker; +import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.ExternalDirtinessChecker; import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.MissingDiffDirtinessChecker; import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.UnionDirtinessChecker; import com.google.devtools.build.lib.util.AbruptExitException; @@ -104,7 +105,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, - Set<Path> immutableDirectories, Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories, Predicate<PathFragment> allowedMissingInputs, Preprocessor.Factory.Supplier preprocessorFactorySupplier, @@ -119,11 +119,10 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { binTools, workspaceStatusActionFactory, buildInfoFactories, - immutableDirectories, allowedMissingInputs, preprocessorFactorySupplier, extraSkyFunctions, - extraPrecomputedValues, /*errorOnExternalFiles=*/ + extraPrecomputedValues, false); this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories); this.customDirtinessCheckers = customDirtinessCheckers; @@ -136,7 +135,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, - Set<Path> immutableDirectories, Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories, Predicate<PathFragment> allowedMissingInputs, Preprocessor.Factory.Supplier preprocessorFactorySupplier, @@ -152,7 +150,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { binTools, workspaceStatusActionFactory, buildInfoFactories, - immutableDirectories, diffAwarenessFactories, allowedMissingInputs, preprocessorFactorySupplier, @@ -168,7 +165,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { TimestampGranularityMonitor tsgm, BlazeDirectories directories, BinTools binTools, WorkspaceStatusAction.Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, - Set<Path> immutableDirectories, Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories) { return create( pkgFactory, @@ -177,7 +173,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { binTools, workspaceStatusActionFactory, buildInfoFactories, - immutableDirectories, diffAwarenessFactories, Predicates.<PathFragment>alwaysFalse(), Preprocessor.Factory.Supplier.NullSupplier.INSTANCE, @@ -331,10 +326,6 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { private void handleDiffsWithMissingDiffInformation(EventHandler eventHandler, Set<Pair<Path, DiffAwarenessManager.ProcessableModifiedFileSet>> pathEntriesWithoutDiffInformation) throws InterruptedException { - if (pathEntriesWithoutDiffInformation.isEmpty() && Iterables.isEmpty(customDirtinessCheckers)) { - return; - } - // Before running the FilesystemValueChecker, ensure that all values marked for invalidation // have actually been invalidated (recall that invalidation happens at the beginning of the // next evaluate() call), because checking those is a waste of time. @@ -346,11 +337,12 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { // system values under package roots for which we don't have diff information. If at least // one path entry doesn't have diff information, then we're going to have to iterate over // the skyframe values at least once no matter what. - Set<Path> pathEntries = new HashSet<>(); + Set<Path> diffPackageRootsUnderWhichToCheck = new HashSet<>(); for (Pair<Path, DiffAwarenessManager.ProcessableModifiedFileSet> pair : pathEntriesWithoutDiffInformation) { - pathEntries.add(pair.getFirst()); + diffPackageRootsUnderWhichToCheck.add(pair.getFirst()); } + Differencer.Diff diff = fsvc.getDirtyKeys( memoizingEvaluator.getValues(), @@ -358,8 +350,9 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { Iterables.concat( customDirtinessCheckers, ImmutableList.<SkyValueDirtinessChecker>of( - new MissingDiffDirtinessChecker(pathEntries))))); - handleChangedFiles(pathEntries, diff); + new ExternalDirtinessChecker(pkgLocator.get()), + new MissingDiffDirtinessChecker(diffPackageRootsUnderWhichToCheck))))); + handleChangedFiles(diffPackageRootsUnderWhichToCheck, diff); for (Pair<Path, DiffAwarenessManager.ProcessableModifiedFileSet> pair : pathEntriesWithoutDiffInformation) { @@ -367,11 +360,13 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { } } - private void handleChangedFiles(Collection<Path> pathEntries, Differencer.Diff diff) { + private void handleChangedFiles( + Collection<Path> diffPackageRootsUnderWhichToCheck, Differencer.Diff diff) { Collection<SkyKey> changedKeysWithoutNewValues = diff.changedKeysWithoutNewValues(); Map<SkyKey, SkyValue> changedKeysWithNewValues = diff.changedKeysWithNewValues(); - logDiffInfo(pathEntries, changedKeysWithoutNewValues, changedKeysWithNewValues); + logDiffInfo(diffPackageRootsUnderWhichToCheck, changedKeysWithoutNewValues, + changedKeysWithNewValues); recordingDiffer.invalidate(changedKeysWithoutNewValues); recordingDiffer.inject(changedKeysWithNewValues); @@ -382,8 +377,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { } private static void logDiffInfo(Iterable<Path> pathEntries, - Collection<SkyKey> changedWithoutNewValue, - Map<SkyKey, ? extends SkyValue> changedWithNewValue) { + Collection<SkyKey> changedWithoutNewValue, + Map<SkyKey, ? extends SkyValue> changedWithNewValue) { int numModified = changedWithNewValue.size() + changedWithoutNewValue.size(); StringBuilder result = new StringBuilder("DiffAwareness found ") .append(numModified) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java index 45881c8751..5b6484d57a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java @@ -23,13 +23,10 @@ import com.google.devtools.build.lib.analysis.config.BinTools; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.Preprocessor; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; -import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; -import java.util.Set; - /** * A factory of SkyframeExecutors that returns SequencedSkyframeExecutor. */ @@ -43,7 +40,6 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, - Set<Path> immutableDirectories, Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories, Predicate<PathFragment> allowedMissingInputs, Preprocessor.Factory.Supplier preprocessorFactorySupplier, @@ -57,7 +53,6 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory binTools, workspaceStatusActionFactory, buildInfoFactories, - immutableDirectories, diffAwarenessFactories, allowedMissingInputs, preprocessorFactorySupplier, 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 9a316f4aa1..4b96a2d54d 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 @@ -173,6 +173,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { private final PackageFactory pkgFactory; private final WorkspaceStatusAction.Factory workspaceStatusActionFactory; private final BlazeDirectories directories; + protected final ExternalFilesHelper externalFilesHelper; @Nullable private OutputService outputService; @@ -244,8 +245,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { protected SkyframeProgressReceiver progressReceiver; private final AtomicReference<CyclesReporter> cyclesReporter = new AtomicReference<>(); - private final Set<Path> immutableDirectories; - private final BinTools binTools; private boolean needToInjectEmbeddedArtifacts = true; private boolean needToInjectPrecomputedValuesForAnalysis = true; @@ -275,7 +274,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, - Set<Path> immutableDirectories, Predicate<PathFragment> allowedMissingInputs, Preprocessor.Factory.Supplier preprocessorFactorySupplier, ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions, @@ -296,7 +294,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { resourceManager, eventBus, statusReporterRef); this.directories = Preconditions.checkNotNull(directories); this.buildInfoFactories = buildInfoFactories; - this.immutableDirectories = immutableDirectories; this.allowedMissingInputs = allowedMissingInputs; this.preprocessorFactorySupplier = preprocessorFactorySupplier; this.extraSkyFunctions = extraSkyFunctions; @@ -310,14 +307,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { binTools, (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider()); this.artifactFactory.set(skyframeBuildView.getArtifactFactory()); + this.externalFilesHelper = new ExternalFilesHelper(pkgLocator, this.errorOnExternalFiles); } private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions( Root buildDataDirectory, PackageFactory pkgFactory, Predicate<PathFragment> allowedMissingInputs) { - ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper(pkgLocator, - immutableDirectories, errorOnExternalFiles); RuleClassProvider ruleClassProvider = pkgFactory.getRuleClassProvider(); // We use an immutable map builder for the nice side effect that it throws if a duplicate key // is inserted. @@ -330,7 +326,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { new FileSymlinkCycleUniquenessFunction()); map.put(SkyFunctions.FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS, new FileSymlinkInfiniteExpansionUniquenessFunction()); - map.put(SkyFunctions.FILE, new FileFunction(pkgLocator, tsgm, externalFilesHelper)); + map.put(SkyFunctions.FILE, new FileFunction(pkgLocator)); map.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()); map.put(SkyFunctions.PACKAGE_LOOKUP, new PackageLookupFunction(deletedPackages)); map.put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java index 7cd3498801..ddfdd237b5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java @@ -24,13 +24,10 @@ import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.Preprocessor; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; -import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionName; -import java.util.Set; - /** * A factory that creates instances of SkyframeExecutor. */ @@ -61,7 +58,6 @@ public interface SkyframeExecutorFactory { BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, - Set<Path> immutableDirectories, Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories, Predicate<PathFragment> allowedMissingInputs, Preprocessor.Factory.Supplier preprocessorFactorySupplier, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java index 345800dfd2..a1bb468638 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java @@ -66,12 +66,12 @@ public class WorkspaceFileFunction implements SkyFunction { Path repoWorkspace = workspaceRoot.getRoot().getRelative(workspaceRoot.getRelativePath()); LegacyBuilder builder = com.google.devtools.build.lib.packages.Package.newExternalPackageBuilder( - repoWorkspace, packageFactory.getRuleClassProvider().getRunfilesPrefix()); + repoWorkspace, ruleClassProvider.getRunfilesPrefix()); try (Mutability mutability = Mutability.create("workspace %s", repoWorkspace)) { WorkspaceFactory parser = new WorkspaceFactory( builder, - packageFactory.getRuleClassProvider(), + ruleClassProvider, packageFactory.getEnvironmentExtensions(), mutability, directories.getEmbeddedBinariesRoot(), |