From f2a07a7336445b9b427c144c95663c2f2649a307 Mon Sep 17 00:00:00 2001 From: gregce Date: Wed, 8 Aug 2018 10:43:59 -0700 Subject: 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 --- .../lib/analysis/RuleConfiguredTargetBuilder.java | 3 +- .../analysis/constraints/ConstraintSemantics.java | 62 ++++++++++++----- .../constraints/SupportedEnvironments.java | 7 +- .../constraints/SupportedEnvironmentsProvider.java | 45 +++++++++++- .../constraints/TopLevelConstraintSemantics.java | 74 +++++++++++++++----- .../lib/analysis/constraints/ConstraintsTest.java | 80 ++++++++++++++++------ 6 files changed, 211 insertions(+), 60 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 removedEnvironmentCulprits = new LinkedHashMap<>(); + Map 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 removedEnvironmentCulprits) { + Map removedEnvironmentCulprits) { Set refinedEnvironmentsSoFar = new LinkedHashSet<>(); // Start with the full set of static environments: refinedEnvironmentsSoFar.addAll(staticEnvironments.getGroupedEnvironments()); @@ -602,7 +604,7 @@ public class ConstraintSemantics { Map labelsToEnvironments, Set refinedEnvironmentsSoFar, Set groupsWithEnvironmentsRemoved, - Map removedEnvironmentCulprits) { + Map 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 groupsWithEnvironmentsRemoved, Set refinedEnvironmentsSoFar, EnvironmentCollection.Builder refinedEnvironments, - Map removedEnvironmentCulprits) { + Map removedEnvironmentCulprits) { Set 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 newlyEmptyGroups, - Map removedEnvironmentCulprits) { - StringBuilder message = new StringBuilder("the current command-line flags disqualify " - + "all supported environments because of incompatible select() paths:"); + Map 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 { * *

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 removedEnvironmentCulprits; + private final ImmutableMap removedEnvironmentCulprits; public SupportedEnvironments( EnvironmentCollection staticEnvironments, EnvironmentCollection refinedEnvironments, - Map removedEnvironmentCulprits) { + Map 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 @@ -42,6 +44,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. + * + *

The culprits are actually two rules: + * + * 

+   *   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"],
+   *       }
+   *
+ * + *

If we build a target with ":has_select" somewhere in its deps and trigger + * ":bcond" and that strips "//foo:a" 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. ":has_select" is the direct culprit, because this is the first rule + * that strips "//foo:a". But it does that because its select() path + * chooses ":bdep", and ":bdep" is why ":has_select" + * decides it's a "//foo:b"-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 { * *

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 exceptionInducingTargets = ArrayListMultimap.create(); + Multimap 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