Make package names in the package_group.packages attribute refer to the repository where the package group is.

There is currently no way to refer to packages in other repositories and that doesn't seem to be useful, because visibility currently checks the repository name in the label and that can be changed in the main WORKSPACE file. If needed, it'd be pretty easy to implement, though.

As a drive-by fix, made the parsing of the package name call into the same logic implemented in the cmdline package because code duplication is bad, mmmkay?

Fixes #767.

--
MOS_MIGRATED_REVID=112032508

4 files changed, 67 insertions, 10 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index c615b9d372..d8d799afbc 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -811,7 +811,7 @@ public class PackageFactoryTest extends PackageFactoryTestBase {
 
   @Test
   public void testPackageGroupSpecBad() throws Exception {
-    expectEvalError("invalid package label", "package_group(name='skin', packages=['--25:17--'])");
+    expectEvalError("invalid package name", "package_group(name='skin', packages=['--25:17--'])");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
index c527a106f5..8f993ab30b 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.packages;
 
 import static org.junit.Assert.assertFalse;
 
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
 import com.google.devtools.build.lib.packages.util.PackageFactoryApparatus;
 import com.google.devtools.build.lib.testutil.Scratch;
@@ -50,7 +51,8 @@ public class PackageGroupStaticInitializationTest {
               @Override
               public void run() {
                 try {
-                  groupQueue.put(PackageSpecification.fromString("//fruits/..."));
+                  groupQueue.put(PackageSpecification.fromString(
+                      Label.parseAbsoluteUnchecked("//context"), "//fruits/..."));
                 } catch (Exception e) {
                   // Can't throw from Runnable, but this will cause the test to timeout
                   // when the consumer can't take the object.
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
index f5c540a383..24002f2734 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
@@ -81,7 +81,31 @@ public class PackageGroupTest {
 
     events.setFailFast(false);
     getPackageGroup("fruits", "apple");
-    events.assertContainsError("invalid package label: vegetables");
+    events.assertContainsError("invalid package name 'vegetables'");
+  }
+
+  @Test
+  public void testPackagesWithRepositoryDoNotWork() throws Exception {
+    scratch.file(
+        "fruits/BUILD",
+        "package_group(name = 'banana',",
+        "              packages = ['@veggies//:cucumber'])");
+
+    events.setFailFast(false);
+    getPackageGroup("fruits", "banana");
+    events.assertContainsError("invalid package name '@veggies//:cucumber'");
+  }
+
+  @Test
+  public void testAllPackagesInMainRepositoryDoesNotWork() throws Exception {
+    scratch.file(
+        "fruits/BUILD",
+        "package_group(name = 'apple',",
+        "              packages = ['@//...'])");
+
+    events.setFailFast(false);
+    getPackageGroup("fruits", "apple");
+    events.assertContainsError("invalid package name '@//...'");
   }
 
   @Test
@@ -96,7 +120,7 @@ public class PackageGroupTest {
 
     events.setFailFast(false);
     getPackageGroup("fruits", "apple");
-    events.assertContainsError("invalid package label: //vegetables:carrot");
+    events.assertContainsError("invalid package name '//vegetables:carrot'");
   }
 
   @Test
@@ -109,7 +133,7 @@ public class PackageGroupTest {
 
     events.setFailFast(false);
     getPackageGroup("fruits", "apple");
-    events.assertContainsError("invalid package label: :carrot");
+    events.assertContainsError("invalid package name ':carrot'");
   }
 
   @Test
diff --git a/src/test/shell/bazel/external_correctness_test.sh b/src/test/shell/bazel/external_correctness_test.sh
index 83d422e52f..5d0874c7df 100755
--- a/src/test/shell/bazel/external_correctness_test.sh
+++ b/src/test/shell/bazel/external_correctness_test.sh
@@ -144,6 +144,30 @@ EOF
     "bazel-genfiles/external/a/b/c/d"
 }
 
+function test_package_group_in_external_repos() {
+  REMOTE=$TEST_TMPDIR/r
+  mkdir -p $REMOTE/v $REMOTE/a v a
+
+  echo 'filegroup(name="rv", srcs=["//:fg"])' > $REMOTE/v/BUILD
+  echo 'filegroup(name="ra", srcs=["//:fg"])' > $REMOTE/a/BUILD
+  echo 'filegroup(name="mv", srcs=["@r//:fg"])' > v/BUILD
+  echo 'filegroup(name="ma", srcs=["@r//:fg"])' > a/BUILD
+  cat > $REMOTE/BUILD <<EOF
+package_group(name="pg", packages=["//v"])
+filegroup(name="fg", visibility=[":pg"])
+EOF
+
+  echo "local_repository(name='r', path='$REMOTE')" > WORKSPACE
+  bazel build @r//v:rv >& $TEST_log || fail "Build failed"
+  bazel build @r//a:ra >& $TEST_log && fail "Build succeeded"
+  expect_log "Target '@r//:fg' is not visible"
+  bazel build //a:ma >& $TEST_log && fail "Build succeeded"
+  expect_log "Target '@r//:fg' is not visible"
+  bazel build //v:mv >& $TEST_log && fail "Build succeeded"
+  expect_log "Target '@r//:fg' is not visible"
+
+}
+
 # Regression test for #517.
 function test_refs_btwn_repos() {
   REMOTE1=$TEST_TMPDIR/remote1
@@ -179,11 +203,11 @@ EOF
   assert_contains 1.0 bazel-genfiles/external/remote2/x.out
 }
 
-function test_visibility_in_external_repo() {
+function test_visibility_attributes_in_external_repos() {
   REMOTE=$TEST_TMPDIR/r
-  mkdir -p $REMOTE/v
+  mkdir -p $REMOTE/v $REMOTE/r
 
-  cat > $REMOTE/BUILD <<EOF
+  cat > $REMOTE/r/BUILD <<EOF
 package(default_visibility=["//v:v"])
 filegroup(name='fg1')  # Inherits default visibility
 filegroup(name='fg2', visibility=["//v:v"])
@@ -193,15 +217,22 @@ EOF
 package_group(name="v", packages=["//"])
 EOF
 
+  cat >$REMOTE/BUILD <<EOF
+filegroup(name="fg", srcs=["//r:fg1", "//r:fg2"])
+EOF
+
   cat > WORKSPACE <<EOF
 local_repository(name = "r", path = "$REMOTE")
 EOF
 
   cat > BUILD <<EOF
-filegroup(name = "fg", srcs=["@r//:fg1", "@r//:fg2"])
+filegroup(name="fg", srcs=["@r//r:fg1", "@r//r:fg2"])
 EOF
 
-  bazel build //:fg || fail "Build failed"
+  bazel build @r//:fg || fail "Build failed"
+  bazel build //:fg >& $TEST_log && fail "Build succeeded"
+  expect_log "Target '@r//r:fg1' is not visible"
+
 }
 
 run_suite "//external correctness tests"