1 files changed, 58 insertions, 16 deletions

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