Rewrite repository lookup to return a failed value rather than throw
We need to lookup repositories as part of converting exec paths to artifacts,
which in turn is needed for action cache lookups. These lookups should not
cause a Skyframe exit, so we must not throw an exception here, unless the
error makes it impossible to continue. Instead, we need to leave the decision
whether to error out or not to the caller.
Note that we may unnecessarily fetch a remote repository in order to do the
action cache lookup, even if the action no longer depends on the input file,
although this should only be possible for C++ compile actions. It's possible
that there's another bug in the C++ compile action key computation that also
contributes.
This change also makes it so that the post-resolution action cache code
ignores any errors wrt. repository lookup rather than throwing. If any of the
paths could not be found, then the action cache lookup fails and we re-execute
the corresponding action, which is exactly what should happen.
Fixes #2759.
PiperOrigin-RevId: 153696243
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index b5d6c1c..a7f0b82 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -328,7 +328,7 @@
@Nullable
public Iterable<Artifact> getCachedInputs(Action action, PackageRootResolver resolver)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
ActionCache.Entry entry = getCacheEntry(action);
if (entry == null || entry.isCorrupted()) {
return ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
index ddeb001..2a1f663 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
@@ -356,8 +356,7 @@
@Override
public synchronized Map<PathFragment, Artifact> resolveSourceArtifacts(
- Iterable<PathFragment> execPaths, PackageRootResolver resolver)
- throws PackageRootResolutionException, InterruptedException {
+ Iterable<PathFragment> execPaths, PackageRootResolver resolver) throws InterruptedException {
Map<PathFragment, Artifact> result = new HashMap<>();
ArrayList<PathFragment> unresolvedPaths = new ArrayList<>();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
index 7bb3b01..3af6cb9 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
@@ -72,10 +72,8 @@
* existing or new source Artifacts for the given execPaths. The artifact is null if the root
* cannot be determined and the artifact did not exist before. Return null if any dependencies
* are missing.
- * @throws PackageRootResolutionException failure to determine package roots of {@code execPaths}
*/
@Nullable
Map<PathFragment, Artifact> resolveSourceArtifacts(
- Iterable<PathFragment> execPaths, PackageRootResolver resolver)
- throws PackageRootResolutionException, InterruptedException;
+ Iterable<PathFragment> execPaths, PackageRootResolver resolver) throws InterruptedException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
deleted file mode 100644
index d93b7b9..0000000
--- a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolutionException.java
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright 2015 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.actions;
-
-/**
- * Exception signaling an error occurred determining package roots. See
- * {@link PackageRootResolver#findPackageRootsForFiles(Iterable)} for further details.
- */
-public class PackageRootResolutionException extends Exception {
-
- public PackageRootResolutionException(String msg, Throwable cause) {
- super(msg, cause);
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
index 2b7a3ca..35a795a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/PackageRootResolver.java
@@ -30,12 +30,10 @@
* start with the path's parent directory, since the path is assumed to be a file.
* @return mappings from {@code execPath} to {@link Root}, or null if for some reason we cannot
* determine the result at this time (such as when used within a SkyFunction)
- * @throws PackageRootResolutionException if unable to determine package roots or lack thereof,
- * typically caused by exceptions encountered while attempting to locate BUILD files
*/
@Nullable
Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException;
+ throws InterruptedException;
/**
* Returns mapping from execPath to Root. Root will be null if the path has no containing package.
@@ -46,11 +44,9 @@
* start with the path's parent directory, since the path is assumed to be a file.
* @return mappings from {@code execPath} to {@link Root}, or null if for some reason we cannot
* determine the result at this time (such as when used within a SkyFunction)
- * @throws PackageRootResolutionException if unable to determine package roots or lack thereof,
- * typically caused by exceptions encountered while attempting to locate BUILD files
*/
// TODO(bazel-team): Remove this once we don't need to find package roots for directories.
@Nullable
Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException;
+ throws InterruptedException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
index 0a41135..9a9eee3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkyframePackageRootResolver.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.analysis;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -39,14 +38,14 @@
@Override
public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
return executor.getArtifactRootsForFiles(eventHandler, execPaths);
}
@Override
@Nullable
public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
return executor.getArtifactRoots(eventHandler, execPaths);
}
}
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 91b2226..1d340cd 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
@@ -142,7 +142,7 @@
public DirtyResult check(
SkyKey skyKey, SkyValue skyValue, @Nullable TimestampGranularityMonitor tsgm) {
RepositoryDirectoryValue repositoryValue = (RepositoryDirectoryValue) skyValue;
- return repositoryValue.isFetchingDelayed()
+ return repositoryValue.repositoryExists() && repositoryValue.isFetchingDelayed()
? DirtyResult.dirty(skyValue)
: DirtyResult.notDirty(skyValue);
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
index 259950b..1cafeb7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -112,7 +112,12 @@
repositoryName, overrides.get(repositoryName), env, repoRoot, markerPath);
}
- Rule rule = RepositoryFunction.getRule(repositoryName, null, env);
+ Rule rule;
+ try {
+ rule = RepositoryFunction.getRule(repositoryName, null, env);
+ } catch (RepositoryFunction.RepositoryNotFoundException e) {
+ return RepositoryDirectoryValue.NO_SUCH_REPOSITORY_VALUE;
+ }
if (rule == null) {
return null;
}
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 cb0d0ab..4d073d5 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
@@ -25,23 +25,110 @@
import java.util.Arrays;
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 byte[] digest;
- @Nullable
- private final DirectoryListingValue sourceDir;
+/** A local view of an external repository. */
+public abstract class RepositoryDirectoryValue implements SkyValue {
+ /**
+ * Returns whether the repository exists or not. If this return false, then the methods below will
+ * throw.
+ */
+ public abstract boolean repositoryExists();
- private RepositoryDirectoryValue(
- Path path, boolean fetchingDelayed, DirectoryListingValue sourceDir, byte[] digest) {
- this.path = path;
- this.fetchingDelayed = fetchingDelayed;
- this.sourceDir = sourceDir;
- this.digest = digest;
+ /**
+ * Returns the path to the directory containing the repository's contents. This directory is
+ * guaranteed to exist. It may contain a full Bazel repository (with a WORKSPACE file,
+ * directories, and BUILD files) or simply contain a file (or set of files) for, say, a jar from
+ * Maven.
+ */
+ public abstract Path getPath();
+
+ public abstract boolean isFetchingDelayed();
+
+ /** Represents a successful repository lookup. */
+ public static final class SuccessfulRepositoryDirectoryValue extends RepositoryDirectoryValue {
+ private final Path path;
+ private final boolean fetchingDelayed;
+ @Nullable private final byte[] digest;
+ @Nullable private final DirectoryListingValue sourceDir;
+
+ private SuccessfulRepositoryDirectoryValue(
+ Path path, boolean fetchingDelayed, DirectoryListingValue sourceDir, byte[] digest) {
+ this.path = path;
+ this.fetchingDelayed = fetchingDelayed;
+ this.sourceDir = sourceDir;
+ this.digest = digest;
+ }
+
+ @Override
+ public boolean repositoryExists() {
+ return true;
+ }
+
+ @Override
+ public Path getPath() {
+ return path;
+ }
+
+ @Override
+ public boolean isFetchingDelayed() {
+ return fetchingDelayed;
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+
+ if (other instanceof SuccessfulRepositoryDirectoryValue) {
+ SuccessfulRepositoryDirectoryValue otherValue = (SuccessfulRepositoryDirectoryValue) other;
+ return Objects.equal(path, otherValue.path)
+ && Objects.equal(sourceDir, otherValue.sourceDir)
+ && Arrays.equals(digest, otherValue.digest);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(path, sourceDir, Arrays.hashCode(digest));
+ }
+
+ @Override
+ public String toString() {
+ return path.getPathString();
+ }
+ }
+
+ /** Represents an unsuccessful repository lookup. */
+ public static final class NoRepositoryDirectoryValue extends RepositoryDirectoryValue {
+ private NoRepositoryDirectoryValue() {}
+
+ @Override
+ public boolean repositoryExists() {
+ return false;
+ }
+
+ @Override
+ public Path getPath() {
+ throw new IllegalStateException();
+ }
+
+ @Override
+ public boolean isFetchingDelayed() {
+ throw new IllegalStateException();
+ }
+ }
+
+ public static final NoRepositoryDirectoryValue NO_SUCH_REPOSITORY_VALUE =
+ new NoRepositoryDirectoryValue();
+
+ /** Creates a key from the given repository name. */
+ public static SkyKey key(RepositoryName repository) {
+ return SkyKey.create(SkyFunctions.REPOSITORY_DIRECTORY, repository);
+ }
+
+ public static Builder builder() {
+ return new Builder();
}
/** A builder to create a {@link RepositoryDirectoryValue}. */
@@ -53,14 +140,6 @@
private Builder() {}
- public Builder copy(RepositoryDirectoryValue value) {
- this.digest = value.digest;
- this.fetchingDelayed = value.fetchingDelayed;
- this.path = value.path;
- this.sourceDir = value.sourceDir;
- return this;
- }
-
public Builder setPath(Path path) {
this.path = path;
return this;
@@ -81,63 +160,13 @@
return this;
}
- public RepositoryDirectoryValue build() {
+ public SuccessfulRepositoryDirectoryValue build() {
Preconditions.checkNotNull(path, "Repository path must be specified!");
// Only if fetching is delayed then we are allowed to have a null digest.
if (!this.fetchingDelayed) {
Preconditions.checkNotNull(digest, "Repository marker digest must be specified!");
}
- return new RepositoryDirectoryValue(path, fetchingDelayed, sourceDir, digest);
+ return new SuccessfulRepositoryDirectoryValue(path, fetchingDelayed, sourceDir, digest);
}
}
-
- public static Builder builder() {
- return new Builder();
- }
-
- /**
- * Returns the path to the directory containing the repository's contents. This directory is
- * guaranteed to exist. It may contain a full Bazel repository (with a WORKSPACE file,
- * directories, and BUILD files) or simply contain a file (or set of files) for, say, a jar from
- * Maven.
- */
- public Path getPath() {
- return path;
- }
-
- public boolean isFetchingDelayed() {
- return fetchingDelayed;
- }
-
- @Override
- public boolean equals(Object other) {
- if (this == other) {
- return true;
- }
-
- if (other instanceof RepositoryDirectoryValue) {
- RepositoryDirectoryValue otherValue = (RepositoryDirectoryValue) other;
- return Objects.equal(path, otherValue.path)
- && Objects.equal(sourceDir, otherValue.sourceDir)
- && Arrays.equals(digest, otherValue.digest);
- }
- return false;
- }
-
- @Override
- public int hashCode() {
- return Objects.hashCode(path, sourceDir, Arrays.hashCode(digest));
- }
-
- @Override
- public String toString() {
- return path.getPathString();
- }
-
- /**
- * Creates a key from the given repository name.
- */
- public static SkyKey key(RepositoryName repository) {
- return SkyKey.create(SkyFunctions.REPOSITORY_DIRECTORY, repository);
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java
index c6040de..ad0e5ab 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java
@@ -28,7 +28,6 @@
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
import javax.annotation.Nullable;
/**
@@ -49,6 +48,9 @@
if (repository == null) {
return null;
}
+ if (!repository.repositoryExists()) {
+ return RepositoryValue.notFound(nameFromRule);
+ }
SkyKey workspaceKey =
WorkspaceFileValue.key(
@@ -77,7 +79,7 @@
+ "cause a build error in future versions"));
}
- return new RepositoryValue(nameFromRule, repository);
+ return RepositoryValue.success(nameFromRule, repository);
}
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 908e6c5..213ceec 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -30,7 +30,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.causes.Cause;
@@ -39,7 +38,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.util.BlazeClock;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.Preconditions;
@@ -240,13 +238,8 @@
Preconditions.checkState(action.discoversInputs(), action);
PackageRootResolverWithEnvironment resolver = new PackageRootResolverWithEnvironment(env);
- Iterable<Artifact> actionCacheInputs;
- try {
- actionCacheInputs = skyframeActionExecutor.getActionCachedInputs(action, resolver);
- } catch (PackageRootResolutionException rre) {
- throw new ActionExecutionFunctionException(
- new ActionExecutionException("Failed to get cached inputs", rre, action, true));
- }
+ Iterable<Artifact> actionCacheInputs =
+ skyframeActionExecutor.getActionCachedInputs(action, resolver);
if (actionCacheInputs == null) {
Preconditions.checkState(env.valuesMissing(), action);
return null;
@@ -295,7 +288,7 @@
@Override
public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
Preconditions.checkState(keysRequested.isEmpty(),
"resolver should only be called once: %s %s", keysRequested, execPaths);
// Create SkyKeys list based on execPaths.
@@ -310,32 +303,27 @@
depKeys.put(path, depKey);
keysRequested.add(depKey);
} catch (LabelSyntaxException e) {
- throw new PackageRootResolutionException(
- String.format("Could not find the external repository for %s", path), e);
+ // This code is only used to do action cache checks. If one of the file names we got from
+ // the action cache is corrupted, or if the action cache is from a different Bazel
+ // binary, then the path may not be valid for this Bazel binary, and trigger this
+ // exception. In that case, it's acceptable for us to ignore the exception - we'll get an
+ // action cache miss and re-execute the action, which is what we should do.
+ continue;
}
}
- Map<SkyKey,
- ValueOrException2<NoSuchPackageException, InconsistentFilesystemException>> values =
- env.getValuesOrThrow(depKeys.values(), NoSuchPackageException.class,
- InconsistentFilesystemException.class);
- // Check values even if some are missing so that we can throw an appropriate exception if
- // needed.
+ Map<SkyKey, SkyValue> values = env.getValues(depKeys.values());
+ if (env.valuesMissing()) {
+ return null;
+ }
Map<PathFragment, Root> result = new HashMap<>();
for (PathFragment path : execPaths) {
- ContainingPackageLookupValue value;
- try {
- value = (ContainingPackageLookupValue) values.get(depKeys.get(path)).get();
- } catch (NoSuchPackageException | InconsistentFilesystemException e) {
- throw new PackageRootResolutionException(
- String.format("Could not determine containing package for %s", path), e);
- }
-
- if (value == null) {
- Preconditions.checkState(env.valuesMissing(), path);
+ if (!depKeys.containsKey(path)) {
continue;
}
+ ContainingPackageLookupValue value =
+ (ContainingPackageLookupValue) values.get(depKeys.get(path));
if (value.hasContainingPackage()) {
// We have found corresponding root for current execPath.
result.put(path,
@@ -347,15 +335,13 @@
result.put(path, null);
}
}
-
- // If some values are missing, return null.
- return env.valuesMissing() ? null : result;
+ return result;
}
@Override
@Nullable
public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
// call sites for this implementation of PackageRootResolver shouldn't be passing in
// directories.
return findPackageRootsForFiles(execPaths);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 0b7eaf9..10e4ace 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -132,6 +132,11 @@
throw new MissingDepException();
}
+ if (!repositoryValue.repositoryExists()) {
+ // This shouldn't be possible; we're given a repository, so we assume that the caller has
+ // already checked for its existence.
+ throw new IllegalStateException(String.format("No such repository '%s'", repository));
+ }
roots.add(repositoryValue.getPath());
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index a64372d..b5ba1da 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -192,7 +192,7 @@
} else {
RepositoryDirectoryValue repositoryValue =
(RepositoryDirectoryValue) graph.getValue(RepositoryDirectoryValue.key(repository));
- if (repositoryValue == null) {
+ if (repositoryValue == null || !repositoryValue.repositoryExists()) {
// If this key doesn't exist, the repository is outside the universe, so we return
// "nothing".
return ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index f848a90..0b7afce 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -433,6 +433,7 @@
switch (packageLookupValue.getErrorReason()) {
case NO_BUILD_FILE:
case DELETED_PACKAGE:
+ case REPOSITORY_NOT_FOUND:
throw new PackageFunctionException(new BuildFileNotFoundException(packageId,
packageLookupValue.getErrorMsg()), Transience.PERSISTENT);
case INVALID_PACKAGE_NAME:
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 2723c6a..e427c34 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
@@ -307,6 +307,10 @@
throw new PackageLookupFunctionException(new BuildFileNotFoundException(id, e.getMessage()),
Transience.PERSISTENT);
}
+ if (!repositoryValue.repositoryExists()) {
+ // TODO(ulfjack): Maybe propagate the error message from the repository delegator function?
+ return PackageLookupValue.NO_SUCH_REPOSITORY_VALUE;
+ }
// This checks for the build file names in the correct precedence order.
for (BuildFileName buildFileName : buildFilesByPriority) {
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 7db3a2a..5ed0e55 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
@@ -88,16 +88,21 @@
new NoBuildFilePackageLookupValue();
public static final DeletedPackageLookupValue DELETED_PACKAGE_VALUE =
new DeletedPackageLookupValue();
+ public static final NoRepositoryPackageLookupValue NO_SUCH_REPOSITORY_VALUE =
+ new NoRepositoryPackageLookupValue();
enum ErrorReason {
- // There is no BUILD file.
+ /** There is no BUILD file. */
NO_BUILD_FILE,
- // The package name is invalid.
+ /** The package name is invalid. */
INVALID_PACKAGE_NAME,
- // The package is considered deleted because of --deleted_packages.
- DELETED_PACKAGE
+ /** The package is considered deleted because of --deleted_packages. */
+ DELETED_PACKAGE,
+
+ /** The repository was not found. */
+ REPOSITORY_NOT_FOUND
}
protected PackageLookupValue() {
@@ -290,4 +295,23 @@
return "Package is considered deleted due to --deleted_packages";
}
}
+
+ /**
+ * Marker value for repository we could not find. This can happen when looking for a label that
+ * specifies a non-existent repository.
+ */
+ public static class NoRepositoryPackageLookupValue extends UnsuccessfulPackageLookupValue {
+
+ private NoRepositoryPackageLookupValue() {}
+
+ @Override
+ ErrorReason getErrorReason() {
+ return ErrorReason.REPOSITORY_NOT_FOUND;
+ }
+
+ @Override
+ public String getErrorMsg() {
+ return "The repository could not be resolved";
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index 3b4b768..a4a3b1b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -244,6 +244,11 @@
throw new MissingDepException();
}
+ if (!repositoryValue.repositoryExists()) {
+ // This shouldn't be possible; we're given a repository, so we assume that the caller has
+ // already checked for its existence.
+ throw new IllegalStateException(String.format("No such repository '%s'", repository));
+ }
roots.add(repositoryValue.getPath());
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java
index cc055ab..575c2a2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java
@@ -15,51 +15,109 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-/**
- * A repository's name and directory.
- */
-public class RepositoryValue implements SkyValue {
- private final RepositoryName repositoryName;
- private final RepositoryDirectoryValue repositoryDirectory;
+/** A repository's name and directory. */
+public abstract class RepositoryValue implements SkyValue {
+ public abstract boolean repositoryExists();
- /**
- * Creates a repository with a given name in a certain directory.
- */
- public RepositoryValue(RepositoryName repositoryName, RepositoryDirectoryValue repository) {
- this.repositoryName = repositoryName;
- this.repositoryDirectory = repository;
- }
+ /** Returns the path to the repository. */
+ public abstract Path getPath();
- /**
- * Returns the path to the repository.
- */
- public Path getPath() {
- return repositoryDirectory.getPath();
- }
+ /** Successful lookup value. */
+ public static final class SuccessfulRepositoryValue extends RepositoryValue {
+ private final RepositoryName repositoryName;
+ private final RepositoryDirectoryValue repositoryDirectory;
- @Override
- public boolean equals(Object other) {
- if (this == other) {
+ /** Creates a repository with a given name in a certain directory. */
+ public SuccessfulRepositoryValue(
+ RepositoryName repositoryName, RepositoryDirectoryValue repository) {
+ Preconditions.checkArgument(repository.repositoryExists());
+ this.repositoryName = repositoryName;
+ this.repositoryDirectory = repository;
+ }
+
+ @Override
+ public boolean repositoryExists() {
return true;
}
- if (other == null || getClass() != other.getClass()) {
+
+ @Override
+ public Path getPath() {
+ return repositoryDirectory.getPath();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (other == null || getClass() != other.getClass()) {
+ return false;
+ }
+
+ SuccessfulRepositoryValue that = (SuccessfulRepositoryValue) other;
+ return Objects.equal(repositoryName, that.repositoryName)
+ && Objects.equal(repositoryDirectory, that.repositoryDirectory);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(repositoryName, repositoryDirectory);
+ }
+ }
+
+ /** Repository could not be resolved. */
+ public static final class NoRepositoryValue extends RepositoryValue {
+ private final RepositoryName repositoryName;
+
+ private NoRepositoryValue(RepositoryName repositoryName) {
+ this.repositoryName = repositoryName;
+ }
+
+ @Override
+ public boolean repositoryExists() {
return false;
}
- RepositoryValue that = (RepositoryValue) other;
- return Objects.equal(repositoryName, that.repositoryName)
- && Objects.equal(repositoryDirectory, that.repositoryDirectory);
+ @Override
+ public Path getPath() {
+ throw new IllegalStateException();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (other == null || getClass() != other.getClass()) {
+ return false;
+ }
+
+ NoRepositoryValue that = (NoRepositoryValue) other;
+ return Objects.equal(repositoryName, that.repositoryName);
+ }
+
+ @Override
+ public int hashCode() {
+ return repositoryName.hashCode();
+ }
}
- @Override
- public int hashCode() {
- return Objects.hashCode(repositoryName, repositoryDirectory);
+ public static RepositoryValue success(
+ RepositoryName repositoryName, RepositoryDirectoryValue repository) {
+ return new SuccessfulRepositoryValue(repositoryName, repository);
+ }
+
+ public static RepositoryValue notFound(RepositoryName repositoryName) {
+ // TODO(ulfjack): Store the cause here? The two possible causes are that the external package
+ // contains errors, or that the repository with the given name does not exist.
+ return new NoRepositoryValue(repositoryName);
}
public static SkyKey key(RepositoryName repositoryName) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 09072e3..d93fd76 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -51,7 +51,6 @@
import com.google.devtools.build.lib.actions.MutableActionGraph;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.TargetOutOfDateException;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
@@ -524,7 +523,7 @@
@Nullable
Iterable<Artifact> getActionCachedInputs(Action action, PackageRootResolver resolver)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
return actionCacheChecker.getCachedInputs(action, resolver);
}
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 dfc2657..b411402 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
@@ -48,7 +48,6 @@
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.Executor;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
import com.google.devtools.build.lib.actions.ResourceManager;
import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.analysis.AspectCollection;
@@ -760,19 +759,19 @@
public Map<PathFragment, Root> getArtifactRootsForFiles(
final ExtendedEventHandler eventHandler, Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
return getArtifactRoots(eventHandler, execPaths, true);
}
public Map<PathFragment, Root> getArtifactRoots(
final ExtendedEventHandler eventHandler, Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
return getArtifactRoots(eventHandler, execPaths, false);
}
private Map<PathFragment, Root> getArtifactRoots(
final ExtendedEventHandler eventHandler, Iterable<PathFragment> execPaths, boolean forFiles)
- throws PackageRootResolutionException, InterruptedException {
+ throws InterruptedException {
final Map<PathFragment, SkyKey> packageKeys = new HashMap<>();
for (PathFragment execPath : execPaths) {
try {
@@ -780,8 +779,7 @@
PackageIdentifier.discoverFromExecPath(execPath, forFiles);
packageKeys.put(execPath, ContainingPackageLookupValue.key(pkgIdentifier));
} catch (LabelSyntaxException e) {
- throw new PackageRootResolutionException(
- String.format("Could not find the external repository for %s", execPath), e);
+ continue;
}
}
@@ -793,8 +791,7 @@
}
if (result.hasError()) {
- throw new PackageRootResolutionException("Exception encountered determining package roots",
- result.getError().getException());
+ return new HashMap<>();
}
Map<PathFragment, Root> roots = new HashMap<>();
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
index 40c0f94..e9bb9b3 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
@@ -32,18 +32,15 @@
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import javax.annotation.Nullable;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Map.Entry;
-
-import javax.annotation.Nullable;
-
/**
* Tests {@link ArtifactFactory}. Also see {@link ArtifactTest} for a test
* of individual artifacts.
@@ -256,8 +253,7 @@
@Override
@Nullable
- public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths)
- throws PackageRootResolutionException {
+ public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths) {
return null; // unused
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
index 6d2f190..72603e8 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
@@ -34,13 +34,11 @@
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.SkyKey;
-
+import java.io.IOException;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-import java.io.IOException;
-
/**
* Unit tests of specific functionality of ASTFileLookupFunction.
*/
@@ -165,10 +163,10 @@
EvaluationResult<ASTFileLookupValue> result =
SkyframeExecutorTestUtils.evaluate(
getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
- ErrorInfo errorInfo = result.getError(skyKey);
- Throwable e = errorInfo.getException();
- assertEquals(skyKey, errorInfo.getRootCauseOfException());
- assertThat(e).isInstanceOf(ErrorReadingSkylarkExtensionException.class);
- assertThat(e.getMessage()).contains("no such package '@a_remote_repo//remote_pkg'");
+ assertThat(result.get(skyKey).lookupSuccessful()).isFalse();
+ assertThat(result.get(skyKey).getErrorMsg())
+ .contains("Unable to load package for '@a_remote_repo//remote_pkg:BUILD'");
+ assertThat(result.get(skyKey).getErrorMsg())
+ .contains("The repository could not be resolved");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 3f2d91a..52dcc45 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -80,8 +80,7 @@
checkError(
"test/skylark",
"the_rule",
- "no such package '@r//': error loading package 'external': "
- + "The repository named 'r' could not be resolved",
+ "no such package '@r//': The repository could not be resolved",
"load('/test/skylark/extension', 'my_rule')",
"",
"my_rule(name='the_rule')");
diff --git a/src/test/shell/bazel/cross_repository_test.sh b/src/test/shell/bazel/cross_repository_test.sh
index de313e2..9fad3c5 100755
--- a/src/test/shell/bazel/cross_repository_test.sh
+++ b/src/test/shell/bazel/cross_repository_test.sh
@@ -228,9 +228,9 @@
# 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"
+ expect_log "no such package '@bar//': The repository 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"
+ expect_log "no such package '@bar//subbar': The repository could not be resolved"
}
# Test for https://github.com/bazelbuild/bazel/issues/2580
