diff options
4 files changed, 117 insertions, 8 deletions
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 c0a115ad6c..09fd17d974 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 @@ -229,9 +229,13 @@ public class PackageLookupFunction implements SkyFunction { 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()); + // The correct package path is the one originally given, minus the part that is the local + // repository. + PathFragment packagePathUnderExecRoot = packageIdentifier.getPathUnderExecRoot(); + PathFragment remainingPath = packagePathUnderExecRoot.relativeTo(localRepository.getPath()); + PackageIdentifier correctPackage = + PackageIdentifier.create(localRepository.getRepository(), remainingPath); + return PackageLookupValue.incorrectRepositoryReference(packageIdentifier, correctPackage); } // There's no local repository, keep going. 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 875edb041c..c1f9ba2a9b 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 @@ -117,6 +117,11 @@ public abstract class PackageLookupValue implements SkyValue { return new InvalidNamePackageLookupValue(errorMsg); } + public static PackageLookupValue incorrectRepositoryReference( + PackageIdentifier invalidPackage, PackageIdentifier correctPackage) { + return new IncorrectRepositoryReferencePackageLookupValue(invalidPackage, correctPackage); + } + /** * For a successful package lookup, returns the root (package path entry) that the package * resides in. @@ -280,6 +285,59 @@ public abstract class PackageLookupValue implements SkyValue { } } + /** Value indicating the package name was in error. */ + public static class IncorrectRepositoryReferencePackageLookupValue + extends UnsuccessfulPackageLookupValue { + + private final PackageIdentifier invalidPackageIdentifier; + private final PackageIdentifier correctedPackageIdentifier; + + private IncorrectRepositoryReferencePackageLookupValue( + PackageIdentifier invalidPackageIdentifier, PackageIdentifier correctedPackageIdentifier) { + this.invalidPackageIdentifier = invalidPackageIdentifier; + this.correctedPackageIdentifier = correctedPackageIdentifier; + } + + public PackageIdentifier getInvalidPackageIdentifier() { + return invalidPackageIdentifier; + } + + public PackageIdentifier getCorrectedPackageIdentifier() { + return correctedPackageIdentifier; + } + + @Override + ErrorReason getErrorReason() { + return ErrorReason.INVALID_PACKAGE_NAME; + } + + @Override + public String getErrorMsg() { + return String.format( + "Invalid package reference %s crosses into repository %s:" + + " did you mean to use %s instead?", + invalidPackageIdentifier, + correctedPackageIdentifier.getRepository(), + correctedPackageIdentifier); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof IncorrectRepositoryReferencePackageLookupValue)) { + return false; + } + IncorrectRepositoryReferencePackageLookupValue other = + (IncorrectRepositoryReferencePackageLookupValue) obj; + return Objects.equal(invalidPackageIdentifier, other.invalidPackageIdentifier) + && Objects.equal(correctedPackageIdentifier, other.correctedPackageIdentifier); + } + + @Override + public int hashCode() { + return Objects.hashCode(invalidPackageIdentifier, correctedPackageIdentifier); + } + } + /** Marker value for a deleted package. */ public static class DeletedPackageLookupValue extends UnsuccessfulPackageLookupValue { 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 7e770ffe81..5404d8e5ef 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 @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAc 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.skyframe.PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue; 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; @@ -381,6 +382,52 @@ public abstract class PackageLookupFunctionTest extends FoundationTestCase { createAndCheckInvalidPackageLabel(false); } + private String getCorrectedPackage(String repository, String directory) throws Exception { + scratch.overwriteFile( + "WORKSPACE", "local_repository(name='local', path='" + repository + "')"); + scratch.file(repository + "/WORKSPACE"); + scratch.file(directory + "/BUILD"); + + PackageLookupValue packageLookupValue = + lookupPackage(PackageIdentifier.createInMainRepo(directory)); + assertThat(packageLookupValue.packageExists()).isFalse(); + assertThat(packageLookupValue) + .isInstanceOf(IncorrectRepositoryReferencePackageLookupValue.class); + + IncorrectRepositoryReferencePackageLookupValue incorrectPackageLookupValue = + (IncorrectRepositoryReferencePackageLookupValue) packageLookupValue; + assertThat(incorrectPackageLookupValue.getInvalidPackageIdentifier()) + .isEqualTo(PackageIdentifier.createInMainRepo(directory)); + return incorrectPackageLookupValue.getCorrectedPackageIdentifier().toString(); + } + + @Test + public void testCorrectPackageDetection_simpleRepo_emptyPackage() throws Exception { + assertThat(getCorrectedPackage("local", "local")).isEqualTo("@local//"); + } + + @Test + public void testCorrectPackageDetection_simpleRepo_singlePackage() throws Exception { + assertThat(getCorrectedPackage("local", "local/package")).isEqualTo("@local//package"); + } + + @Test + public void testCorrectPackageDetection_simpleRepo_subPackage() throws Exception { + assertThat(getCorrectedPackage("local", "local/package/subpackage")) + .isEqualTo("@local//package/subpackage"); + } + + @Test + public void testCorrectPackageDetection_deepRepo_emptyPackage() throws Exception { + assertThat(getCorrectedPackage("local/repo", "local/repo")).isEqualTo("@local//"); + } + + @Test + public void testCorrectPackageDetection_deepRepo_subPackage() throws Exception { + assertThat(getCorrectedPackage("local/repo", "local/repo/package")) + .isEqualTo("@local//package"); + } + @Test public void testSymlinkCycleInWorkspace() throws Exception { scratch.overwriteFile("WORKSPACE", "local_repository(name='local', path='local/repo')"); diff --git a/src/test/shell/bazel/cross_repository_test.sh b/src/test/shell/bazel/cross_repository_test.sh index 9fad3c5232..5c6a97f2ca 100755 --- a/src/test/shell/bazel/cross_repository_test.sh +++ b/src/test/shell/bazel/cross_repository_test.sh @@ -72,9 +72,9 @@ EOF # 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" + expect_log "no such package 'bar': Invalid package reference bar 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" + expect_log "no such package 'bar/subbar': Invalid package reference bar/subbar crosses into repository @bar" } function test_top_level_local_repository { @@ -121,7 +121,7 @@ EOF # 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" + expect_log "no such package 'bar': Invalid package reference bar crosses into repository @bar" } # Load rules via an invalid label. @@ -188,9 +188,9 @@ EOF # 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" + expect_log "no such package 'bar': Invalid package reference bar 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" + expect_log "no such package 'bar/subbar': Invalid package reference bar/subbar crosses into repository @bar" # These should succeed. echo "about to test @bar//" |