diff options
author | gregce <gregce@google.com> | 2018-08-08 10:43:59 -0700 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-08-08 10:45:57 -0700 |
commit | f2a07a7336445b9b427c144c95663c2f2649a307 (patch) | |
tree | f4408c5b57fa270456deef047f512e6fb59ffa17 /src/main/java/com/google/devtools | |
parent | 8ed7d9670e619b6c0d2ef35ace61ede1562b9d56 (diff) |
Improve error messaging for constraints violations.
1) Break up dense lines with clearer pretty-printing.
2) When a violation happens because of a select(), mention
both the target with the select (as before) *and*
the dep that the select() chose.
3) Integrate this messaging into --target_environment violations,
which currently provide no info about the root cause.
Examples:
-------------------------------------
select() + compatible_with violation:
-------------------------------------
Before:
ERROR: /workspace/testapp/BUILD:41:1: in cc_binary rule //testapp:top: the current command-line flags disqualify all supported environments because of incompatible select() paths:
environment: //constraints:p removed by: //testapp:midlib (/workspace/testapp/BUILD:28:1)
After:
ERROR: /workspace/testapp/BUILD:41:1: in cc_binary rule //testapp:top: the current command line flags disqualify all supported environments because of incompatible select() paths:
environment: //constraints:p
removed by: //testapp:midlib (/workspace/testapp/BUILD:28:1)
which has a select() that chooses dep: //testapp:glib
which lacks: //constraints:p.
-------------------------------------
select() + --target_environment=//constraints:p violation:
-------------------------------------
Before:
ERROR: This is a restricted-environment build.
- //testapp:top does not support required environment //constraints:p
After:
ERROR: This is a restricted-environment build.
//testapp:top does not support:
environment: //constraints:p
removed by: //testapp:midlib (/workspace/testapp/BUILD:28:1)
which has a select() that chooses dep: //testapp:g
which lacks: //constraints:p
Fixes: #5795
PiperOrigin-RevId: 207910308
Diffstat (limited to 'src/main/java/com/google/devtools')
5 files changed, 153 insertions, 38 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index ddc1ad6746..09b989f751 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics; import com.google.devtools.build.lib.analysis.constraints.EnvironmentCollection; import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironments; import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider; +import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider.RemovedEnvironmentCulprit; import com.google.devtools.build.lib.analysis.test.ExecutionInfo; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesProvider; import com.google.devtools.build.lib.analysis.test.TestActionBuilder; @@ -183,7 +184,7 @@ public final class RuleConfiguredTargetBuilder { constraintSemantics.getSupportedEnvironments(ruleContext); if (supportedEnvironments != null) { EnvironmentCollection.Builder refinedEnvironments = new EnvironmentCollection.Builder(); - Map<Label, LabelAndLocation> removedEnvironmentCulprits = new LinkedHashMap<>(); + Map<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits = new LinkedHashMap<>(); constraintSemantics.checkConstraints(ruleContext, supportedEnvironments, refinedEnvironments, removedEnvironmentCulprits); add(SupportedEnvironmentsProvider.class, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java index ee74b3fcff..747bfd5695 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.constraints.EnvironmentCollection.EnvironmentWithGroup; +import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider.RemovedEnvironmentCulprit; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.AttributeMap; @@ -47,6 +48,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.StringJoiner; import javax.annotation.Nullable; /** @@ -537,7 +539,7 @@ public class ConstraintSemantics { RuleContext ruleContext, EnvironmentCollection staticEnvironments, EnvironmentCollection.Builder refinedEnvironments, - Map<Label, LabelAndLocation> removedEnvironmentCulprits) { + Map<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits) { Set<EnvironmentWithGroup> refinedEnvironmentsSoFar = new LinkedHashSet<>(); // Start with the full set of static environments: refinedEnvironmentsSoFar.addAll(staticEnvironments.getGroupedEnvironments()); @@ -602,7 +604,7 @@ public class ConstraintSemantics { Map<Label, EnvironmentWithGroup> labelsToEnvironments, Set<EnvironmentWithGroup> refinedEnvironmentsSoFar, Set<EnvironmentLabels> groupsWithEnvironmentsRemoved, - Map<Label, LabelAndLocation> removedEnvironmentCulprits) { + Map<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits) { SupportedEnvironmentsProvider depEnvironments = dep.getProvider(SupportedEnvironmentsProvider.class); @@ -632,7 +634,7 @@ public class ConstraintSemantics { refinedEnvironmentsSoFar.remove(envToPrune); groupsWithEnvironmentsRemoved.add(envToPrune.group()); removedEnvironmentCulprits.put(envToPrune.environment(), - findOriginalRefiner(ruleContext, depEnvironments, envToPrune)); + findOriginalRefiner(ruleContext, dep.getLabel(), depEnvironments, envToPrune)); prunedEnvironmentsFromThisDep.add(envToPrune.environment()); } @@ -662,7 +664,7 @@ public class ConstraintSemantics { Set<EnvironmentLabels> groupsWithEnvironmentsRemoved, Set<EnvironmentWithGroup> refinedEnvironmentsSoFar, EnvironmentCollection.Builder refinedEnvironments, - Map<Label, LabelAndLocation> removedEnvironmentCulprits) { + Map<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits) { Set<EnvironmentLabels> refinedGroups = new LinkedHashSet<>(); for (EnvironmentWithGroup envWithGroup : refinedEnvironmentsSoFar) { refinedEnvironments.put(envWithGroup.group(), envWithGroup.environment()); @@ -683,24 +685,40 @@ public class ConstraintSemantics { */ private static String getOverRefinementError( Set<EnvironmentLabels> newlyEmptyGroups, - Map<Label, LabelAndLocation> removedEnvironmentCulprits) { - StringBuilder message = new StringBuilder("the current command-line flags disqualify " - + "all supported environments because of incompatible select() paths:"); + Map<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits) { + StringJoiner message = new StringJoiner("\n") + .add("the current command line flags disqualify all supported environments because of " + + "incompatible select() paths:"); for (EnvironmentLabels group : newlyEmptyGroups) { if (newlyEmptyGroups.size() > 1) { - message.append("\n\nenvironment group: " + group.getLabel() + ":"); + message + .add(" ") + .add("environment group: " + group.getLabel() + ":"); } for (Label prunedEnvironment : group.getEnvironments()) { - LabelAndLocation culprit = removedEnvironmentCulprits.get(prunedEnvironment); - if (culprit != null) { // Only environments this rule declared support for have culprits. - message.append("\n environment: " + prunedEnvironment - + " removed by: " + culprit.getLabel() + " (" + culprit.getLocation() + ")"); + RemovedEnvironmentCulprit culprit = removedEnvironmentCulprits.get(prunedEnvironment); + // Only environments this rule statically declared support for have culprits. + if (culprit != null) { + message + .add(" ") + .add(getMissingEnvironmentCulpritMessage(prunedEnvironment, culprit)); } } } return message.toString(); } + static String getMissingEnvironmentCulpritMessage(Label environment, + RemovedEnvironmentCulprit reason) { + LabelAndLocation culprit = reason.culprit(); + return new StringJoiner("\n") + .add(" environment: " + environment) + .add(" removed by: " + culprit.getLabel() + " (" + culprit.getLocation() + ")") + .add(" which has a select() that chooses dep: " + reason.selectedDepForCulprit()) + .add(" which lacks: " + environment) + .toString(); + } + /** * Given an environment that should be refined out of the current rule because of the given dep, * returns the original dep that caused the removal. @@ -713,12 +731,24 @@ public class ConstraintSemantics { * * <p>then D2 is the original refiner (even though D1 and R inherit the same pruning). */ - private static LabelAndLocation findOriginalRefiner( - RuleContext ruleContext, SupportedEnvironmentsProvider dep, EnvironmentWithGroup envToPrune) { - LabelAndLocation depCulprit = dep.getRemovedEnvironmentCulprit(envToPrune.environment()); + private static RemovedEnvironmentCulprit findOriginalRefiner(RuleContext ruleContext, Label dep, + SupportedEnvironmentsProvider depEnvironments, EnvironmentWithGroup envToPrune) { + RemovedEnvironmentCulprit depCulprit = + depEnvironments.getRemovedEnvironmentCulprit(envToPrune.environment()); + if (depCulprit != null) { + return depCulprit; + } // If the dep has no record of this environment being refined, that means the current rule // is the culprit. - return depCulprit == null ? LabelAndLocation.of(ruleContext.getTarget()) : depCulprit; + return RemovedEnvironmentCulprit.create( + LabelAndLocation.of(ruleContext.getTarget()), + // While it'd be nice to know the dep's location too, it isn't strictly necessary. + // Especially since we already have the parent's location. So it's easy enough to find the + // dep. And we want to respect the efficiency concerns described in LabelAndLocation. + // + // Alternatively, we could prepare error strings directly in SupportedEnvironmentsProvider, + // which should remove the need for LabelAndLocation for any target. + dep); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironments.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironments.java index c1eaac7419..c1c5408bbd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironments.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironments.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.analysis.constraints; import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.analysis.LabelAndLocation; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import java.util.Map; @@ -25,12 +24,12 @@ import java.util.Map; public class SupportedEnvironments implements SupportedEnvironmentsProvider { private final EnvironmentCollection staticEnvironments; private final EnvironmentCollection refinedEnvironments; - private final ImmutableMap<Label, LabelAndLocation> removedEnvironmentCulprits; + private final ImmutableMap<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits; public SupportedEnvironments( EnvironmentCollection staticEnvironments, EnvironmentCollection refinedEnvironments, - Map<Label, LabelAndLocation> removedEnvironmentCulprits) { + Map<Label, RemovedEnvironmentCulprit> removedEnvironmentCulprits) { this.staticEnvironments = staticEnvironments; this.refinedEnvironments = refinedEnvironments; this.removedEnvironmentCulprits = ImmutableMap.copyOf(removedEnvironmentCulprits); @@ -47,7 +46,7 @@ public class SupportedEnvironments implements SupportedEnvironmentsProvider { } @Override - public LabelAndLocation getRemovedEnvironmentCulprit(Label environment) { + public RemovedEnvironmentCulprit getRemovedEnvironmentCulprit(Label environment) { return removedEnvironmentCulprits.get(environment); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironmentsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironmentsProvider.java index 6ef6785474..3ad2a25ac4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironmentsProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/SupportedEnvironmentsProvider.java @@ -14,9 +14,11 @@ package com.google.devtools.build.lib.analysis.constraints; +import com.google.auto.value.AutoValue; import com.google.devtools.build.lib.analysis.LabelAndLocation; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; /** * A provider that advertises which environments the associated target is compatible with @@ -43,6 +45,47 @@ public interface SupportedEnvironmentsProvider extends TransitiveInfoProvider { EnvironmentCollection getRefinedEnvironments(); /** + * Provides all context necessary to communicate which dependencies caused an environment to be + * refined out of the current rule. + * + * <p>The culprit<b>s</b> are actually two rules: + * + * <pre> + * some_rule(name = "adep", restricted_to = ["//foo:a"]) + * + * some_rule(name = "bdep", restricted_to = ["//foo:b"]) + * + * some_rule( + * name = "has_select", + * restricted_to = ["//foo:a", "//foo:b"], + * deps = select({ + * ":acond": [:"adep"], + * ":bcond": [:"bdep"], + * } + * </pre> + * + * <p>If we build a target with <code>":has_select"</code> somewhere in its deps and trigger + * <code>":bcond"</code> and that strips <code>"//foo:a"</code> out of the top-level target's + * environments in a way that triggers an error, the user needs to understand two rules to trace + * this error. <code>":has_select"</code> is the direct culprit, because this is the first rule + * that strips <code>"//foo:a"</code>. But it does that because its <code>select()</code> path + * chooses <code>":bdep"</code>, and <code>":bdep"</code> is why <code>":has_select"</code> + * decides it's a <code>"//foo:b"</code>-only rule for this build. + */ + @AutoValue + abstract class RemovedEnvironmentCulprit { + @AutoCodec.Instantiator + public static RemovedEnvironmentCulprit create(LabelAndLocation culprit, + Label selectedDepForCulprit) { + return new AutoValue_SupportedEnvironmentsProvider_RemovedEnvironmentCulprit(culprit, + selectedDepForCulprit); + } + + abstract LabelAndLocation culprit(); + abstract Label selectedDepForCulprit(); + } + + /** * If the given environment was refined away from this target's set of supported environments, * returns the dependency that originally removed the environment. * @@ -55,5 +98,5 @@ public interface SupportedEnvironmentsProvider extends TransitiveInfoProvider { * * <p>See {@link ConstraintSemantics} class documentation for more details on refinement. */ - LabelAndLocation getRemovedEnvironmentCulprit(Label environment); + RemovedEnvironmentCulprit getRemovedEnvironmentCulprit(Label environment); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java index 130d4ef453..b4d38b231c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis.constraints; -import com.google.common.base.Joiner; import com.google.common.base.Predicates; import com.google.common.base.Verify; import com.google.common.collect.ArrayListMultimap; @@ -26,6 +25,7 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.ViewCreationFailedException; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; +import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider.RemovedEnvironmentCulprit; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -39,6 +39,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.StringJoiner; import java.util.function.Function; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -71,6 +72,19 @@ public class TopLevelConstraintSemantics { this.eventHandler = eventHandler; } + private static class MissingEnvironment { + private final Label environment; + @Nullable + // If null, the top-level target just didn't declare a required environment. If not null, that + // means the declaration got "refined" away due to some select() somewhere in its deps. See + // ConstraintSemantics's documentation for an explanation of refinement. + private final RemovedEnvironmentCulprit culprit; + private MissingEnvironment(Label environment, RemovedEnvironmentCulprit culprit) { + this.environment = environment; + this.culprit = culprit; + } + } + /** * Checks that if this is an environment-restricted build, all top-level targets support expected * top-level environments. Expected top-level environments can be declared explicitly through @@ -97,7 +111,8 @@ public class TopLevelConstraintSemantics { // they're missing. These targets trigger a ViewCreationFailedException, which halts the build. // Targets with missing *implicitly* required environments don't belong here, since the build // continues while skipping them. - Multimap<ConfiguredTarget, Label> exceptionInducingTargets = ArrayListMultimap.create(); + Multimap<ConfiguredTarget, MissingEnvironment> exceptionInducingTargets = + ArrayListMultimap.create(); for (ConfiguredTarget topLevelTarget : topLevelTargets) { BuildConfiguration config = configurationProvider.apply(topLevelTarget.getConfigurationKey()); Target target = null; @@ -132,8 +147,7 @@ public class TopLevelConstraintSemantics { badTargets.add(topLevelTarget); } } catch (NoSuchPackageException - | NoSuchTargetException - | ConstraintSemantics.EnvironmentLookupException e) { + | NoSuchTargetException e) { throw new ViewCreationFailedException("invalid target environment", e); } } @@ -153,8 +167,7 @@ public class TopLevelConstraintSemantics { */ private List<Label> autoConfigureTargetEnvironments(BuildConfiguration config, @Nullable Label environmentGroupLabel) - throws InterruptedException, NoSuchTargetException, NoSuchPackageException, - ConstraintSemantics.EnvironmentLookupException { + throws InterruptedException, NoSuchTargetException, NoSuchPackageException { if (environmentGroupLabel == null) { return ImmutableList.of(); } @@ -181,7 +194,7 @@ public class TopLevelConstraintSemantics { * @throw InterruptedException if environment target resolution fails * @throw ViewCreationFailedException if an expected environment isn't a valid target */ - private Collection<Label> getMissingEnvironments(ConfiguredTarget topLevelTarget, + private Collection<MissingEnvironment> getMissingEnvironments(ConfiguredTarget topLevelTarget, Collection<Label> expectedEnvironmentLabels) throws InterruptedException, ViewCreationFailedException { if (expectedEnvironmentLabels.isEmpty()) { @@ -218,6 +231,8 @@ public class TopLevelConstraintSemantics { // other environment groups. We don't care about those. We only care about the environments // explicitly referenced. .filter(Predicates.in(expectedEnvironmentLabels)) + .map(environment -> + new MissingEnvironment(environment, provider.getRemovedEnvironmentCulprit(environment))) .collect(Collectors.toSet()); } @@ -225,15 +240,42 @@ public class TopLevelConstraintSemantics { * Prepares a user-friendly error message for a list of targets missing support for required * environments. */ - private static String getBadTargetsUserMessage(Multimap<ConfiguredTarget, Label> badTargets) { - StringBuilder msg = new StringBuilder(); - msg.append("This is a restricted-environment build."); - for (Map.Entry<ConfiguredTarget, Collection<Label>> entry : badTargets.asMap().entrySet()) { - msg.append(String.format("\n - %s does not support required environment%s %s.", - entry.getKey().getLabel(), - entry.getValue().size() == 1 ? "" : "s", - Joiner.on(", ").join(entry.getValue()))); + private static String getBadTargetsUserMessage(Multimap<ConfiguredTarget, + MissingEnvironment> badTargets) { + StringJoiner msg = new StringJoiner("\n"); + msg.add("This is a restricted-environment build."); + for (Map.Entry<ConfiguredTarget, Collection<MissingEnvironment>> entry : + badTargets.asMap().entrySet()) { + msg + .add(" ") + .add(entry.getKey().getLabel() + " does not support:"); + boolean isFirst = true; + boolean lastEntryWasMultiline = false; + for (MissingEnvironment missingEnvironment : entry.getValue()) { + if (missingEnvironment.culprit == null) { + // The target didn't declare support for this environment. + if (lastEntryWasMultiline) { + // Pretty-format: if the last environment message was multi-line, make it clear this + // one is a different entry. But we don't want to do that if all entries are single-line + // because that would be pointlessly long. + msg.add(" "); + } + msg.add(" " + missingEnvironment.environment); + lastEntryWasMultiline = false; + } else { + // The target declared support, but it was refined out by a select() somewhere in its + // transitive deps. + if (!isFirst) { + msg.add(" "); // Pretty-format for clarity. + } + msg.add( + ConstraintSemantics.getMissingEnvironmentCulpritMessage( + missingEnvironment.environment, missingEnvironment.culprit)); + lastEntryWasMultiline = true; + } + isFirst = false; + } } - return msg.toString(); + return msg.add(" ").toString(); } } |