Fix local repository detection when the repository path is absolute.
Fixes #3874.
Change-Id: Ibbe3ea27b77426f551e2f70f082478edb2234749
PiperOrigin-RevId: 171957230
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 09fd17d..3ffcc33 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
@@ -231,8 +231,14 @@
// There is a repository mismatch, this is an error.
// 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());
+ PathFragment pathToRequestedPackage = packageIdentifier.getPathUnderExecRoot();
+ PathFragment localRepositoryPath = localRepository.getPath();
+ if (localRepositoryPath.isAbsolute()) {
+ // We need the package path to also be absolute.
+ pathToRequestedPackage =
+ packagePathEntry.asFragment().getRelative(pathToRequestedPackage);
+ }
+ PathFragment remainingPath = pathToRequestedPackage.relativeTo(localRepositoryPath);
PackageIdentifier correctPackage =
PackageIdentifier.create(localRepository.getRepository(), remainingPath);
return PackageLookupValue.incorrectRepositoryReference(packageIdentifier, correctPackage);
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 c1f9ba2..3922043 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
@@ -283,6 +283,11 @@
public int hashCode() {
return errorMsg.hashCode();
}
+
+ @Override
+ public String toString() {
+ return String.format("%s: %s", this.getClass().getSimpleName(), this.errorMsg);
+ }
}
/** Value indicating the package name was in error. */
@@ -336,6 +341,15 @@
public int hashCode() {
return Objects.hashCode(invalidPackageIdentifier, correctedPackageIdentifier);
}
+
+ @Override
+ public String toString() {
+ return String.format(
+ "%s: invalidPackageIdenfitier: %s, corrected: %s",
+ this.getClass().getSimpleName(),
+ this.invalidPackageIdentifier,
+ this.correctedPackageIdentifier);
+ }
}
/** Marker value for a deleted package. */
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 990e593..9d3d23b 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
@@ -57,7 +57,9 @@
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
+import java.util.ArrayList;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicBoolean;
@@ -66,6 +68,8 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
/** Tests for {@link PackageLookupFunction}. */
public abstract class PackageLookupFunctionTest extends FoundationTestCase {
@@ -384,52 +388,6 @@
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')");
@@ -455,4 +413,113 @@
+ "directory /workspace/local/repo");
}
}
+
+ /** Tests for detection of invalid package identifiers for local repositories. */
+ @RunWith(Parameterized.class)
+ public static class CorrectedLocalRepositoryTest extends PackageLookupFunctionTest {
+
+ /**
+ * Create parameters for this test. The contents are:
+ *
+ * <ol>
+ * <li>description
+ * <li>repository path
+ * <li>package path - under the repository
+ * <li>expected corrected package identifier
+ * </ol>
+ */
+ @Parameters(name = "{0}")
+ public static List<Object[]> parameters() {
+ List<Object[]> params = new ArrayList<>();
+
+ params.add(new String[] {"simpleRepo_emptyPackage", "local", "", "@local//"});
+ params.add(new String[] {"simpleRepo_singlePackage", "local", "package", "@local//package"});
+ params.add(
+ new String[] {
+ "simpleRepo_subPackage", "local", "package/subpackage", "@local//package/subpackage"
+ });
+ params.add(new String[] {"deepRepo_emptyPackage", "local/repo", "", "@local//"});
+ params.add(new String[] {"deepRepo_subPackage", "local/repo", "package", "@local//package"});
+
+ return params;
+ }
+
+ private String description;
+ private String repositoryPath;
+ private String packagePath;
+ private String expectedCorrectedPackageIdentifier;
+
+ public CorrectedLocalRepositoryTest(
+ String description,
+ String repositoryPath,
+ String packagePath,
+ String expectedCorrectedPackageIdentifier) {
+ this.description = description;
+ this.repositoryPath = repositoryPath;
+ this.packagePath = packagePath;
+ this.expectedCorrectedPackageIdentifier = expectedCorrectedPackageIdentifier;
+ }
+
+ @Override
+ protected CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy() {
+ return CrossRepositoryLabelViolationStrategy.ERROR;
+ }
+
+ @Test
+ public void testCorrectPackageDetection_relativePath() throws Exception {
+ String fullPackagePath = packagePath + "/BUILD";
+ scratch.overwriteFile(
+ "WORKSPACE", "local_repository(name='local', path='" + repositoryPath + "')");
+ scratch.file(PathFragment.create(repositoryPath).getRelative("WORKSPACE").getPathString());
+ scratch.file(
+ PathFragment.create(repositoryPath)
+ .getRelative(packagePath)
+ .getRelative("BUILD")
+ .getPathString());
+
+ PackageIdentifier packageIdentifier =
+ PackageIdentifier.createInMainRepo(
+ PathFragment.create(repositoryPath).getRelative(packagePath));
+ PackageLookupValue packageLookupValue = lookupPackage(packageIdentifier);
+ assertThat(packageLookupValue.packageExists()).isFalse();
+ assertThat(packageLookupValue)
+ .isInstanceOf(IncorrectRepositoryReferencePackageLookupValue.class);
+
+ IncorrectRepositoryReferencePackageLookupValue incorrectPackageLookupValue =
+ (IncorrectRepositoryReferencePackageLookupValue) packageLookupValue;
+ assertThat(incorrectPackageLookupValue.getInvalidPackageIdentifier())
+ .isEqualTo(packageIdentifier);
+ assertThat(incorrectPackageLookupValue.getCorrectedPackageIdentifier().toString())
+ .isEqualTo(expectedCorrectedPackageIdentifier);
+ }
+
+ @Test
+ public void testCorrectPackageDetection_absolutePath() throws Exception {
+ String fullPackagePath = packagePath + "/BUILD";
+ scratch.overwriteFile(
+ "WORKSPACE",
+ "local_repository(name='local', path=__workspace_dir__ + '/" + repositoryPath + "')");
+ scratch.file(PathFragment.create(repositoryPath).getRelative("WORKSPACE").getPathString());
+ scratch.file(
+ PathFragment.create(repositoryPath)
+ .getRelative(packagePath)
+ .getRelative("BUILD")
+ .getPathString());
+
+ PackageIdentifier packageIdentifier =
+ PackageIdentifier.createInMainRepo(
+ PathFragment.create(repositoryPath).getRelative(packagePath));
+ PackageLookupValue packageLookupValue = lookupPackage(packageIdentifier);
+ assertThat(packageLookupValue.packageExists()).isFalse();
+ assertThat(packageLookupValue)
+ .isInstanceOf(IncorrectRepositoryReferencePackageLookupValue.class);
+
+ IncorrectRepositoryReferencePackageLookupValue incorrectPackageLookupValue =
+ (IncorrectRepositoryReferencePackageLookupValue) packageLookupValue;
+ assertThat(incorrectPackageLookupValue.getInvalidPackageIdentifier())
+ .isEqualTo(packageIdentifier);
+ assertThat(incorrectPackageLookupValue.getCorrectedPackageIdentifier().toString())
+ .isEqualTo(expectedCorrectedPackageIdentifier);
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/skyframe/ReverseDepsUtilityTest.java b/src/test/java/com/google/devtools/build/skyframe/ReverseDepsUtilityTest.java
index d783795..3a60399 100644
--- a/src/test/java/com/google/devtools/build/skyframe/ReverseDepsUtilityTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/ReverseDepsUtilityTest.java
@@ -33,7 +33,7 @@
private final int numElements;
@Parameters(name = "numElements-{0}")
- public static List<Object[]> paramenters() {
+ public static List<Object[]> parameters() {
List<Object[]> params = new ArrayList<>();
for (int i = 0; i < 20; i++) {
params.add(new Object[] {i});
