From 5a2936fd02b213f3edbf62193a0426825931d6dd Mon Sep 17 00:00:00 2001 From: Kristina Chodorow Date: Fri, 22 Apr 2016 17:02:19 +0000 Subject: Check for additions to the directory in new_local_repository Fixes #806. RELNOTES: External repository correctness fix: adding a new file/directory as a child of a new_local_repository is now noticed. -- MOS_MIGRATED_REVID=120557511 --- .../repository/NewLocalRepositoryFunction.java | 33 +++++++++-- .../rules/repository/RepositoryDirectoryValue.java | 26 +++++++-- .../lib/rules/repository/RepositoryFunction.java | 66 ++++++++++++++++++++++ .../build/lib/skyframe/DirectoryListingValue.java | 2 +- .../build/lib/skyframe/ExternalFilesHelper.java | 53 +++++------------ .../build/lib/skyframe/SkyframeExecutor.java | 3 +- 6 files changed, 132 insertions(+), 51 deletions(-) (limited to 'src/main/java') diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java index be7826cd28..66942f390d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java @@ -17,10 +17,15 @@ package com.google.devtools.build.lib.rules.repository; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.skyframe.DirectoryListingValue; +import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException; +import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; @@ -38,7 +43,7 @@ public class NewLocalRepositoryFunction extends RepositoryFunction { @Override public SkyValue fetch( Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env) - throws SkyFunctionException { + throws SkyFunctionException { NewRepositoryBuildFileHandler buildFileHandler = new NewRepositoryBuildFileHandler(directories.getWorkspace()); @@ -48,9 +53,29 @@ public class NewLocalRepositoryFunction extends RepositoryFunction { PathFragment pathFragment = getTargetPath(rule, directories.getWorkspace()); + FileSystem fs = directories.getOutputBase().getFileSystem(); + Path path = fs.getPath(pathFragment); + + // fetch() creates symlinks to each child under 'path' and DiffAwareness handles checking all + // of these files and directories for changes. However, if a new file/directory is added + // directly under 'path', Bazel doesn't know that this has to be symlinked in. Thus, this + // creates a dependency on the contents of the 'path' directory. + SkyKey dirKey = DirectoryListingValue.key( + RootedPath.toRootedPath(fs.getRootDirectory(), path)); + DirectoryListingValue directoryValue; + try { + directoryValue = (DirectoryListingValue) env.getValueOrThrow( + dirKey, InconsistentFilesystemException.class); + } catch (InconsistentFilesystemException e) { + throw new RepositoryFunctionException( + new IOException(e), SkyFunctionException.Transience.PERSISTENT); + } + if (directoryValue == null) { + return null; + } + // Link x/y/z to /some/path/to/y/z. - if (!symlinkLocalRepositoryContents( - outputDirectory, directories.getOutputBase().getFileSystem().getPath(pathFragment))) { + if (!symlinkLocalRepositoryContents(outputDirectory, fs.getPath(pathFragment))) { return null; } @@ -68,7 +93,7 @@ public class NewLocalRepositoryFunction extends RepositoryFunction { } createWorkspaceFile(outputDirectory, rule); - return RepositoryDirectoryValue.create(outputDirectory); + return RepositoryDirectoryValue.createWithSourceDirectory(outputDirectory, directoryValue); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java index 5756d51753..9ada38de87 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java @@ -16,28 +16,43 @@ package com.google.devtools.build.lib.rules.repository; import com.google.common.base.Objects; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.skyframe.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import javax.annotation.Nullable; + /** * A local view of an external repository. */ public class RepositoryDirectoryValue implements SkyValue { private final Path path; private final boolean fetchingDelayed; + @Nullable + private final DirectoryListingValue sourceDir; - private RepositoryDirectoryValue(Path path, boolean fetchingDelayed) { + private RepositoryDirectoryValue( + Path path, boolean fetchingDelayed, DirectoryListingValue sourceDir) { this.path = path; this.fetchingDelayed = fetchingDelayed; + this.sourceDir = sourceDir; } /** * Creates an immutable external repository. */ public static RepositoryDirectoryValue create(Path repositoryDirectory) { - return new RepositoryDirectoryValue(repositoryDirectory, false); + return new RepositoryDirectoryValue(repositoryDirectory, false, null); + } + + /** + * new_local_repositories + */ + public static RepositoryDirectoryValue createWithSourceDirectory( + Path repositoryDirectory, DirectoryListingValue sourceDir) { + return new RepositoryDirectoryValue(repositoryDirectory, false, sourceDir); } /** @@ -45,7 +60,7 @@ public class RepositoryDirectoryValue implements SkyValue { * {@code --nofetch} command line option. */ public static RepositoryDirectoryValue fetchingDelayed(Path repositoryDirectory) { - return new RepositoryDirectoryValue(repositoryDirectory, true); + return new RepositoryDirectoryValue(repositoryDirectory, true, null); } /** @@ -70,14 +85,15 @@ public class RepositoryDirectoryValue implements SkyValue { if (other instanceof RepositoryDirectoryValue) { RepositoryDirectoryValue otherValue = (RepositoryDirectoryValue) other; - return path.equals(otherValue.path); + return Objects.equal(path, otherValue.path) + && Objects.equal(sourceDir, otherValue.sourceDir); } return false; } @Override public int hashCode() { - return Objects.hashCode(path); + return Objects.hashCode(path, sourceDir); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 1fd1b5fccc..d9c0b0b697 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.skyframe.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.FileSymlinkException; import com.google.devtools.build.lib.skyframe.FileValue; import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException; @@ -35,6 +36,7 @@ import com.google.devtools.build.lib.skyframe.WorkspaceFileValue; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.Preconditions; +import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -80,6 +82,7 @@ import javax.annotation.Nullable; * {@link RepositoryDirectoryValue} is invalidated using the usual Skyframe route. */ public abstract class RepositoryFunction { + /** * Exception thrown when something goes wrong accessing a remote repository. * @@ -349,6 +352,69 @@ public abstract class RepositoryFunction { .getRelative(Label.EXTERNAL_PATH_PREFIX); } + /** + * For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding rule + * 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. + */ + public static void addExternalFilesDependencies( + RootedPath rootedPath, BlazeDirectories directories, Environment env) + throws IOException { + Path externalRepoDir = getExternalRepositoryDirectory(directories); + PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir); + if (repositoryPath.segmentCount() == 0) { + // We are the top of the repository path (/external), not in an actual external + // repository path. + return; + } + String repositoryName = repositoryPath.getSegment(0); + + Rule repositoryRule; + try { + repositoryRule = RepositoryFunction.getRule(repositoryName, env); + } catch (RepositoryFunction.RepositoryNotFoundException ex) { + // The repository we are looking for does not exist so we should depend on the whole + // WORKSPACE file. In that case, the call to RepositoryFunction#getRule(String, Environment) + // already requested all repository functions from the WORKSPACE file from Skyframe as part + // of the resolution. Therefore we are safe to ignore that Exception. + return; + } catch (RepositoryFunction.RepositoryFunctionException ex) { + // This should never happen. + throw new IllegalStateException( + "Repository " + repositoryName + " cannot be resolved for path " + rootedPath, ex); + } + if (repositoryRule == null) { + return; + } + + // new_local_repository needs a dependency on the directory that `path` points to, as the + // external/repo-name DirStateValue has a logical dependency on that directory that is not + // reflected in the SkyFrame tree, since it's not symlinked to it or anything. + if (repositoryRule.getRuleClass().equals(NewLocalRepositoryRule.NAME) + && repositoryPath.segmentCount() == 1) { + PathFragment pathDir = RepositoryFunction.getTargetPath( + repositoryRule, directories.getWorkspace()); + FileSystem fs = directories.getWorkspace().getFileSystem(); + SkyKey dirKey = DirectoryListingValue.key( + RootedPath.toRootedPath(fs.getRootDirectory(), fs.getPath(pathDir))); + try { + env.getValueOrThrow( + dirKey, IOException.class, FileSymlinkException.class, + InconsistentFilesystemException.class); + } catch (FileSymlinkException | InconsistentFilesystemException e) { + throw new IOException(e.getMessage()); + } + } + } + /** * Returns the RuleDefinition class for this type of repository. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java index 7731c1dd4e..3f61fe660e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java @@ -54,7 +54,7 @@ public abstract class DirectoryListingValue implements SkyValue { * from a directory listing on its parent directory). */ @ThreadSafe - static SkyKey key(RootedPath directoryUnderRoot) { + public static SkyKey key(RootedPath directoryUnderRoot) { return SkyKey.create(SkyFunctions.DIRECTORY_LISTING, directoryUnderRoot); } 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 d55a0297f4..792cc5fa13 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,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -20,16 +21,17 @@ import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; 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.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; +import java.io.IOException; import java.util.concurrent.atomic.AtomicReference; /** Common utilities for dealing with files outside the package roots. */ public class ExternalFilesHelper { private final AtomicReference pkgLocator; private final ExternalFileAction externalFileAction; + private final BlazeDirectories directories; // These variables are set to true from multiple threads, but only read in the main thread. // So volatility or an AtomicBoolean is not needed. @@ -42,18 +44,22 @@ public class ExternalFilesHelper { * @param errorOnExternalFiles If files outside of package paths should be allowed. */ public ExternalFilesHelper( - AtomicReference pkgLocator, boolean errorOnExternalFiles) { + AtomicReference pkgLocator, boolean errorOnExternalFiles, + BlazeDirectories directories) { this( pkgLocator, errorOnExternalFiles ? ExternalFileAction.ERROR_OUT - : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES); + : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES, + directories); } private ExternalFilesHelper(AtomicReference pkgLocator, - ExternalFileAction externalFileAction) { + ExternalFileAction externalFileAction, + BlazeDirectories directories) { this.pkgLocator = pkgLocator; this.externalFileAction = externalFileAction; + this.directories = directories; } private enum ExternalFileAction { @@ -117,7 +123,7 @@ public class ExternalFilesHelper { } ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() { - return new ExternalFilesHelper(pkgLocator, externalFileAction); + return new ExternalFilesHelper(pkgLocator, externalFileAction, directories); } FileType getAndNoteFileType(RootedPath rootedPath) { @@ -152,7 +158,7 @@ public class ExternalFilesHelper { */ @ThreadSafe public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env) - throws FileOutsidePackageRootsException { + throws IOException { FileType fileType = getAndNoteFileType(rootedPath); if (fileType == FileType.INTERNAL) { return; @@ -166,39 +172,6 @@ public class ExternalFilesHelper { Preconditions.checkState( externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES, externalFileAction); - - // For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding rule - // 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. - - Path externalRepoDir = pkgLocator.get().getOutputBase().getRelative(Label.EXTERNAL_PATH_PREFIX); - PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir); - if (repositoryPath.segmentCount() == 0) { - // We are the top of the repository path (/external), not in an actual external - // repository path. - return; - } - String repositoryName = repositoryPath.getSegment(0); - - try { - RepositoryFunction.getRule(repositoryName, env); - } catch (RepositoryFunction.RepositoryNotFoundException ex) { - // The repository we are looking for does not exist so we should depend on the whole - // WORKSPACE file. In that case, the call to RepositoryFunction#getRule(String, Environment) - // already requested all repository functions from the WORKSPACE file from Skyframe as part of - // the resolution. Therefore we are safe to ignore that Exception. - } catch (RepositoryFunction.RepositoryFunctionException ex) { - // This should never happen. - throw new IllegalStateException( - "Repository " + repositoryName + " cannot be resolved for path " + rootedPath, ex); - } + RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env); } } 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 ca8dfadd2d..25bfc92688 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 @@ -310,7 +310,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { binTools, (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider()); this.artifactFactory.set(skyframeBuildView.getArtifactFactory()); - this.externalFilesHelper = new ExternalFilesHelper(pkgLocator, this.errorOnExternalFiles); + this.externalFilesHelper = new ExternalFilesHelper( + pkgLocator, this.errorOnExternalFiles, directories); } private ImmutableMap skyFunctions( -- cgit v1.2.3