From b6fbab7115c1567705e7bddc7b79bdee6313fce3 Mon Sep 17 00:00:00 2001
From: Kristina Chodorow <kchodorow@google.com>
Date: Thu, 28 Jan 2016 14:38:31 +0000
Subject: Resolve repository-relative labels within the current repository

Using $(location //foo) from an external repository was resolving to @//foo, not
@repo//foo, which generally wouldn't be in the main repository. This may also
fix other cases where getRelative was resolving incorrectly.

Fixes #819.

--
MOS_MIGRATED_REVID=113256854
---
 .../google/devtools/build/lib/cmdline/Label.java   | 26 +++---
 .../devtools/build/lib/cmdline/RepositoryName.java |  2 +-
 .../devtools/build/lib/packages/Package.java       |  3 +-
 .../devtools/build/lib/cmdline/LabelTest.java      | 92 +++++++++++++++++++---
 src/test/shell/bazel/bazel_rules_test.sh           | 31 ++++++++
 5 files changed, 125 insertions(+), 29 deletions(-)

(limited to 'src')

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
index 7e1046f8a1..b1bcffa482 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -49,6 +49,8 @@ public final class Label implements Comparable<Label>, Serializable, SkylarkPrin
    * things to Bazel.
    */
   private static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES = ImmutableSet.of(
+      // Used for select
+      new PathFragment("conditions"),
       // dependencies that are a function of the configuration
       new PathFragment("tools/defaults"),
       // Visibility is labels aren't actually targets
@@ -370,18 +372,26 @@ public final class Label implements Comparable<Label>, Serializable, SkylarkPrin
    */
   @SkylarkCallable(name = "relative", doc =
         "Resolves a label that is either absolute (starts with <code>//</code>) or relative to the"
-      + " current package.<br>"
+      + " current package. If this label is in a remote repository, the argument will be resolved "
+      + "relative to that repository. If the argument contains a repository, it will be returned "
+      + "as-is. Reserved labels will also be returned as-is.<br>"
       + "For example:<br>"
       + "<pre class=language-python>\n"
       + "Label(\"//foo/bar:baz\").relative(\":quux\") == Label(\"//foo/bar:quux\")\n"
       + "Label(\"//foo/bar:baz\").relative(\"//wiz:quux\") == Label(\"//wiz:quux\")\n"
+      + "Label(\"@repo//foo/bar:baz\").relative(\"//wiz:quux\") == Label(\"@repo//wiz:quux\")\n"
+      + "Label(\"@repo//foo/bar:baz\").relative(\"//visibility:public\") == "
+      + "Label(\"//visibility:public\")\n"
+      + "Label(\"@repo//foo/bar:baz\").relative(\"@other//wiz:quux\") == "
+      + "Label(\"@other//wiz:quux\")\n"
       + "</pre>")
   public Label getRelative(String relName) throws LabelSyntaxException {
     if (relName.length() == 0) {
       throw new LabelSyntaxException("empty package-relative label");
     }
+
     if (LabelValidator.isAbsolute(relName)) {
-      return parseAbsolute(relName);
+      return resolveRepositoryRelative(parseAbsolute(relName));
     } else if (relName.equals(":")) {
       throw new LabelSyntaxException("':' is not a valid package-relative label");
     } else if (relName.charAt(0) == ':') {
@@ -399,18 +409,6 @@ public final class Label implements Comparable<Label>, Serializable, SkylarkPrin
    * repository would point back to the main repository, which is usually not what is intended.
    */
   public Label resolveRepositoryRelative(Label relative) {
-    if (relative.packageIdentifier.getRepository().getName().equals("@")) {
-      try {
-        return new Label(
-            PackageIdentifier.create(
-                PackageIdentifier.DEFAULT_REPOSITORY_NAME,
-                relative.packageIdentifier.getPackageFragment()),
-            relative.getName());
-      } catch (LabelSyntaxException e) {
-        throw new IllegalStateException(e);
-      }
-    }
-
     if (packageIdentifier.getRepository().isDefault()
         || !relative.packageIdentifier.getRepository().isDefault()
         || ABSOLUTE_PACKAGE_NAMES.contains(relative.getPackageIdentifier().getPackageFragment())) {
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
index 2aa253dcb1..6f5811d58a 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -189,7 +189,7 @@ public final class RepositoryName implements Serializable {
    * Returns the path at which this repository is mapped within the exec root.
    */
   public PathFragment getPathFragment() {
-    return isDefault()
+    return isDefault() || this.equals(PackageIdentifier.MAIN_REPOSITORY_NAME)
         ? PathFragment.EMPTY_FRAGMENT
         : new PathFragment(Label.EXTERNAL_PATH_PREFIX).getRelative(strippedName());
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 51a496dc7a..1cfb443936 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -267,7 +267,8 @@ public class Package {
 
   private static Path getSourceRoot(Path buildFile, PathFragment packageFragment) {
     Path current = buildFile.getParentDirectory();
-    for (int i = 0, len = packageFragment.segmentCount(); i < len && current != null; i++) {
+    for (int i = 0, len = packageFragment.segmentCount();
+         i < len && !packageFragment.equals(PathFragment.EMPTY_FRAGMENT); i++) {
       current = current.getParentDirectory();
     }
     return current;
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
index 1f3ce8b212..9837861a56 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
@@ -93,19 +93,24 @@ public class LabelTest {
   }
 
   @Test
-  public void testGetRelative() throws Exception {
-    Label base = Label
-        .parseAbsolute("//foo/bar:baz");
-    {
-      Label l = base.getRelative("//p1/p2:target");
-      assertEquals("p1/p2", l.getPackageName());
-      assertEquals("target", l.getName());
-    }
-    {
-      Label l = base.getRelative(":quux");
-      assertEquals("foo/bar", l.getPackageName());
-      assertEquals("quux", l.getName());
-    }
+  public void testGetRelativeWithAbsoluteLabel() throws Exception {
+    Label base = Label.parseAbsolute("//foo/bar:baz");
+    Label l = base.getRelative("//p1/p2:target");
+    assertEquals("p1/p2", l.getPackageName());
+    assertEquals("target", l.getName());
+  }
+
+  @Test
+  public void testGetRelativeWithRelativeLabel() throws Exception {
+    Label base = Label.parseAbsolute("//foo/bar:baz");
+    Label l = base.getRelative(":quux");
+    assertEquals("foo/bar", l.getPackageName());
+    assertEquals("quux", l.getName());
+  }
+
+  @Test
+  public void testGetRelativeWithIllegalLabel() throws Exception {
+    Label base = Label.parseAbsolute("//foo/bar:baz");
     try {
       base.getRelative("/p1/p2:target");
       fail();
@@ -132,6 +137,67 @@ public class LabelTest {
     }
   }
 
+  @Test
+  public void testGetRelativeWithDifferentRepo() throws Exception {
+    PackageIdentifier packageId = PackageIdentifier.create("@repo", new PathFragment("foo"));
+    Label base = Label.create(packageId, "bar");
+
+    Label relative = base.getRelative("@remote//x:y");
+
+    assertEquals(RepositoryName.create("@remote"), relative.getPackageIdentifier().getRepository());
+    assertEquals(new PathFragment("x"), relative.getPackageFragment());
+    assertEquals("y", relative.getName());
+  }
+
+  @Test
+  public void testGetRelativeWithRepoLocalAbsoluteLabel() throws Exception {
+    PackageIdentifier packageId = PackageIdentifier.create("@repo", new PathFragment("foo"));
+    Label base = Label.create(packageId, "bar");
+
+    Label relative = base.getRelative("//x:y");
+
+    assertEquals(packageId.getRepository(), relative.getPackageIdentifier().getRepository());
+    assertEquals(new PathFragment("x"), relative.getPackageFragment());
+    assertEquals("y", relative.getName());
+  }
+
+  @Test
+  public void testGetRelativeWithLocalRepoRelativeLabel() throws Exception {
+    PackageIdentifier packageId = PackageIdentifier.create("@repo", new PathFragment("foo"));
+    Label base = Label.create(packageId, "bar");
+
+    Label relative = base.getRelative(":y");
+
+    assertEquals(packageId.getRepository(), relative.getPackageIdentifier().getRepository());
+    assertEquals(new PathFragment("foo"), relative.getPackageFragment());
+    assertEquals("y", relative.getName());
+  }
+
+  @Test
+  public void testGetRelativeWithRepoAndReservedPackage() throws Exception {
+    PackageIdentifier packageId = PackageIdentifier.create("@repo", new PathFragment("foo"));
+    Label base = Label.create(packageId, "bar");
+
+    Label relative = base.getRelative("//conditions:default");
+
+    PackageIdentifier expected = PackageIdentifier.createInDefaultRepo("conditions");
+    assertEquals(expected.getRepository(), relative.getPackageIdentifier().getRepository());
+    assertEquals(expected.getPackageFragment(), relative.getPackageFragment());
+    assertEquals("default", relative.getName());
+  }
+
+  @Test
+  public void testGetRelativeWithRemoteRepoToDefaultRepo() throws Exception {
+    PackageIdentifier packageId = PackageIdentifier.create("@repo", new PathFragment("foo"));
+    Label base = Label.create(packageId, "bar");
+
+    Label relative = base.getRelative("@//x:y");
+
+    assertEquals(RepositoryName.create("@"), relative.getPackageIdentifier().getRepository());
+    assertEquals(new PathFragment("x"), relative.getPackageFragment());
+    assertEquals("y", relative.getName());
+  }
+
   @Test
   public void testFactory() throws Exception {
     Label l = Label.create("foo/bar", "quux");
diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh
index 2547ef57d2..bb9e5e2a0f 100755
--- a/src/test/shell/bazel/bazel_rules_test.sh
+++ b/src/test/shell/bazel/bazel_rules_test.sh
@@ -243,4 +243,35 @@ EOF
     bazel-genfiles/pkg/test.out
 }
 
+function test_genrule_remote() {
+  cat > WORKSPACE <<EOF
+local_repository(
+    name = "r",
+    path = __workspace_dir__,
+)
+EOF
+  mkdir package
+  cat > package/BUILD <<EOF
+genrule(
+    name = "abs_dep",
+    srcs = ["//package:in"],
+    outs = ["abs_dep.out"],
+    cmd = "echo '\$(locations //package:in)' > \$@",
+)
+
+sh_binary(
+    name = "in",
+    srcs = ["in.sh"],
+)
+EOF
+
+  cat > package/in.sh << EOF
+#!/bin/bash
+echo "Hi"
+EOF
+  chmod +x package/in.sh
+
+  bazel build @r//package:abs_dep >$TEST_log 2>&1 || fail "Should build"
+}
+
 run_suite "rules test"
-- 
cgit v1.2.3