Make repository-local labels in visibility declarations actually be repository-local.
Fixes #765.
--
MOS_MIGRATED_REVID=112027627
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 70b4183..8fe14e7 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 @@
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 @@
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 2c1837a..39c3183 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 @@
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 @@
* 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 @@
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 436a631..ba9111c 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 @@
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 @@
@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 @@
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 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 @@
return result;
}
- result = PackageGroupsRuleVisibility.tryParse(original);
+ result = PackageGroupsRuleVisibility.tryParse(ruleLabel, original);
return result;
}
@@ -882,7 +882,7 @@
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 ae6e2dc..c5eb1db 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 @@
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 @@
* @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 3f8c36d..1b07ae0 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 @@
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 25a8f6e..83d422e 100755
--- a/src/test/shell/bazel/external_correctness_test.sh
+++ b/src/test/shell/bazel/external_correctness_test.sh
@@ -179,4 +179,29 @@
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"
