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
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index cc99514..0282489 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -1215,7 +1215,7 @@
// Check visibility attribute
for (PackageSpecification specification :
prerequisite.getProvider(VisibilityProvider.class).getVisibility()) {
- if (specification.containsPackage(rule.getLabel().getPackageFragment())) {
+ if (specification.containsPackage(rule.getLabel().getPackageIdentifier())) {
return true;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
index 64851d6..13f795e 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -301,7 +301,7 @@
repo = input.substring(0, packageStartPos);
packageName = input.substring(packageStartPos + 2);
} else if (input.startsWith("@")) {
- throw new LabelSyntaxException("invalid package name '" + input + "'");
+ throw new LabelSyntaxException("starts with a '@' but does not contain '//'");
} else if (packageStartPos == 0) {
repo = PackageIdentifier.DEFAULT_REPOSITORY;
packageName = input.substring(2);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
index 3695e36..67ea4ed 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
@@ -51,7 +51,7 @@
for (String containedPackage : packages) {
PackageSpecification specification = null;
try {
- specification = PackageSpecification.fromString(containedPackage);
+ specification = PackageSpecification.fromString(label, containedPackage);
} catch (PackageSpecification.InvalidPackageSpecificationException e) {
containsErrors = true;
eventHandler.handle(Event.error(location, e.getMessage()));
@@ -74,7 +74,7 @@
public boolean contains(Package pkg) {
for (PackageSpecification specification : packageSpecifications) {
- if (specification.containsPackage(pkg.getNameFragment())) {
+ if (specification.containsPackage(pkg.getPackageIdentifier())) {
return true;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
index c81123a..fb2073e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
@@ -13,8 +13,10 @@
// limitations under the License.
package com.google.devtools.build.lib.packages;
+import com.google.common.base.Verify;
import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelValidator;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.vfs.PathFragment;
/**
@@ -24,10 +26,14 @@
private static final String PACKAGE_LABEL = "__pkg__";
private static final String SUBTREE_LABEL = "__subpackages__";
private static final String ALL_BENEATH_SUFFIX = "/...";
- public static final PackageSpecification EVERYTHING =
- new AllPackagesBeneath(new PathFragment(""));
+ public static final PackageSpecification EVERYTHING = new PackageSpecification() {
+ @Override
+ public boolean containsPackage(PackageIdentifier packageName) {
+ return true;
+ }
+ };
- public abstract boolean containsPackage(PathFragment packageName);
+ public abstract boolean containsPackage(PackageIdentifier packageName);
@Override
public int hashCode() {
@@ -53,39 +59,39 @@
*
* <p>Note that these strings are different from what {@link #fromLabel} understands.
*/
- public static PackageSpecification fromString(final String spec)
+ public static PackageSpecification fromString(Label context, final String spec)
throws InvalidPackageSpecificationException {
String result = spec;
boolean allBeneath = false;
-
- if (result.startsWith("//")) {
- result = spec.substring(2);
- } else {
- throw new InvalidPackageSpecificationException("invalid package label: " + spec);
- }
-
- if (result.indexOf(':') >= 0) {
- throw new InvalidPackageSpecificationException("invalid package label: " + spec);
- }
-
- if (result.equals("...")) {
- // Special case: //... will not end in /...
- return EVERYTHING;
- }
-
if (result.endsWith(ALL_BENEATH_SUFFIX)) {
allBeneath = true;
result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length());
+ if (result.equals("/")) {
+ // Special case: //... will not end in /...
+ return EVERYTHING;
+ }
}
- String errorMessage = LabelValidator.validatePackageName(result);
- if (errorMessage == null) {
- return allBeneath ?
- new AllPackagesBeneath(new PathFragment(result)) :
- new SinglePackage(new PathFragment(result));
- } else {
- throw new InvalidPackageSpecificationException(errorMessage);
+ if (!spec.startsWith("//")) {
+ throw new InvalidPackageSpecificationException("invalid package name '" + spec
+ + "': must start with '//'");
}
+
+ PackageIdentifier packageId;
+ try {
+ packageId = PackageIdentifier.parse(result);
+ } catch (LabelSyntaxException e) {
+ throw new InvalidPackageSpecificationException(
+ "invalid package name '" + spec + "': " + e.getMessage());
+ }
+
+ Verify.verify(packageId.getRepository().isDefault());
+ packageId = PackageIdentifier.create(
+ context.getPackageIdentifier().getRepository(), packageId.getPackageFragment());
+
+ return allBeneath ?
+ new AllPackagesBeneath(packageId) :
+ new SinglePackage(packageId);
}
/**
@@ -95,23 +101,23 @@
*/
public static PackageSpecification fromLabel(Label label) {
if (label.getName().equals(PACKAGE_LABEL)) {
- return new SinglePackage(label.getPackageFragment());
+ return new SinglePackage(label.getPackageIdentifier());
} else if (label.getName().equals(SUBTREE_LABEL)) {
- return new AllPackagesBeneath(label.getPackageFragment());
+ return new AllPackagesBeneath(label.getPackageIdentifier());
} else {
return null;
}
}
private static class SinglePackage extends PackageSpecification {
- private PathFragment singlePackageName;
+ private PackageIdentifier singlePackageName;
- public SinglePackage(PathFragment packageName) {
+ public SinglePackage(PackageIdentifier packageName) {
this.singlePackageName = packageName;
}
@Override
- public boolean containsPackage(PathFragment packageName) {
+ public boolean containsPackage(PackageIdentifier packageName) {
return this.singlePackageName.equals(packageName);
}
@@ -122,15 +128,16 @@
}
private static class AllPackagesBeneath extends PackageSpecification {
- private PathFragment prefix;
+ private PackageIdentifier prefix;
- public AllPackagesBeneath(PathFragment prefix) {
+ public AllPackagesBeneath(PackageIdentifier prefix) {
this.prefix = prefix;
}
@Override
- public boolean containsPackage(PathFragment packageName) {
- return packageName.startsWith(prefix);
+ public boolean containsPackage(PackageIdentifier packageName) {
+ return packageName.getRepository().equals(prefix.getRepository())
+ && packageName.getPackageFragment().startsWith(prefix.getPackageFragment());
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java
index 92be032..427ea84 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java
@@ -29,7 +29,7 @@
@Override
public boolean contains(Target target) {
- return packageSpecification.containsPackage(target.getPackage().getNameFragment());
+ return packageSpecification.containsPackage(target.getLabel().getPackageIdentifier());
}
@Override
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 c615b9d..d8d799a 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 @@
@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 c527a10..8f993ab 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 @@
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 @@
@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 f5c540a..24002f2 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 @@
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 @@
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 @@
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 83d422e..5d0874c 100755
--- a/src/test/shell/bazel/external_correctness_test.sh
+++ b/src/test/shell/bazel/external_correctness_test.sh
@@ -144,6 +144,30 @@
"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 @@
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 @@
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"
