diff options
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" |