Pass through information about package lookup failure into import lookup.
This fixes 'Misleading error message "file must have a corresponding package"' '#7094.
PiperOrigin-RevId: 235806611
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java
index f3ca988..0cdc9e8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunction.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.ErrorReason;
import com.google.devtools.build.lib.skyframe.PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
@@ -49,6 +50,9 @@
return env.getValue(ContainingPackageLookupValue.key(correctPackageIdentifier));
}
+ if (ErrorReason.REPOSITORY_NOT_FOUND.equals(pkgLookupValue.getErrorReason())) {
+ return new ContainingPackageLookupValue.NoContainingPackage(pkgLookupValue.getErrorMsg());
+ }
PathFragment parentDir = dir.getPackageFragment().getParentDirectory();
if (parentDir == null) {
return ContainingPackageLookupValue.NONE;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
index 7ab319c..302d6db 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import javax.annotation.Nonnull;
/**
* A value that represents the result of looking for the existence of a package that owns a
@@ -45,6 +46,14 @@
/** If there is a containing package, returns its package root */
public abstract Root getContainingPackageRoot();
+ /**
+ * If there is not a containing package, returns a reason why (this is usually the reason the
+ * outer-most directory isn't a package).
+ */
+ public String getReasonForNoContainingPackage() {
+ throw new IllegalStateException();
+ }
+
public static Key key(PackageIdentifier id) {
Preconditions.checkArgument(!id.getPackageFragment().isAbsolute(), id);
Preconditions.checkArgument(!id.getRepository().isDefault(), id);
@@ -112,7 +121,15 @@
/** Value indicating there is no containing package. */
public static class NoContainingPackage extends ContainingPackageLookupValue {
- private NoContainingPackage() {}
+ private final String reason;
+
+ private NoContainingPackage() {
+ this.reason = null;
+ }
+
+ NoContainingPackage(@Nonnull String reason) {
+ this.reason = reason;
+ }
@Override
public boolean hasContainingPackage() {
@@ -133,6 +150,11 @@
public String toString() {
return getClass().getName();
}
+
+ @Override
+ public String getReasonForNoContainingPackage() {
+ return reason;
+ }
}
/** A successful lookup value. */
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 aed63e4..95fe4be 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
@@ -321,7 +321,7 @@
}
if (!repositoryValue.repositoryExists()) {
// TODO(ulfjack): Maybe propagate the error message from the repository delegator function?
- return PackageLookupValue.NO_SUCH_REPOSITORY_VALUE;
+ return new PackageLookupValue.NoRepositoryPackageLookupValue(id.getRepository().getName());
}
// This checks for the build file names in the correct precedence order.
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 f5c5f8c..7d24e17 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
@@ -51,10 +51,6 @@
public static final DeletedPackageLookupValue DELETED_PACKAGE_VALUE =
new DeletedPackageLookupValue();
- @AutoCodec
- public static final NoRepositoryPackageLookupValue NO_SUCH_REPOSITORY_VALUE =
- new NoRepositoryPackageLookupValue();
-
enum ErrorReason {
/** There is no BUILD file. */
NO_BUILD_FILE,
@@ -391,11 +387,15 @@
}
/**
- * Marker value for repository we could not find. This can happen when looking for a label that
- * specifies a non-existent repository.
+ * 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() {}
+ private final String repositoryName;
+
+ NoRepositoryPackageLookupValue(String repositoryName) {
+ this.repositoryName = repositoryName;
+ }
@Override
ErrorReason getErrorReason() {
@@ -404,7 +404,7 @@
@Override
public String getErrorMsg() {
- return "The repository could not be resolved";
+ return String.format("The repository '%s' could not be resolved", repositoryName);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index 0c3c081..179dc54 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -249,7 +249,8 @@
return null;
}
if (!containingPackageLookupValue.hasContainingPackage()) {
- throw SkylarkImportFailedException.noBuildFile(fileLabel.toPathFragment());
+ throw SkylarkImportFailedException.noBuildFile(
+ fileLabel, containingPackageLookupValue.getReasonForNoContainingPackage());
}
if (!containingPackageLookupValue.getContainingPackageName().equals(
fileLabel.getPackageIdentifier())) {
@@ -600,7 +601,11 @@
cause);
}
- static SkylarkImportFailedException noBuildFile(PathFragment file) {
+ static SkylarkImportFailedException noBuildFile(Label file, @Nullable String reason) {
+ if (reason != null) {
+ return new SkylarkImportFailedException(
+ String.format("Unable to find package for %s: %s.", file, reason));
+ }
return new SkylarkImportFailedException(
String.format("Every .bzl file must have a corresponding package, but '%s' "
+ "does not have one. Please create a BUILD file in the same or any parent directory."
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 f857159..07bb6c4 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
@@ -164,6 +164,6 @@
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");
+ .contains("The repository '@a_remote_repo' could not be resolved");
}
}
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 18bb0fb..1739a30 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
@@ -38,8 +38,11 @@
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.ContainingPackageLookupValue.ContainingPackage;
+import com.google.devtools.build.lib.skyframe.ContainingPackageLookupValue.NoContainingPackage;
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.ErrorReason;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
@@ -185,6 +188,18 @@
.get(key);
}
+ private PackageLookupValue lookupPackage(PackageIdentifier packageIdentifier)
+ throws InterruptedException {
+ SkyKey key = PackageLookupValue.key(packageIdentifier);
+ EvaluationContext evaluationContext =
+ EvaluationContext.newBuilder()
+ .setKeepGoing(false)
+ .setNumThreads(SkyframeExecutor.DEFAULT_THREAD_COUNT)
+ .setEventHander(NullEventHandler.INSTANCE)
+ .build();
+ return driver.<PackageLookupValue>evaluate(ImmutableList.of(key), evaluationContext).get(key);
+ }
+
@Test
public void testNoContainingPackage() throws Exception {
ContainingPackageLookupValue value = lookupContainingPackage("a/b");
@@ -265,4 +280,51 @@
.addEqualityGroup(valueCOther)
.testEquals();
}
+
+ @Test
+ public void testNonExistentExternalRepositoryErrorReason() throws Exception {
+ PackageIdentifier identifier =
+ PackageIdentifier.create("@some_repo", PathFragment.create(":atarget"));
+ ContainingPackageLookupValue value = lookupContainingPackage(identifier);
+ assertThat(value.hasContainingPackage()).isFalse();
+ assertThat(value.getClass()).isEqualTo(NoContainingPackage.class);
+ assertThat(value.getReasonForNoContainingPackage())
+ .isEqualTo("The repository '@some_repo' could not be resolved");
+ }
+
+ @Test
+ public void testInvalidPackageLabelErrorReason() throws Exception {
+ ContainingPackageLookupValue value = lookupContainingPackage("invalidpackagename:42/BUILD");
+ assertThat(value.hasContainingPackage()).isFalse();
+ assertThat(value.getClass()).isEqualTo(NoContainingPackage.class);
+ // As for invalid package name we continue to climb up the parent packages,
+ // we will find the top-level package with the path "" - empty string.
+ assertThat(value.getReasonForNoContainingPackage()).isNull();
+ }
+
+ @Test
+ public void testDeletedPackageErrorReason() throws Exception {
+ PackageIdentifier identifier = PackageIdentifier.createInMainRepo("deletedpackage");
+ deletedPackages.set(ImmutableSet.of(identifier));
+ scratch.file("BUILD");
+
+ PackageLookupValue packageLookupValue = lookupPackage(identifier);
+ assertThat(packageLookupValue.packageExists()).isFalse();
+ assertThat(packageLookupValue.getErrorReason()).isEqualTo(ErrorReason.DELETED_PACKAGE);
+ assertThat(packageLookupValue.getErrorMsg())
+ .isEqualTo("Package is considered deleted due to --deleted_packages");
+
+ ContainingPackageLookupValue value = lookupContainingPackage(identifier);
+ assertThat(value.hasContainingPackage()).isTrue();
+ assertThat(value.getContainingPackageName().toString()).isEmpty();
+ assertThat(value.getClass()).isEqualTo(ContainingPackage.class);
+ }
+
+ @Test
+ public void testNoBuildFileErrorReason() throws Exception {
+ ContainingPackageLookupValue value = lookupContainingPackage("abc");
+ assertThat(value.hasContainingPackage()).isFalse();
+ assertThat(value.getClass()).isEqualTo(NoContainingPackage.class);
+ assertThat(value.getReasonForNoContainingPackage()).isNull();
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
index 7bfcddc..c05b615 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
@@ -415,6 +415,31 @@
}
@Test
+ public void testWithNonExistentRepository_And_DisallowLoadUsingLabelThatCrossesBoundaryOfPackage()
+ throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries");
+
+ scratch.file("BUILD", "load(\"@repository//dir:file.bzl\", \"foo\")");
+
+ SkyKey skylarkImportLookupKey = key("@repository//dir:file.bzl");
+ EvaluationResult<com.google.devtools.build.lib.skyframe.SkylarkImportLookupValue> result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter);
+ assertThat(result.hasError()).isTrue();
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .isInstanceOf(SkylarkImportFailedException.class);
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains(
+ "Unable to find package for @repository//dir:file.bzl: The repository '@repository' "
+ + "could not be resolved.");
+ }
+
+ @Test
public void testLoadBzlFileFromWorkspaceWithRemapping() throws Exception {
scratch.deleteFile(preludeLabelRelativePath);
Path p =
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 be0ff46..4ba3f7b 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
@@ -91,7 +91,7 @@
checkError(
"test/skylark",
"the_rule",
- "no such package '@r//': The repository could not be resolved",
+ "no such package '@r//': The repository '@r' could not be resolved",
"load('//test/skylark:extension.bzl', '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 5c6a97f..abb6303 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 could not be resolved"
+ expect_log "no such package '@bar//': The repository '@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 could not be resolved"
+ expect_log "no such package '@bar//subbar': The repository '@bar' could not be resolved"
}
# Test for https://github.com/bazelbuild/bazel/issues/2580
