Fix local repository detection when the repository path is absolute.

Fixes #3874.

Change-Id: Ibbe3ea27b77426f551e2f70f082478edb2234749
PiperOrigin-RevId: 171957230

4 files changed, 136 insertions, 49 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 09fd17d974..3ffcc33b82 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 @@ public class PackageLookupFunction implements SkyFunction {
         // 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 c1f9ba2a9b..3922043d10 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 abstract class PackageLookupValue implements SkyValue {
     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 abstract class PackageLookupValue implements SkyValue {
     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 990e5935b7..9d3d23b5b0 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.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.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.Before;
 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 @@ 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')");
@@ -455,4 +413,113 @@ public abstract class PackageLookupFunctionTest extends FoundationTestCase {
                   + "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 d783795b2d..3a603998a8 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 @@ public class ReverseDepsUtilityTest {
   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});