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);
