Make error messages about illegal dependencies involving aliases clearer.
RELNOTES: None.
PiperOrigin-RevId: 190759949
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java
index 3633fd2..c704b83 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AliasProvider.java
@@ -66,12 +66,41 @@
return aliasChain;
}
- public static String printLabelWithAliasChain(ConfiguredTargetAndData target) {
- AliasProvider aliasProvider = target.getConfiguredTarget().getProvider(AliasProvider.class);
- String suffix = aliasProvider == null
- ? ""
- : " (aliased through '" + Joiner.on("' -> '").join(aliasProvider.getAliasChain()) + "')";
+ /** The way {@link #describeTargetWithAliases(ConfiguredTargetAndData, TargetMode) reports the
+ * kind of a target. */
+ public enum TargetMode {
+ WITH_KIND, // Specify the kind of the target
+ WITHOUT_KIND, // Only say "target"
+ }
- return "'" + target.getTarget().getLabel() + "'" + suffix;
+ /**
+ * Prints a nice description of a target.
+ *
+ * Also adds the aliases it was reached through, if any.
+ *
+ * @param target the target to describe
+ * @param targetMode how to express the kind of the target
+ * @return
+ */
+ public static String describeTargetWithAliases(
+ ConfiguredTargetAndData target, TargetMode targetMode) {
+ String kind = targetMode == TargetMode.WITH_KIND
+ ? target.getTarget().getTargetKind() : "target";
+ AliasProvider aliasProvider = target.getConfiguredTarget().getProvider(AliasProvider.class);
+ if (aliasProvider == null) {
+ return kind + " '" + target.getTarget().getLabel() + "'";
+ }
+
+ ImmutableList<Label> aliasChain = aliasProvider.getAliasChain();
+ StringBuilder result = new StringBuilder();
+ result.append("alias '" + aliasChain.get(0) + "'");
+ result.append(" referring to " + kind + " '" + target.getTarget().getLabel() + "'");
+ if (aliasChain.size() > 1) {
+ result.append(" through '"
+ + Joiner.on("' -> '").join(aliasChain.subList(1, aliasChain.size()))
+ + "'");
+ }
+
+ return result.toString();
}
}
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 6bab955..c8c4775 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
@@ -41,6 +41,7 @@
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.PrerequisiteValidator;
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoKey;
@@ -1679,26 +1680,25 @@
}
private String badPrerequisiteMessage(
- String targetKind, ConfiguredTargetAndData prerequisite, String reason, boolean isWarning) {
- String msgPrefix = targetKind != null ? targetKind + " " : "";
+ ConfiguredTargetAndData prerequisite, String reason, boolean isWarning) {
+ String targetKind = prerequisite.getTarget().getTargetKind();
String msgReason = reason != null ? " (" + reason + ")" : "";
if (isWarning) {
return String.format(
- "%s%s is unexpected here%s; continuing anyway",
- msgPrefix, AliasProvider.printLabelWithAliasChain(prerequisite),
+ "%s is unexpected here%s; continuing anyway",
+ AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITH_KIND),
msgReason);
}
- return String.format("%s%s is misplaced here%s",
- msgPrefix, AliasProvider.printLabelWithAliasChain(prerequisite), msgReason);
+ return String.format("%s is misplaced here%s",
+ AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITH_KIND), msgReason);
}
private void reportBadPrerequisite(
Attribute attribute,
- String targetKind,
ConfiguredTargetAndData prerequisite,
String reason,
boolean isWarning) {
- String message = badPrerequisiteMessage(targetKind, prerequisite, reason, isWarning);
+ String message = badPrerequisiteMessage(prerequisite, reason, isWarning);
if (isWarning) {
attributeWarning(attribute.getName(), message);
} else {
@@ -1716,8 +1716,7 @@
String reason = attribute.getValidityPredicate().checkValid(rule, prerequisiteRule);
if (reason != null) {
- reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(),
- prerequisite, reason, false);
+ reportBadPrerequisite(attribute, prerequisite, reason, false);
}
}
@@ -1742,7 +1741,7 @@
}
} else {
// The file exists but has a bad extension
- reportBadPrerequisite(attribute, "file", prerequisite,
+ reportBadPrerequisite(attribute, prerequisite,
"expected " + attribute.getAllowedFileTypesPredicate(), false);
}
}
@@ -1880,7 +1879,6 @@
// but maybe prerequisite provides required providers? do not reject yet.
unfulfilledRequirements.add(
badPrerequisiteMessage(
- prerequisite.getTarget().getTargetKind(),
prerequisite,
"expected " + attribute.getAllowedRuleClassesPredicate(),
false));
@@ -1901,7 +1899,6 @@
Predicate<RuleClass> allowedRuleClasses = attribute.getAllowedRuleClassesPredicate();
reportBadPrerequisite(
attribute,
- prerequisite.getTarget().getTargetKind(),
prerequisite,
allowedRuleClasses == Predicates.<RuleClass>alwaysTrue()
? null
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java
index bfb346e..a4f2f3d 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelPrerequisiteValidator.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.bazel.rules;
import com.google.devtools.build.lib.analysis.AliasProvider;
+import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.packages.Attribute;
@@ -56,18 +57,20 @@
if (!context.getConfiguration().checkVisibility()) {
errorMessage =
String.format(
- "Target '%s' violates visibility of target "
+ "Target '%s' violates visibility of "
+ "%s. Continuing because --nocheck_visibility is active",
- rule.getLabel(), AliasProvider.printLabelWithAliasChain(prerequisite));
+ rule.getLabel(), AliasProvider.describeTargetWithAliases(prerequisite,
+ TargetMode.WITHOUT_KIND));
context.ruleWarning(errorMessage);
} else {
// Oddly enough, we use reportError rather than ruleError here.
errorMessage =
String.format(
- "Target %s is not visible from target '%s'. Check "
+ "%s is not visible from target '%s'. Check "
+ "the visibility declaration of the former target if you think "
+ "the dependency is legitimate",
- AliasProvider.printLabelWithAliasChain(prerequisite), rule.getLabel());
+ AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND),
+ rule.getLabel());
context.reportError(rule.getLocation(), errorMessage);
}
// We can always post the visibility error as, regardless of the value of keep going,
@@ -91,8 +94,8 @@
+ rule.getRuleClass()
+ " rule "
+ rule.getLabel()
- + ": package group "
- + AliasProvider.printLabelWithAliasChain(prerequisite)
+ + ": "
+ + AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITH_KIND)
+ " is misplaced here "
+ "(they are only allowed in the visibility attribute)");
}
@@ -116,8 +119,8 @@
String message =
"non-test target '"
+ rule.getLabel()
- + "' depends on testonly target "
- + AliasProvider.printLabelWithAliasChain(prerequisite)
+ + "' depends on testonly "
+ + AliasProvider.describeTargetWithAliases(prerequisite, TargetMode.WITHOUT_KIND)
+ " and doesn't have testonly attribute set";
if (thisPackage.startsWith("experimental/")) {
context.ruleWarning(message);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 5f37d87..7580624 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1353,7 +1353,7 @@
"'" + packageName + ":gvalid' does not produce any " + descriptionPlural,
"'" + packageName + ":gmix' does not produce any " + descriptionPlural);
assertSrcsValidity(ruleType, packageName + ":invalid", true,
- "file '" + packageName + ":a.foo' is misplaced here " + descriptionPluralFile,
+ "source file '" + packageName + ":a.foo' is misplaced here " + descriptionPluralFile,
"'" + packageName + ":ginvalid' does not produce any " + descriptionPlural);
assertSrcsValidity(ruleType, packageName + ":mix", true,
"'" + packageName + ":a.foo' does not produce any " + descriptionPlural);
@@ -1736,7 +1736,7 @@
protected String getErrorMsgMisplacedFiles(String attrName, String ruleType, String ruleName,
String fileName) {
- return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": file '"
+ return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": source file '"
+ fileName + "' is misplaced here";
}
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
index 2b32955..8f1a373 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
@@ -432,7 +432,7 @@
invalidatePackages();
getConfiguredTarget("//:x");
assertContainsEvent(
- "Target '//external:zlib' is not visible from target '//:x'. "
+ "target '//external:zlib' is not visible from target '//:x'. "
+ "Check the visibility declaration of the former target if you think the "
+ "dependency is legitimate");
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/AliasTest.java b/src/test/java/com/google/devtools/build/lib/rules/AliasTest.java
index 4784aca..6bc52b5 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/AliasTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/AliasTest.java
@@ -78,7 +78,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//c:c");
assertContainsEvent(
- "Target '//a:a' (aliased through '//b:b') is not visible from target '//c:c'");
+ "alias '//b:b' referring to target '//a:a' is not visible from target '//c:c'");
}
@Test
@@ -94,8 +94,8 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//d:d");
- assertContainsEvent(
- "Target '//a:a' (aliased through '//c:c' -> '//b:b') is not visible from target '//d:d'");
+ assertContainsEvent("alias '//c:c' referring to target '//a:a' through '//b:b' "
+ + "is not visible from target '//d:d'");
}
@Test
@@ -131,7 +131,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a:a");
- assertContainsEvent("filegroup rule '//a:c' (aliased through '//a:b') is misplaced here");
+ assertContainsEvent("alias '//a:b' referring to filegroup rule '//a:c' is misplaced here");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidCommonTest.java
index 8c36bdd..bb33ecb 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidCommonTest.java
@@ -74,7 +74,7 @@
"'//java/srcs:gvalid' is misplaced " + descriptionPlural,
"'//java/srcs:gmix' does not produce any " + descriptionPlural);
assertSrcsValidity(rule, "//java/srcs:invalid", true,
- "file '//java/srcs:a.properties' is misplaced here " + descriptionPluralFile,
+ "source file '//java/srcs:a.properties' is misplaced here " + descriptionPluralFile,
"'//java/srcs:ginvalid' does not produce any " + descriptionPlural);
assertSrcsValidity(rule, "//java/srcs:mix", true,
"'//java/srcs:a.properties' does not produce any " + descriptionPlural);
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 2e0f7b0..8c4b7df 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -1077,7 +1077,7 @@
invalidatePackages();
getConfiguredTarget("//:r");
assertContainsEvent("in label_dict attribute of my_rule rule //:r: "
- + "file '//:myfile.cpp' is misplaced here (expected no files)");
+ + "source file '//:myfile.cpp' is misplaced here (expected no files)");
}
@Test
diff --git a/src/test/shell/bazel/external_correctness_test.sh b/src/test/shell/bazel/external_correctness_test.sh
index d497aa9..b663acc 100755
--- a/src/test/shell/bazel/external_correctness_test.sh
+++ b/src/test/shell/bazel/external_correctness_test.sh
@@ -163,11 +163,11 @@
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"
+ 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"
+ 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"
+ expect_log "target '@r//:fg' is not visible"
}
@@ -235,7 +235,7 @@
bazel build @r//:fg || fail "Build failed"
bazel build //:fg >& $TEST_log && fail "Build succeeded"
- expect_log "Target '@r//r:fg1' is not visible"
+ expect_log "target '@r//r:fg1' is not visible"
}
diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh
index 3f6e2db..2e6b763 100755
--- a/src/test/shell/bazel/local_repository_test.sh
+++ b/src/test/shell/bazel/local_repository_test.sh
@@ -798,7 +798,7 @@
bazel build @r//:public >& $TEST_log || fail "failed to build public target"
bazel build @r//:private >& $TEST_log && fail "could build private target"
- expect_log "Target '//:private' is not visible from target '//external:private'"
+ expect_log "target '//:private' is not visible from target '//external:private'"
}
function test_load_in_remote_repository() {
