Add new skyframe function to lookup the repository given a path, and use that to report invalid package references. Fixes #1592. -- MOS_MIGRATED_REVID=137164164
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ErrorDeterminingRepositoryException.java b/src/main/java/com/google/devtools/build/lib/packages/ErrorDeterminingRepositoryException.java new file mode 100644 index 0000000..f8659ae --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/ErrorDeterminingRepositoryException.java
@@ -0,0 +1,27 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.packages; + +/** Exception indicating an attempt to access a repository which is not found or does not exist. */ +public class ErrorDeterminingRepositoryException extends NoSuchThingException { + + public ErrorDeterminingRepositoryException(String message) { + super(message); + } + + public ErrorDeterminingRepositoryException(String message, Throwable cause) { + super(message, cause); + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 54ccc3e..7e1fca4 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.packages; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -475,11 +476,23 @@ return (Rule) targets.get(targetName); } + /** Returns all rules in the package that match the given rule class. */ + public Iterable<Rule> getRulesMatchingRuleClass(final String ruleClass) { + Iterable<Rule> targets = getTargets(Rule.class); + return Iterables.filter( + targets, + new Predicate<Rule>() { + @Override + public boolean apply(@Nullable Rule rule) { + return rule.getRuleClass().equals(ruleClass); + } + }); + } + /** * Returns this package's workspace name. * - * <p>Package-private to encourage callers to get their workspace name from a rule, not a - * package.</p> + * <p>Package-private to encourage callers to get their workspace name from a rule, not a package. */ public String getWorkspaceName() { return workspaceName;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java new file mode 100644 index 0000000..4c97875 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java
@@ -0,0 +1,246 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe; + +import com.google.common.base.Optional; +import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.packages.AggregatingAttributeMapper; +import com.google.devtools.build.lib.packages.BuildFileNotFoundException; +import com.google.devtools.build.lib.packages.ErrorDeterminingRepositoryException; +import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.Package.NameConflictException; +import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule; +import com.google.devtools.build.lib.skyframe.PackageFunction.PackageFunctionException; +import com.google.devtools.build.lib.syntax.Type; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionException; +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 javax.annotation.Nullable; + +/** SkyFunction for {@link LocalRepositoryLookupValue}s. */ +public class LocalRepositoryLookupFunction implements SkyFunction { + + @Override + @Nullable + public String extractTag(SkyKey skyKey) { + return null; + } + + // Implementation note: Although LocalRepositoryLookupValue.NOT_FOUND exists, it should never be + // returned from this method. + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws SkyFunctionException, InterruptedException { + RootedPath directory = (RootedPath) skyKey.argument(); + + // Is this the root directory? If so, we're in the MAIN repository. This assumes that the main + // repository has a WORKSPACE in the root directory, but Bazel will have failed with an error + // before this can be called if that is incorrect. + if (directory.getRelativePath().equals(PathFragment.EMPTY_FRAGMENT)) { + return LocalRepositoryLookupValue.mainRepository(); + } + + // Does this directory contain a WORKSPACE file? + Optional<Boolean> maybeWorkspaceFileExists = maybeGetWorkspaceFileExistence(env, directory); + if (!maybeWorkspaceFileExists.isPresent()) { + return null; + } else if (maybeWorkspaceFileExists.get()) { + Optional<LocalRepositoryLookupValue> maybeRepository = + maybeCheckWorkspaceForRepository(env, directory); + if (!maybeRepository.isPresent()) { + return null; + } + LocalRepositoryLookupValue repository = maybeRepository.get(); + // If the repository that was discovered doesn't exist, continue recursing. + if (repository.exists()) { + return repository; + } + } + + // If we haven't found a repository yet, check the parent directory. + RootedPath parentDirectory = + RootedPath.toRootedPath( + directory.getRoot(), directory.getRelativePath().getParentDirectory()); + return env.getValue(LocalRepositoryLookupValue.key(parentDirectory)); + } + + private Optional<Boolean> maybeGetWorkspaceFileExistence(Environment env, RootedPath directory) + throws InterruptedException, LocalRepositoryLookupFunctionException { + try { + RootedPath workspaceRootedFile = + RootedPath.toRootedPath( + directory.getRoot(), + directory + .getRelativePath() + .getChild(PackageLookupValue.BuildFileName.WORKSPACE.getFilename())); + FileValue workspaceFileValue = + (FileValue) + env.getValueOrThrow( + FileValue.key(workspaceRootedFile), + IOException.class, + FileSymlinkException.class, + InconsistentFilesystemException.class); + if (workspaceFileValue == null) { + return Optional.absent(); + } + return Optional.of(workspaceFileValue.exists()); + } catch (IOException e) { + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "IOException while checking if there is a WORKSPACE file in " + + directory.asPath().getPathString(), + e), + Transience.PERSISTENT); + } catch (FileSymlinkException e) { + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "FileSymlinkException while checking if there is a WORKSPACE file in " + + directory.asPath().getPathString(), + e), + Transience.PERSISTENT); + } catch (InconsistentFilesystemException e) { + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "InconsistentFilesystemException while checking if there is a WORKSPACE file in " + + directory.asPath().getPathString(), + e), + Transience.PERSISTENT); + } + } + + /** + * Checks whether the directory exists and is a workspace root. Returns {@link Optional#absent()} + * if Skyframe needs to re-run, {@link Optional#of(LocalRepositoryLookupValue)} otherwise. + */ + private Optional<LocalRepositoryLookupValue> maybeCheckWorkspaceForRepository( + Environment env, RootedPath directory) + throws InterruptedException, LocalRepositoryLookupFunctionException { + // Look up the main WORKSPACE file by the external package, to find all repositories. + PackageLookupValue externalPackageLookupValue; + try { + externalPackageLookupValue = + (PackageLookupValue) + env.getValueOrThrow( + PackageLookupValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER), + BuildFileNotFoundException.class, + InconsistentFilesystemException.class); + if (externalPackageLookupValue == null) { + return Optional.absent(); + } + } catch (BuildFileNotFoundException e) { + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "BuildFileNotFoundException while loading the //external package", e), + Transience.PERSISTENT); + } catch (InconsistentFilesystemException e) { + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "InconsistentFilesystemException while loading the //external package", e), + Transience.PERSISTENT); + } + + RootedPath workspacePath = + externalPackageLookupValue.getRootedPath(Label.EXTERNAL_PACKAGE_IDENTIFIER); + + SkyKey workspaceKey = WorkspaceFileValue.key(workspacePath); + do { + WorkspaceFileValue value; + try { + value = + (WorkspaceFileValue) + env.getValueOrThrow( + workspaceKey, PackageFunctionException.class, NameConflictException.class); + if (value == null) { + return Optional.absent(); + } + } catch (PackageFunctionException e) { + // TODO(jcater): When WFF is rewritten to not throw a PFE, update this. + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "PackageFunctionException while loading the root WORKSPACE file", e), + Transience.PERSISTENT); + } catch (NameConflictException e) { + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "NameConflictException while loading the root WORKSPACE file", e), + Transience.PERSISTENT); + } + + Package externalPackage = value.getPackage(); + if (externalPackage.containsErrors()) { + Event.replayEventsOn(env.getListener(), externalPackage.getEvents()); + } + + // Find all local_repository rules in the WORKSPACE, and check if any have a "path" attribute + // the same as the requested directory. + Iterable<Rule> localRepositories = + externalPackage.getRulesMatchingRuleClass(LocalRepositoryRule.NAME); + Rule rule = + Iterables.find( + localRepositories, + new Predicate<Rule>() { + @Override + public boolean apply(@Nullable Rule rule) { + AggregatingAttributeMapper mapper = AggregatingAttributeMapper.of(rule); + PathFragment pathAttr = new PathFragment(mapper.get("path", Type.STRING)); + return directory.getRelativePath().equals(pathAttr); + } + }, + null); + if (rule != null) { + try { + return Optional.of( + LocalRepositoryLookupValue.success(RepositoryName.create("@" + rule.getName()))); + } catch (LabelSyntaxException e) { + // This shouldn't be possible if the rule name is valid, and it should already have been + // validated. + throw new LocalRepositoryLookupFunctionException( + new ErrorDeterminingRepositoryException( + "LabelSyntaxException while creating the repository name from the rule " + + rule.getName(), + e), + Transience.PERSISTENT); + } + } + workspaceKey = value.next(); + + // TODO(bazel-team): This loop can be quadratic in the number of load() statements, consider + // rewriting or unrolling. + } while (workspaceKey != null); + + return Optional.of(LocalRepositoryLookupValue.notFound()); + } + + /** + * Used to declare all the exception types that can be wrapped in the exception thrown by {@link + * LocalRepositoryLookupFunction#compute}. + */ + private static final class LocalRepositoryLookupFunctionException extends SkyFunctionException { + public LocalRepositoryLookupFunctionException( + ErrorDeterminingRepositoryException e, Transience transience) { + super(e, transience); + } + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupValue.java new file mode 100644 index 0000000..794dcc3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupValue.java
@@ -0,0 +1,166 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe; + +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; + +/** + * A value that represents a local repository lookup result. + * + * <p>Local repository lookups will always produce a value. The {@code #getRepository} method + * returns the name of the repository that the directory resides in. + */ +public abstract class LocalRepositoryLookupValue implements SkyValue { + + static SkyKey key(RootedPath directory) { + return SkyKey.create(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, directory); + } + + private static final LocalRepositoryLookupValue MAIN_REPO_VALUE = new MainRepositoryLookupValue(); + private static final LocalRepositoryLookupValue NOT_FOUND_VALUE = + new NotFoundLocalRepositoryLookupValue(); + + public static LocalRepositoryLookupValue mainRepository() { + return MAIN_REPO_VALUE; + } + + public static LocalRepositoryLookupValue success(RepositoryName repositoryName) { + return new SuccessfulLocalRepositoryLookupValue(repositoryName); + } + + public static LocalRepositoryLookupValue notFound() { + return NOT_FOUND_VALUE; + } + + /** + * Returns {@code true} if the local repository lookup succeeded and the {@link #getRepository} + * method will return a useful result. + */ + public abstract boolean exists(); + + /** + * Returns the {@link RepositoryName} of the local repository contained in the directory which was + * looked up, {@link RepositoryName#MAIN} if the directory is part of the main repository, or + * throws a {@link IllegalStateException} if there was no repository found. + */ + public abstract RepositoryName getRepository(); + + /** Represents a successful lookup of the main repository. */ + public static final class MainRepositoryLookupValue extends LocalRepositoryLookupValue { + + // This should be a singleton value. + private MainRepositoryLookupValue() {} + + @Override + public boolean exists() { + return true; + } + + @Override + public RepositoryName getRepository() { + return RepositoryName.MAIN; + } + + @Override + public String toString() { + return "MainRepositoryLookupValue"; + } + + @Override + public boolean equals(Object obj) { + // All MainRepositoryLookupValue instances are equivalent. + return obj instanceof MainRepositoryLookupValue; + } + + @Override + public int hashCode() { + return MainRepositoryLookupValue.class.getSimpleName().hashCode(); + } + } + + /** Represents a successful lookup of a local repository. */ + public static final class SuccessfulLocalRepositoryLookupValue + extends LocalRepositoryLookupValue { + private final RepositoryName repositoryName; + + public SuccessfulLocalRepositoryLookupValue(RepositoryName repositoryName) { + this.repositoryName = repositoryName; + } + + @Override + public boolean exists() { + return true; + } + + @Override + public RepositoryName getRepository() { + return repositoryName; + } + + @Override + public String toString() { + return "SuccessfulLocalRepositoryLookupValue(" + repositoryName + ")"; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof SuccessfulLocalRepositoryLookupValue)) { + return false; + } + SuccessfulLocalRepositoryLookupValue other = (SuccessfulLocalRepositoryLookupValue) obj; + return repositoryName.equals(other.repositoryName); + } + + @Override + public int hashCode() { + return repositoryName.hashCode(); + } + } + + /** Represents the state where no repository was found, either local or the main repository. */ + public static final class NotFoundLocalRepositoryLookupValue extends LocalRepositoryLookupValue { + + // This should be a singleton value. + private NotFoundLocalRepositoryLookupValue() {} + + @Override + public boolean exists() { + return false; + } + + @Override + public RepositoryName getRepository() { + throw new IllegalStateException("Repository was not found"); + } + + @Override + public String toString() { + return "NotFoundLocalRepositoryLookupValue"; + } + + @Override + public boolean equals(Object obj) { + // All NotFoundLocalRepositoryLookupValue instances are equivalent. + return obj instanceof NotFoundLocalRepositoryLookupValue; + } + + @Override + public int hashCode() { + return NotFoundLocalRepositoryLookupValue.class.getSimpleName().hashCode(); + } + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java index 86545011..331a081 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -19,10 +19,12 @@ import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.BuildFileNotFoundException; +import com.google.devtools.build.lib.packages.ErrorDeterminingRepositoryException; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName; import com.google.devtools.build.lib.syntax.EvalException; +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; @@ -61,6 +63,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws PackageLookupFunctionException, InterruptedException { PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env); + PackageIdentifier packageKey = (PackageIdentifier) skyKey.argument(); if (PackageFunction.isDefaultsPackage(packageKey)) { return PackageLookupValue.success(pkgLocator.getPathEntries().get(0), BuildFileName.BUILD); @@ -149,10 +152,50 @@ RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry, buildFileFragment); - if (crossRepositoryLabelViolationStrategy != CrossRepositoryLabelViolationStrategy.IGNORE) { - // TODO(jcater): Check for cross repository package label violations. + if (crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.ERROR) { + // Is this path part of a local repository? + RootedPath currentPath = + RootedPath.toRootedPath(packagePathEntry, buildFileFragment.getParentDirectory()); + SkyKey repositoryLookupKey = LocalRepositoryLookupValue.key(currentPath); + + // TODO(jcater): Consider parallelizing these lookups. + LocalRepositoryLookupValue localRepository; + try { + localRepository = + (LocalRepositoryLookupValue) + env.getValueOrThrow( + repositoryLookupKey, ErrorDeterminingRepositoryException.class); + if (localRepository == null) { + return null; + } + } catch (ErrorDeterminingRepositoryException e) { + // If the directory selected isn't part of a repository, that's an error. + // TODO(katre): Improve the error message given here. + throw new PackageLookupFunctionException( + new BuildFileNotFoundException( + packageIdentifier, + "Unable to determine the local repository for directory " + + currentPath.asPath().getPathString()), + Transience.PERSISTENT); + } + + if (localRepository.exists() + && !localRepository.getRepository().equals(packageIdentifier.getRepository())) { + // There is a repository mismatch, this is an error. + // TODO(jcater): Work out the correct package name for this error message. + return PackageLookupValue.invalidPackageName( + "Package crosses into repository " + localRepository.getRepository().getName()); + } + + // There's no local repository, keep going. + } else { + // Future-proof against adding future values to CrossRepositoryLabelViolationStrategy. + Preconditions.checkState( + crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.IGNORE, + crossRepositoryLabelViolationStrategy); } + // Check for the existence of the build file. FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier); if (fileValue == null) { return null;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java index a9861b7..8ad5ae0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
@@ -15,7 +15,6 @@ import com.google.common.base.Objects; import com.google.devtools.build.lib.cmdline.PackageIdentifier; -import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 09dc01b..3e138ad 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -104,6 +104,8 @@ public static final SkyFunctionName EXTERNAL_PACKAGE = SkyFunctionName.create("EXTERNAL_PACKAGE"); public static final SkyFunctionName ACTION_TEMPLATE_EXPANSION = SkyFunctionName.create("ACTION_TEMPLATE_EXPANSION"); + public static final SkyFunctionName LOCAL_REPOSITORY_LOOKUP = + SkyFunctionName.create("LOCAL_REPOSITORY_LOOKUP"); public static Predicate<SkyKey> isSkyFunction(final SkyFunctionName functionName) { return new Predicate<SkyKey>() {
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 146c468..39cf8c8 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
@@ -417,6 +417,7 @@ new RecursiveFilesystemTraversalFunction()); map.put(SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction()); map.put(SkyFunctions.ACTION_TEMPLATE_EXPANSION, new ActionTemplateExpansionFunction()); + map.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); map.putAll(extraSkyFunctions); return map.build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java index 201e8fc..d97c034 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
@@ -19,7 +19,6 @@ import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - import java.io.Serializable; import java.util.Objects; @@ -75,6 +74,11 @@ } @Override + public String toString() { + return importLabel + (inWorkspace ? " (in workspace)" : ""); + } + + @Override public boolean equals(Object obj) { if (this == obj) { return true;
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 87ec20c..faacb3c 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
@@ -110,7 +110,11 @@ return null; } parser.execute(ast, importResult.importMap); - } catch (PackageFunctionException | NameConflictException e) { + } catch (PackageFunctionException e) { + // TODO(jcater): Unwrap the PackageFunctionException and handle the underlying error. + // PFE shouldn't be exposed to callers. + throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT); + } catch (NameConflictException e) { throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT); } @@ -129,12 +133,12 @@ } private static final class WorkspaceFileFunctionException extends SkyFunctionException { - public WorkspaceFileFunctionException(Exception e, Transience transience) { + public WorkspaceFileFunctionException(PackageFunctionException e, Transience transience) { super(e, transience); } - public WorkspaceFileFunctionException(Exception e) { - super(e, Transience.PERSISTENT); + public WorkspaceFileFunctionException(NameConflictException e, Transience transience) { + super(e, transience); } } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java index ea1fc81..fa5f891 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
@@ -77,6 +77,11 @@ skyFunctions.put(SkyFunctions.FILE_STATE, new FileStateFunction( new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper)); skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator)); + skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()); + skyFunctions.put( + SkyFunctions.DIRECTORY_LISTING_STATE, + new DirectoryListingStateFunction(externalFilesHelper)); + skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); RecordingDifferencer differencer = new RecordingDifferencer(); evaluator = new InMemoryMemoizingEvaluator(skyFunctions, differencer); driver = new SequentialBuildDriver(evaluator);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 131dbbb..6de849a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -157,6 +157,7 @@ TestRuleClassProvider.getRuleClassProvider(), fs), directories)) .put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()) + .put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()) .build(), differencer); PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID());
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java index 3b66ae1..e030cef 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java
@@ -105,6 +105,7 @@ skyFunctions.put(SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES, new BlacklistedPackagePrefixesFunction()); skyFunctions.put(SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction()); + skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); differencer = new RecordingDifferencer(); evaluator = new InMemoryMemoizingEvaluator(skyFunctions, differencer);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java index 0209343..4ee3a8e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
@@ -144,6 +144,11 @@ new FileStateFunction( new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper)); skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator)); + skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()); + skyFunctions.put( + SkyFunctions.DIRECTORY_LISTING_STATE, + new DirectoryListingStateFunction(externalFilesHelper)); + skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); return skyFunctions; } @@ -692,7 +697,7 @@ assertGlobMatches("symlinks/*.txt", "symlinks/existing.txt"); } - private class CustomInMemoryFs extends InMemoryFileSystem { + private static final class CustomInMemoryFs extends InMemoryFileSystem { private Map<Path, FileStatus> stubbedStats = Maps.newHashMap();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java new file mode 100644 index 0000000..d4b51bb --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java
@@ -0,0 +1,235 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.util.AnalysisMock; +import com.google.devtools.build.lib.bazel.rules.BazelRulesModule; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.packages.RuleClassProvider; +import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction; +import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; +import com.google.devtools.build.lib.testutil.FoundationTestCase; +import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +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.EvaluationResult; +import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator; +import com.google.devtools.build.skyframe.MemoizingEvaluator; +import com.google.devtools.build.skyframe.RecordingDifferencer; +import com.google.devtools.build.skyframe.SequentialBuildDriver; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link LocalRepositoryLookupFunction}. */ +@RunWith(JUnit4.class) +public class LocalRepositoryLookupFunctionTest extends FoundationTestCase { + private AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages; + private MemoizingEvaluator evaluator; + private SequentialBuildDriver driver; + private RecordingDifferencer differencer; + + @Before + public final void setUp() throws Exception { + AnalysisMock analysisMock = AnalysisMock.get(); + AtomicReference<PathPackageLocator> pkgLocator = + new AtomicReference<>(new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory))); + deletedPackages = new AtomicReference<>(ImmutableSet.<PackageIdentifier>of()); + BlazeDirectories directories = + new BlazeDirectories( + rootDirectory, outputBase, rootDirectory, analysisMock.getProductName()); + ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper( + pkgLocator, ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, directories); + + Map<SkyFunctionName, SkyFunction> skyFunctions = new HashMap<>(); + skyFunctions.put( + SkyFunctions.PACKAGE_LOOKUP, + new PackageLookupFunction(deletedPackages, CrossRepositoryLabelViolationStrategy.ERROR)); + skyFunctions.put( + SkyFunctions.FILE_STATE, + new FileStateFunction( + new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper)); + skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator)); + skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()); + skyFunctions.put( + SkyFunctions.DIRECTORY_LISTING_STATE, + new DirectoryListingStateFunction(externalFilesHelper)); + RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); + skyFunctions.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider)); + skyFunctions.put( + SkyFunctions.WORKSPACE_FILE, + new WorkspaceFileFunction( + ruleClassProvider, + analysisMock + .getPackageFactoryForTesting() + .create( + ruleClassProvider, + new BazelRulesModule().getPackageEnvironmentExtension(), + scratch.getFileSystem()), + directories)); + skyFunctions.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()); + skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); + skyFunctions.put( + SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction()); + + differencer = new RecordingDifferencer(); + evaluator = new InMemoryMemoizingEvaluator(skyFunctions, differencer); + driver = new SequentialBuildDriver(evaluator); + PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); + } + + private SkyKey createKey(RootedPath directory) { + return LocalRepositoryLookupValue.key(directory); + } + + private LocalRepositoryLookupValue lookupDirectory(RootedPath directory) + throws InterruptedException { + SkyKey key = createKey(directory); + return lookupDirectory(key).get(key); + } + + private EvaluationResult<LocalRepositoryLookupValue> lookupDirectory(SkyKey directoryKey) + throws InterruptedException { + return driver.<LocalRepositoryLookupValue>evaluate( + ImmutableList.of(directoryKey), + false, + SkyframeExecutor.DEFAULT_THREAD_COUNT, + NullEventHandler.INSTANCE); + } + + @Test + public void testNoPath() throws Exception { + LocalRepositoryLookupValue repositoryLookupValue = + lookupDirectory(RootedPath.toRootedPath(rootDirectory, PathFragment.EMPTY_FRAGMENT)); + assertThat(repositoryLookupValue).isNotNull(); + assertThat(repositoryLookupValue.getRepository()).isEqualTo(RepositoryName.MAIN); + } + + @Test + public void testActualPackage() throws Exception { + scratch.file("some/path/BUILD"); + + LocalRepositoryLookupValue repositoryLookupValue = + lookupDirectory(RootedPath.toRootedPath(rootDirectory, new PathFragment("some/path"))); + assertThat(repositoryLookupValue).isNotNull(); + assertThat(repositoryLookupValue.getRepository()).isEqualTo(RepositoryName.MAIN); + } + + @Test + public void testLocalRepository() throws Exception { + scratch.overwriteFile("WORKSPACE", "local_repository(name='local', path='local/repo')"); + scratch.file("local/repo/WORKSPACE"); + scratch.file("local/repo/BUILD"); + + LocalRepositoryLookupValue repositoryLookupValue = + lookupDirectory(RootedPath.toRootedPath(rootDirectory, new PathFragment("local/repo"))); + assertThat(repositoryLookupValue).isNotNull(); + assertThat(repositoryLookupValue.getRepository().getName()).isEqualTo("@local"); + } + + @Test + public void testLocalRepositorySubPackage() throws Exception { + scratch.overwriteFile("WORKSPACE", "local_repository(name='local', path='local/repo')"); + scratch.file("local/repo/WORKSPACE"); + scratch.file("local/repo/BUILD"); + scratch.file("local/repo/sub/package/BUILD"); + + LocalRepositoryLookupValue repositoryLookupValue = + lookupDirectory( + RootedPath.toRootedPath(rootDirectory, new PathFragment("local/repo/sub/package"))); + assertThat(repositoryLookupValue).isNotNull(); + assertThat(repositoryLookupValue.getRepository().getName()).isEqualTo("@local"); + } + + @Test + public void testWorkspaceButNoLocalRepository() throws Exception { + scratch.overwriteFile("WORKSPACE", ""); + scratch.file("local/repo/WORKSPACE"); + scratch.file("local/repo/BUILD"); + + LocalRepositoryLookupValue repositoryLookupValue = + lookupDirectory(RootedPath.toRootedPath(rootDirectory, new PathFragment("local/repo"))); + assertThat(repositoryLookupValue).isNotNull(); + assertThat(repositoryLookupValue.getRepository()).isEqualTo(RepositoryName.MAIN); + } + + @Test + public void testLocalRepository_LocalWorkspace_SymlinkCycle() throws Exception { + scratch.overwriteFile("WORKSPACE", "local_repository(name='local', path='local/repo')"); + Path localRepoWorkspace = scratch.resolve("local/repo/WORKSPACE"); + Path localRepoWorkspaceLink = scratch.resolve("local/repo/WORKSPACE.link"); + FileSystemUtils.createDirectoryAndParents(localRepoWorkspace.getParentDirectory()); + FileSystemUtils.createDirectoryAndParents(localRepoWorkspaceLink.getParentDirectory()); + localRepoWorkspace.createSymbolicLink(localRepoWorkspaceLink); + localRepoWorkspaceLink.createSymbolicLink(localRepoWorkspace); + scratch.file("local/repo/BUILD"); + + SkyKey localRepositoryKey = + createKey(RootedPath.toRootedPath(rootDirectory, new PathFragment("local/repo"))); + EvaluationResult<LocalRepositoryLookupValue> result = lookupDirectory(localRepositoryKey); + + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(localRepositoryKey) + .hasExceptionThat() + .hasMessage( + "FileSymlinkException while checking if there is a WORKSPACE file in " + + "/workspace/local/repo"); + } + + @Test + public void testLocalRepository_MainWorkspace_NotFound() throws Exception { + // Do not add a local_repository to WORKSPACE. + scratch.overwriteFile("WORKSPACE", ""); + scratch.deleteFile("WORKSPACE"); + scratch.file("local/repo/WORKSPACE"); + scratch.file("local/repo/BUILD"); + + LocalRepositoryLookupValue repositoryLookupValue = + lookupDirectory(RootedPath.toRootedPath(rootDirectory, new PathFragment("local/repo"))); + assertThat(repositoryLookupValue).isNotNull(); + // In this case, the repository should be MAIN as we can't find any local_repository rules. + assertThat(repositoryLookupValue.getRepository()).isEqualTo(RepositoryName.MAIN); + } + + // TODO(katre): Add tests for the following exceptions + // While reading dir/WORKSPACE: + // - IOException + // - FileSymlinkException + // - InconsistentFilesystemException + // While loading //external + // - BuildFileNotFoundException + // - InconsistentFilesystemException + // While reading //external:WORKSPACE + // - PackageFunctionException + // - NameConflictException + // - WorkspaceFileException +}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java index 40bebb7..a8ead28 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
@@ -14,12 +14,14 @@ package com.google.devtools.build.lib.skyframe; +import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -27,17 +29,25 @@ import com.google.devtools.build.lib.bazel.rules.BazelRulesModule; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.packages.BuildFileNotFoundException; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction; +import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule; +import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; +import com.google.devtools.build.lib.rules.repository.RepositoryFunction; +import com.google.devtools.build.lib.rules.repository.RepositoryLoaderFunction; import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName; import com.google.devtools.build.lib.skyframe.PackageLookupValue.ErrorReason; import com.google.devtools.build.lib.testutil.FoundationTestCase; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; +import com.google.devtools.build.lib.vfs.FileSystemUtils; 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.EvaluationResult; import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator; import com.google.devtools.build.skyframe.MemoizingEvaluator; import com.google.devtools.build.skyframe.RecordingDifferencer; @@ -48,6 +58,7 @@ import java.util.HashMap; import java.util.Map; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import org.junit.Before; import org.junit.Test; @@ -88,6 +99,10 @@ skyFunctions.put(SkyFunctions.FILE_STATE, new FileStateFunction( new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper)); skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator)); + skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction()); + skyFunctions.put( + SkyFunctions.DIRECTORY_LISTING_STATE, + new DirectoryListingStateFunction(externalFilesHelper)); skyFunctions.put(SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES, new BlacklistedPackagePrefixesFunction()); RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); @@ -104,6 +119,17 @@ scratch.getFileSystem()), directories)); skyFunctions.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()); + skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); + skyFunctions.put( + SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction()); + + ImmutableMap<String, RepositoryFunction> repositoryHandlers = + ImmutableMap.of(LocalRepositoryRule.NAME, new LocalRepositoryFunction()); + skyFunctions.put( + SkyFunctions.REPOSITORY_DIRECTORY, + new RepositoryDelegatorFunction(repositoryHandlers, null, new AtomicBoolean(true))); + skyFunctions.put(SkyFunctions.REPOSITORY, new RepositoryLoaderFunction()); + differencer = new RecordingDifferencer(); evaluator = new InMemoryMemoizingEvaluator(skyFunctions, differencer); driver = new SequentialBuildDriver(evaluator); @@ -111,18 +137,26 @@ PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get()); PrecomputedValue.BLACKLISTED_PACKAGE_PREFIXES_FILE.set( differencer, PathFragment.EMPTY_FRAGMENT); + PrecomputedValue.BLAZE_DIRECTORIES.set(differencer, directories); } - private PackageLookupValue lookupPackage(String packageName) throws InterruptedException { + protected PackageLookupValue lookupPackage(String packageName) throws InterruptedException { return lookupPackage(PackageIdentifier.createInMainRepo(packageName)); } - private PackageLookupValue lookupPackage(PackageIdentifier packageId) + protected PackageLookupValue lookupPackage(PackageIdentifier packageId) throws InterruptedException { SkyKey key = PackageLookupValue.key(packageId); + return lookupPackage(key).get(key); + } + + protected EvaluationResult<PackageLookupValue> lookupPackage(SkyKey packageIdentifierSkyKey) + throws InterruptedException { return driver.<PackageLookupValue>evaluate( - ImmutableList.of(key), false, SkyframeExecutor.DEFAULT_THREAD_COUNT, - NullEventHandler.INSTANCE).get(key); + ImmutableList.of(packageIdentifierSkyKey), + false, + SkyframeExecutor.DEFAULT_THREAD_COUNT, + NullEventHandler.INSTANCE); } @Test @@ -256,35 +290,83 @@ .testEquals(); } + protected void createAndCheckInvalidPackageLabel(boolean expectedPackageExists) throws Exception { + scratch.overwriteFile("WORKSPACE", "local_repository(name='local', path='local/repo')"); + scratch.file("local/repo/WORKSPACE"); + scratch.file("local/repo/BUILD"); + + // First, use the correct label. + PackageLookupValue packageLookupValue = + lookupPackage(PackageIdentifier.create("@local", PathFragment.EMPTY_FRAGMENT)); + assertTrue(packageLookupValue.packageExists()); + + // Then, use the incorrect label. + packageLookupValue = lookupPackage(PackageIdentifier.createInMainRepo("local/repo")); + assertEquals(expectedPackageExists, packageLookupValue.packageExists()); + } + /** - * Runs all tests in the base {@link PackageLookupFunctionTest} class with the - * {@link CrossRepositoryLabelViolationStrategy#IGNORE} enum set, and also additional tests - * specific to that setting. + * Runs all tests in the base {@link PackageLookupFunctionTest} class with the {@link + * CrossRepositoryLabelViolationStrategy#IGNORE} enum set, and also additional tests specific to + * that setting. */ @RunWith(JUnit4.class) - public static class IgnoreLabelViolationsTest - extends PackageLookupFunctionTest { + public static class IgnoreLabelViolationsTest extends PackageLookupFunctionTest { @Override protected CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy() { return CrossRepositoryLabelViolationStrategy.IGNORE; } // Add any ignore-specific tests here. + + @Test + public void testInvalidPackageLabelIsIgnored() throws Exception { + createAndCheckInvalidPackageLabel(true); + } } /** - * Runs all tests in the base {@link PackageLookupFunctionTest} class with the - * {@link CrossRepositoryLabelViolationStrategy#ERROR} enum set, and also additional tests - * specific to that setting. + * Runs all tests in the base {@link PackageLookupFunctionTest} class with the {@link + * CrossRepositoryLabelViolationStrategy#ERROR} enum set, and also additional tests specific to + * that setting. */ @RunWith(JUnit4.class) - public static class ErrorLabelViolationsTest - extends PackageLookupFunctionTest { + public static class ErrorLabelViolationsTest extends PackageLookupFunctionTest { @Override protected CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy() { return CrossRepositoryLabelViolationStrategy.ERROR; } // Add any error-specific tests here. + + @Test + public void testInvalidPackageLabelIsError() throws Exception { + createAndCheckInvalidPackageLabel(false); + } + + @Test + public void testSymlinkCycleInWorkspace() throws Exception { + scratch.overwriteFile("WORKSPACE", "local_repository(name='local', path='local/repo')"); + Path localRepoWorkspace = scratch.resolve("local/repo/WORKSPACE"); + Path localRepoWorkspaceLink = scratch.resolve("local/repo/WORKSPACE.link"); + FileSystemUtils.createDirectoryAndParents(localRepoWorkspace.getParentDirectory()); + FileSystemUtils.createDirectoryAndParents(localRepoWorkspaceLink.getParentDirectory()); + localRepoWorkspace.createSymbolicLink(localRepoWorkspaceLink); + localRepoWorkspaceLink.createSymbolicLink(localRepoWorkspace); + scratch.file("local/repo/BUILD"); + + SkyKey skyKey = PackageLookupValue.key(PackageIdentifier.createInMainRepo("local/repo")); + EvaluationResult<PackageLookupValue> result = lookupPackage(skyKey); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skyKey) + .hasExceptionThat() + .isInstanceOf(BuildFileNotFoundException.class); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skyKey) + .hasExceptionThat() + .hasMessage( + "no such package 'local/repo': Unable to determine the local repository for " + + "directory /workspace/local/repo"); + } } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index b769b6d..58b31e7 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -120,6 +120,7 @@ .create(ruleClassProvider, scratch.getFileSystem()), directories)); skyFunctions.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction()); + skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); progressReceiver = new RecordingEvaluationProgressReceiver(); differencer = new RecordingDifferencer();
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 241577a..9ffd05d 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD
@@ -202,6 +202,13 @@ ) sh_test( + name = "cross_repository_test", + size = "large", + srcs = ["cross_repository_test.sh"], + data = [":test-deps"], +) + +sh_test( name = "external_skylark_load_test", size = "large", srcs = ["external_skylark_load_test.sh"],
diff --git a/src/test/shell/bazel/cross_repository_test.sh b/src/test/shell/bazel/cross_repository_test.sh new file mode 100755 index 0000000..b484437 --- /dev/null +++ b/src/test/shell/bazel/cross_repository_test.sh
@@ -0,0 +1,241 @@ +#!/bin/bash +# +# Copyright 2016 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Tests for package labels that cross repository boundaries +# Includes regression tests for #1592. +# + +# Load the test setup defined in the parent directory +CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${CURRENT_DIR}/../integration_test_setup.sh" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +function write_rule { + name=$1 + shift + srcs="" + for src in "$@"; do + srcs="\"$src\", $srcs" + done + + cat <<EOF +genrule( + name = "$name", + srcs = [ + $srcs + ], + outs = ["$name.out"], + cmd = "echo $name > \$@", + visibility = ["//visibility:public"], +) +EOF +} + +# Basic use case: Define a repository nested in the main repository, check +# correct targets are visible +function test_basic_cross_repo_targets { + cat > WORKSPACE <<EOF +local_repository( + name = "bar", + path = "bar", +) +EOF + + write_rule depend_via_repo "@bar//:bar" >> BUILD + write_rule depend_via_repo_subdir "@bar//subbar:subbar" >> BUILD + write_rule depend_directly "//bar:bar" >> BUILD + write_rule depend_directly_subdir "//bar/subbar:subbar" >> BUILD + + mkdir -p bar + touch bar/WORKSPACE + write_rule bar >> bar/BUILD + mkdir -p bar/subbar + write_rule subbar >> bar/subbar/BUILD + + # These should succeed, they use the correct label. + bazel build //:depend_via_repo || fail "build should succeed" + bazel build //:depend_via_repo_subdir || fail "build should succeed" + + # These should fail, they is using the wrong label that crosses the + # repository boundary. + bazel build //:depend_directly >& $TEST_log && fail "build should fail" + expect_log "no such package 'bar': Package crosses into repository @bar" + bazel build //:depend_directly_subdir >& $TEST_log && fail "build should fail" + expect_log "no such package 'bar/subbar': Package crosses into repository @bar" +} + +function test_top_level_local_repository { + cat > WORKSPACE <<EOF +local_repository( + name = "loc", + path = ".", +) +EOF + + write_rule foo >> BUILD + + # These should succeed, they use the correct label. + bazel build //:foo || fail "build should succeed" + bazel build @loc//:foo || fail "build should succeed" +} + +# Loading rules. +function test_workspace_loads_rules { + cat > WORKSPACE <<EOF +load("//baz:rules.bzl", "baz_rule") +local_repository( + name = "bar", + path = "bar", +) +EOF + + write_rule depend_via_repo "@bar//:bar" >> BUILD + write_rule depend_directly "//bar:bar" >> BUILD + + mkdir -p bar + touch bar/WORKSPACE + write_rule bar >> bar/BUILD + + mkdir -p baz + touch baz/BUILD + cat > baz/rules.bzl <<EOF +baz_rule = 'baz_rule' +EOF + + # This should succeed, it uses the correct label. + bazel build //:depend_via_repo || fail "build should succeed" + + # This should fail, it is using the wrong label that crosses the repository + # boundary. + bazel build //:depend_directly >& $TEST_log && fail "build should fail" + expect_log "no such package 'bar': Package crosses into repository @bar" +} + +# Load rules via an invalid label. +function test_workspace_loads_rules_failure { + cat > WORKSPACE <<EOF +load("//bar:rules.bzl", "bar_rule") +local_repository( + name = "bar", + path = "bar", +) +EOF + + write_rule depend_via_repo "@bar//:bar" >> BUILD + + mkdir -p bar + touch bar/WORKSPACE + write_rule bar >> bar/BUILD + cat > bar/rules.bzl <<EOF +bar_rule = 'bar_rule' +EOF + + # This should fail in workspace parsing. + bazel build //:depend_via_repo >& $TEST_log && fail "build should fail" + # TODO(jcater): Show a better error when this occurs + #expect_log "no such package 'bar': Package crosses into repository @bar" + # This is the current error shown. + expect_log "cycles detected" +} + +function test_top_level_local_repository { + cat > WORKSPACE <<EOF +local_repository( + name = "loc", + path = ".", +) +EOF + + write_rule foo >> BUILD + + # These should succeed, they use the correct label. + bazel build //:foo || fail "build should succeed" + bazel build @loc//:foo || fail "build should succeed" +} + +function test_incremental_add_repository { + # Empty workspace, defines no local_repository. + cat > WORKSPACE + + mkdir -p bar + write_rule bar >> bar/BUILD + mkdir -p bar/subbar + write_rule subbar >> bar/subbar/BUILD + + bazel query //bar/... >& $TEST_log || fail "query should succeed" + + # Now add the local_repository and WORKSPACE. + cat > WORKSPACE <<EOF +local_repository( + name = "bar", + path = "bar", +) +EOF + touch bar/WORKSPACE + + # These should now fail, using the incorrect label. + bazel query //bar:bar >& $TEST_log && fail "build should fail" + expect_log "no such package 'bar': Package crosses into repository @bar" + bazel query //bar/subbar:subbar >& $TEST_log && fail "build should fail" + expect_log "no such package 'bar/subbar': Package crosses into repository @bar" + + # These should succeed. + echo "about to test @bar//" + bazel query @bar//:bar || fail "query should succeed" + bazel query @bar//subbar:subbar || fail "query should succeed" +} + +function test_incremental_remove_repository { + # Add the local_repository and WORKSPACE. + cat > WORKSPACE <<EOF +local_repository( + name = "bar", + path = "bar", +) +EOF + mkdir -p bar + touch bar/WORKSPACE + write_rule bar >> bar/BUILD + mkdir -p bar/subbar + write_rule subbar >> bar/subbar/BUILD + + # These should succeed. + echo "about to test @bar//" + bazel query @bar//:bar || fail "query should succeed" + bazel query @bar//subbar:subbar || fail "query should succeed" + + # Now remove the workspace and the local_repository. + cat > WORKSPACE + rm bar/WORKSPACE + + # These should now succeed. + bazel query //bar:bar || fail "query should succeed" + bazel query //bar/subbar:subbar || fail "query should succeed" + + + # These should now fail. + bazel query @bar//:bar >& $TEST_log && fail "build should fail" + expect_log "no such package '@bar//':.*The repository named 'bar' could not be resolved" + bazel query @bar//subbar:subbar >& $TEST_log && fail "build should fail" + expect_log "no such package '@bar//subbar':.*The repository named 'bar' could not be resolved" +} + +# TODO(katre): Add tests to verify incremental package reloads are necessary and correct. +# - /WORKSPACE edited, rule not changed - no reload +# - /WORKSPACE not edited, /dir/WORKSPACE added or removed - only packages in +# /dir invalidated and re-loaded + +run_suite "cross-repository tests"