Make repository-local labels in visibility declarations actually be repository-local.

Fixes #765.

--
MOS_MIGRATED_REVID=112027627

6 files changed, 53 insertions, 35 deletions

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 70b4183d75..8fe14e780c 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
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.cmdline;
 
 import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -43,6 +44,18 @@ import java.io.Serializable;
 public final class Label implements Comparable<Label>, Serializable, SkylarkPrintableValue {
   public static final PathFragment EXTERNAL_PACKAGE_NAME = new PathFragment("external");
 
+  /**
+   * Package names that aren't made relative to the current repository because they mean special
+   * things to Bazel.
+   */
+  private static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES = ImmutableSet.of(
+      // dependencies that are a function of the configuration
+      new PathFragment("tools/defaults"),
+      // Visibility is labels aren't actually targets
+      new PathFragment("visibility"),
+      // There is only one //external package
+      Label.EXTERNAL_PACKAGE_NAME);
+
   public static final PackageIdentifier EXTERNAL_PACKAGE_IDENTIFIER =
       PackageIdentifier.createInDefaultRepo(EXTERNAL_PACKAGE_NAME);
 
@@ -386,7 +399,7 @@ public final class Label implements Comparable<Label>, Serializable, SkylarkPrin
 
     if (packageIdentifier.getRepository().isDefault()
         || !relative.packageIdentifier.getRepository().isDefault()
-        || relative.packageIdentifier.getPackageFragment().equals(EXTERNAL_PACKAGE_NAME)) {
+        || ABSOLUTE_PACKAGE_NAMES.contains(relative.getPackageIdentifier().getPackageFragment())) {
       return relative;
     } else {
       try {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
index 2c1837a836..39c3183bed 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
@@ -15,12 +15,10 @@ package com.google.devtools.build.lib.packages;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.BuildType.SelectorList;
 import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.vfs.PathFragment;
 
 import javax.annotation.Nullable;
 
@@ -32,19 +30,6 @@ import javax.annotation.Nullable;
  * data before exposing it to consumers.
  */
 public abstract class AbstractAttributeMapper implements AttributeMap {
-
-  /**
-   * Package names that aren't made relative to the current repository because they mean special
-   * things to Bazel.
-   */
-  private static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES = ImmutableSet.of(
-      // dependencies that are a function of the configuration
-      new PathFragment("tools/defaults"),
-      // Visibility is labels aren't actually targets
-      new PathFragment("visibility"),
-      // There is only one //external package
-      Label.EXTERNAL_PACKAGE_NAME);
-
   private final Package pkg;
   private final RuleClass ruleClass;
   private final Label ruleLabel;
@@ -172,13 +157,7 @@ public abstract class AbstractAttributeMapper implements AttributeMap {
     Object value = get(attribute.getName(), type);
     if (value != null) { // null values are particularly possible for computed defaults.
       for (Label label : extractLabels(type, value)) {
-        Label absoluteLabel;
-        if (label.getPackageIdentifier().getRepository().isDefault()
-            && ABSOLUTE_PACKAGE_NAMES.contains(label.getPackageIdentifier().getPackageFragment())) {
-          absoluteLabel = label;
-        } else {
-          absoluteLabel = ruleLabel.resolveRepositoryRelative(label);
-        }
+        Label absoluteLabel = ruleLabel.resolveRepositoryRelative(label);
         observer.acceptLabelAttribute(absoluteLabel, attribute);
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 436a631027..ba9111c375 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -102,7 +102,7 @@ public final class PackageFactory {
 
     private void convertAndProcess(
         Package.LegacyBuilder pkgBuilder, Location location, Object value)
-        throws EvalException, ConversionException {
+        throws EvalException {
       T typedValue = type.convert(value, "'package' argument", pkgBuilder.getBuildFileLabel());
       process(pkgBuilder, location, typedValue);
     }
@@ -180,7 +180,7 @@ public final class PackageFactory {
     @Override
     protected void process(Package.LegacyBuilder pkgBuilder, Location location,
         List<Label> value) {
-      pkgBuilder.setDefaultVisibility(getVisibility(value));
+      pkgBuilder.setDefaultVisibility(getVisibility(pkgBuilder.getBuildFileLabel(), value));
     }
   }
 
@@ -698,7 +698,7 @@ public final class PackageFactory {
 
     RuleVisibility visibility = EvalUtils.isNullOrNone(visibilityO)
         ? ConstantRuleVisibility.PUBLIC
-        : getVisibility(BuildType.LABEL_LIST.convert(
+        : getVisibility(pkgBuilder.getBuildFileLabel(), BuildType.LABEL_LIST.convert(
               visibilityO,
               "'exports_files' operand",
               pkgBuilder.getBuildFileLabel()));
@@ -848,7 +848,7 @@ public final class PackageFactory {
     }
   }
 
-  public static RuleVisibility getVisibility(List<Label> original) {
+  public static RuleVisibility getVisibility(Label ruleLabel, List<Label> original) {
     RuleVisibility result;
 
     result = ConstantRuleVisibility.tryParse(original);
@@ -856,7 +856,7 @@ public final class PackageFactory {
       return result;
     }
 
-    result = PackageGroupsRuleVisibility.tryParse(original);
+    result = PackageGroupsRuleVisibility.tryParse(ruleLabel, original);
     return result;
   }
 
@@ -882,7 +882,7 @@ public final class PackageFactory {
     return new BaseFunction("package", FunctionSignature.namedOnly(0, argumentNames)) {
       @Override
       public Object call(Object[] arguments, FuncallExpression ast, Environment env)
-          throws EvalException, ConversionException {
+          throws EvalException {
 
         Package.LegacyBuilder pkgBuilder = getContext(env, ast).pkgBuilder;
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
index ae6e2dc267..c5eb1db615 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
@@ -32,17 +32,18 @@ public class PackageGroupsRuleVisibility implements RuleVisibility {
   private final List<PackageSpecification> directPackages;
   private final List<Label> declaredLabels;
 
-  public PackageGroupsRuleVisibility(List<Label> labels) {
+  public PackageGroupsRuleVisibility(Label ruleLabel, List<Label> labels) {
     declaredLabels = ImmutableList.copyOf(labels);
     ImmutableList.Builder<PackageSpecification> directPackageBuilder = ImmutableList.builder();
     ImmutableList.Builder<Label> packageGroupBuilder = ImmutableList.builder();
 
     for (Label label : labels) {
-      PackageSpecification specification = PackageSpecification.fromLabel(label);
+      Label resolved = ruleLabel.resolveRepositoryRelative(label);
+      PackageSpecification specification = PackageSpecification.fromLabel(resolved);
       if (specification != null) {
         directPackageBuilder.add(specification);
       } else {
-        packageGroupBuilder.add(label);
+        packageGroupBuilder.add(resolved);
       }
     }
 
@@ -75,7 +76,7 @@ public class PackageGroupsRuleVisibility implements RuleVisibility {
    * @return The resulting visibility object. A list of labels can always be
    * parsed into a PackageGroupsRuleVisibility.
    */
-  public static PackageGroupsRuleVisibility tryParse(List<Label> labels) {
-    return new PackageGroupsRuleVisibility(labels);
+  public static PackageGroupsRuleVisibility tryParse(Label ruleLabel, List<Label> labels) {
+    return new PackageGroupsRuleVisibility(ruleLabel, labels);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 3f8c36df67..1b07ae0bba 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1578,7 +1578,7 @@ public final class RuleClass {
         rule.reportError(rule.getLabel() + ": //visibility:legacy_public only allowed in package "
             + "declaration", eventHandler);
       }
-      rule.setVisibility(PackageFactory.getVisibility(attrList));
+      rule.setVisibility(PackageFactory.getVisibility(rule.getLabel(), attrList));
     }
 
     rule.setAttributeValue(attr, converted, /*explicit=*/true);
diff --git a/src/test/shell/bazel/external_correctness_test.sh b/src/test/shell/bazel/external_correctness_test.sh
index 25a8f6e53f..83d422e52f 100755
--- a/src/test/shell/bazel/external_correctness_test.sh
+++ b/src/test/shell/bazel/external_correctness_test.sh
@@ -179,4 +179,29 @@ EOF
   assert_contains 1.0 bazel-genfiles/external/remote2/x.out
 }
 
+function test_visibility_in_external_repo() {
+  REMOTE=$TEST_TMPDIR/r
+  mkdir -p $REMOTE/v
+
+  cat > $REMOTE/BUILD <<EOF
+package(default_visibility=["//v:v"])
+filegroup(name='fg1')  # Inherits default visibility
+filegroup(name='fg2', visibility=["//v:v"])
+EOF
+
+  cat > $REMOTE/v/BUILD <<EOF
+package_group(name="v", packages=["//"])
+EOF
+
+  cat > WORKSPACE <<EOF
+local_repository(name = "r", path = "$REMOTE")
+EOF
+
+  cat > BUILD <<EOF
+filegroup(name = "fg", srcs=["@r//:fg1", "@r//:fg2"])
+EOF
+
+  bazel build //:fg || fail "Build failed"
+}
+
 run_suite "//external correctness tests"