Simplify the algorithm to compute top-level artifacts to build even more by creating a HIDDEN_TOP_LEVEL output group and putting the artifacts required for building runfiles there.

--
MOS_MIGRATED_REVID=87039530

7 files changed, 80 insertions, 111 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AlwaysBuiltArtifactsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/AlwaysBuiltArtifactsProvider.java
deleted file mode 100644
index e4d40fc265..0000000000
--- a/src/main/java/com/google/devtools/build/lib/analysis/AlwaysBuiltArtifactsProvider.java
+++ /dev/null
@@ -1,46 +0,0 @@
-// Copyright 2014 Google Inc. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.devtools.build.lib.analysis;
-
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-
-/**
- * Artifacts that should be built when a target is mentioned in the command line, but are neither in
- * the {@code filesToBuild} nor in the runfiles.
- *
- * <p>
- * Link actions, may not run a link for their transitive dependencies, so it does not force the
- * source files in the transitive closure to be built by default. However, users expect builds to
- * fail when there is an error in a dependent library, so we use this mechanism to force their
- * compilation.
- */
-@Immutable
-public final class AlwaysBuiltArtifactsProvider implements TransitiveInfoProvider {
-
-  private final NestedSet<Artifact> artifactsToAlwaysBuild;
-
-  public AlwaysBuiltArtifactsProvider(NestedSet<Artifact> artifactsToAlwaysBuild) {
-    this.artifactsToAlwaysBuild = artifactsToAlwaysBuild;
-  }
-
-  /**
-   * Returns the collection of artifacts to be built.
-   */
-  public NestedSet<Artifact> getArtifactsToAlwaysBuild() {
-    return artifactsToAlwaysBuild;
-  }
-}
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 5a053c6943..22d6af008c 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
@@ -92,20 +92,28 @@ public final class RuleConfiguredTargetBuilder {
       return null;
     }
 
-    if (!outputGroupBuilders.isEmpty()) {
-      ImmutableMap.Builder<String, NestedSet<Artifact>> outputGroups = ImmutableMap.builder();
-      for (Map.Entry<String, NestedSetBuilder<Artifact>> entry : outputGroupBuilders.entrySet()) {
-        outputGroups.put(entry.getKey(), entry.getValue().build());
-      }
-
-      add(TopLevelArtifactProvider.class, new TopLevelArtifactProvider(outputGroups.build()));
-    }
-
     FilesToRunProvider filesToRunProvider = new FilesToRunProvider(ruleContext.getLabel(),
         RuleContext.getFilesToRun(runfilesSupport, filesToBuild), runfilesSupport, executable);
     add(FileProvider.class, new FileProvider(ruleContext.getLabel(), filesToBuild));
     add(FilesToRunProvider.class, filesToRunProvider);
 
+    if (runfilesSupport != null) {
+      // If a binary is built, build its runfiles, too
+      addOutputGroup(
+          TopLevelArtifactProvider.HIDDEN_TOP_LEVEL, runfilesSupport.getRunfilesMiddleman());
+    } else if (providers.get(RunfilesProvider.class) != null) {
+      // If we don't have a RunfilesSupport (probably because this is not a binary rule), we still
+      // want to build the files this rule contributes to runfiles of dependent rules so that we
+      // report an error if one of these is broken.
+      //
+      // Note that this is a best-effort thing: there is .getDataRunfiles() and all the language-
+      // specific *RunfilesProvider classes, which we don't add here for reasons that are lost in
+      // the mists of time.
+      addOutputGroup(TopLevelArtifactProvider.HIDDEN_TOP_LEVEL,
+          ((RunfilesProvider) providers.get(RunfilesProvider.class))
+              .getDefaultRunfiles().getAllArtifacts());
+    }
+
     // Create test action and artifacts if target was successfully initialized
     // and is a test.
     if (TargetUtils.isTestRule(ruleContext.getTarget())) {
@@ -113,6 +121,16 @@ public final class RuleConfiguredTargetBuilder {
       add(TestProvider.class, initializeTestProvider(filesToRunProvider));
     }
     add(ExtraActionArtifactsProvider.class, initializeExtraActions());
+    if (!outputGroupBuilders.isEmpty()) {
+      ImmutableMap.Builder<String, NestedSet<Artifact>> outputGroups = ImmutableMap.builder();
+      for (Map.Entry<String, NestedSetBuilder<Artifact>> entry : outputGroupBuilders.entrySet()) {
+        outputGroups.put(entry.getKey(), entry.getValue().build());
+      }
+
+      add(TopLevelArtifactProvider.class, new TopLevelArtifactProvider(outputGroups.build()));
+    }
+
+
     return new RuleConfiguredTarget(
         ruleContext, mandatoryStampFiles, skylarkProviders.build(), providers);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java
index d4c1061298..f6473d7c2e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactContext.java
@@ -27,27 +27,18 @@ import java.util.Set;
 public final class TopLevelArtifactContext {
 
   public static final TopLevelArtifactContext DEFAULT = new TopLevelArtifactContext(
-      "", /*buildDefaultArtifacts=*/true, /*runTestsExclusively=*/false,
-      /*outputGroups=*/ImmutableSet.<String>of(), /*shouldRunTests=*/false);
+      /*buildDefaultArtifacts=*/true, /*runTestsExclusively=*/false,
+      /*outputGroups=*/ImmutableSet.<String>of());
 
-  private final String buildCommand;
   private final boolean buildDefaultArtifacts;
   private final boolean runTestsExclusively;
   private final ImmutableSet<String> outputGroups;
-  private final boolean shouldRunTests;
 
-  public TopLevelArtifactContext(String buildCommand, boolean filesToRun,
-      boolean runTestsExclusively, ImmutableSet<String> outputGroups, boolean shouldRunTests) {
-    this.buildCommand = buildCommand;
+  public TopLevelArtifactContext(boolean filesToRun,
+      boolean runTestsExclusively, ImmutableSet<String> outputGroups) {
     this.buildDefaultArtifacts = filesToRun;
     this.runTestsExclusively = runTestsExclusively;
     this.outputGroups = outputGroups;
-    this.shouldRunTests = shouldRunTests;
-  }
-
-  /** Returns the build command as a string. */
-  public String buildCommand() {
-    return buildCommand;
   }
 
   /** Returns the value of the (undocumented) --build_default_artifacts flag. */
@@ -65,22 +56,15 @@ public final class TopLevelArtifactContext {
     return outputGroups;
   }
 
-  /** Whether the top-level request command may run tests. */
-  public boolean shouldRunTests() {
-    return shouldRunTests;
-  }
-  
   // TopLevelArtifactContexts are stored in maps in BuildView,
   // so equals() and hashCode() need to work.
   @Override
   public boolean equals(Object other) {
     if (other instanceof TopLevelArtifactContext) {
       TopLevelArtifactContext otherContext = (TopLevelArtifactContext) other;
-      return buildCommand.equals(otherContext.buildCommand)
-          && buildDefaultArtifacts == otherContext.buildDefaultArtifacts
+      return buildDefaultArtifacts == otherContext.buildDefaultArtifacts
           && runTestsExclusively == otherContext.runTestsExclusively
-          && outputGroups.equals(otherContext.outputGroups)
-          && shouldRunTests == otherContext.shouldRunTests;
+          && outputGroups.equals(otherContext.outputGroups);
     } else {
       return false;
     }
@@ -88,7 +72,6 @@ public final class TopLevelArtifactContext {
 
   @Override
   public int hashCode() {
-    return Objects.hash(
-        buildCommand, buildDefaultArtifacts, runTestsExclusively, outputGroups, shouldRunTests);
+    return Objects.hash(buildDefaultArtifacts, runTestsExclusively, outputGroups);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
index 44d1e27f91..c6d0dfad24 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactHelper.java
@@ -131,25 +131,13 @@ public final class TopLevelArtifactHelper {
     if (context.buildDefaultArtifacts()
         && !context.outputGroups().contains(TopLevelArtifactProvider.COMPILATION_PREREQUISITES)
         && !context.outputGroups().contains(TopLevelArtifactProvider.FILES_TO_COMPILE)) {
-      FilesToRunProvider filesToRunProvider = target.getProvider(FilesToRunProvider.class);
-      boolean hasRunfilesSupport = false;
-      if (filesToRunProvider != null) {
-        importantBuilder.addAll(filesToRunProvider.getFilesToRun());
-        hasRunfilesSupport = filesToRunProvider.getRunfilesSupport() != null;
+      FileProvider fileProvider = target.getProvider(FileProvider.class);
+      if (fileProvider != null) {
+        importantBuilder.addTransitive(fileProvider.getFilesToBuild());
       }
-
-      if (!hasRunfilesSupport) {
-        RunfilesProvider runfilesProvider =
-            target.getProvider(RunfilesProvider.class);
-        if (runfilesProvider != null) {
-          allBuilder.addTransitive(runfilesProvider.getDefaultRunfiles().getAllArtifacts());
-        }
-      }
-
-      AlwaysBuiltArtifactsProvider forcedArtifacts = target.getProvider(
-          AlwaysBuiltArtifactsProvider.class);
-      if (forcedArtifacts != null) {
-        allBuilder.addTransitive(forcedArtifacts.getArtifactsToAlwaysBuild());
+      if (topLevelArtifactProvider != null) {
+        allBuilder.addTransitive(
+            topLevelArtifactProvider.getOutputGroup(TopLevelArtifactProvider.HIDDEN_TOP_LEVEL));
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactProvider.java
index f44fddbf0b..3b9631f990 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TopLevelArtifactProvider.java
@@ -36,11 +36,38 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
  */
 @Immutable
 public final class TopLevelArtifactProvider implements TransitiveInfoProvider {
+
+  /**
+   * Prefix for output groups that are not reported to the user on the terminal output of Blaze when
+   * they are built.
+   */
   public static final String HIDDEN_OUTPUT_GROUP_PREFIX = "_";
-  public static final String BASELINE_COVERAGE = HIDDEN_OUTPUT_GROUP_PREFIX + "_baseline_coverage";
+
+  /**
+   * Baseline coverage artifacts.
+   */
+  public static final String BASELINE_COVERAGE = HIDDEN_OUTPUT_GROUP_PREFIX + "baseline_coverage";
+
+  /**
+   * Building these artifacts only results in the compilation (and not e.g. linking) of the
+   * associated target. Mostly useful for C++, less so for e.g. Java.
+   */
   public static final String FILES_TO_COMPILE = "files_to_compile";
+
+  /**
+   * These artifacts are the direct requirements for compilation, but building these does not
+   * actually compile the target. Mostly useful when IDEs want Blaze to emit generated code so that
+   * they can do the compilation in their own way.
+   */
   public static final String COMPILATION_PREREQUISITES = "compilation_prerequisites";
 
+  /**
+   * These files are built when a target is mentioned on the command line, but are not reported to
+   * the user. This is mostly runfiles, which is necessary because we don't want a target to
+   * successfully build if a file in its runfiles is broken.
+   */
+  public static final String HIDDEN_TOP_LEVEL = HIDDEN_OUTPUT_GROUP_PREFIX + "hidden_top_level";
+
   private final ImmutableMap<String, NestedSet<Artifact>> outputGroups;
 
   TopLevelArtifactProvider(ImmutableMap<String, NestedSet<Artifact>> outputGroups) {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
index 4cbf5a1ba9..ac8c693736 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
@@ -512,10 +512,10 @@ public class BuildRequest implements OptionsClassProvider {
 
   /** Creates a new TopLevelArtifactContext from this build request. */
   public TopLevelArtifactContext getTopLevelArtifactContext() {
-    return new TopLevelArtifactContext(getCommandName(),
+    return new TopLevelArtifactContext(
         getBuildOptions().buildDefaultArtifacts,
         getOptions(ExecutionOptions.class).testStrategy.equals("exclusive"),
-        determineOutputGroups(), shouldRunTests());
+        determineOutputGroups());
   }
 
   private ImmutableSet<String> determineOutputGroups() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index d85a6fa091..50654e6b67 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -19,7 +19,6 @@ import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.AlwaysBuiltArtifactsProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
@@ -232,7 +231,7 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
     CcLinkingOutputs linkedLibraries = info.getCcLinkingOutputsExcludingPrecompiledLibraries();
 
     NestedSet<Artifact> artifactsToForce =
-        collectArtifactsToForce(ruleContext, common, info.getCcCompilationOutputs());
+        collectHiddenTopLevelArtifacts(ruleContext, common, info.getCcCompilationOutputs());
 
     NestedSetBuilder<Artifact> filesBuilder = NestedSetBuilder.stableOrder();
     filesBuilder.addAll(LinkerInputs.toLibraryArtifacts(linkedLibraries.getStaticLibraries()));
@@ -266,23 +265,23 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
         .add(CppRunfilesProvider.class, new CppRunfilesProvider(staticRunfiles, sharedRunfiles))
         .add(ImplementedCcPublicLibrariesProvider.class,
             new ImplementedCcPublicLibrariesProvider(getImplementedCcPublicLibraries(ruleContext)))
-        .add(AlwaysBuiltArtifactsProvider.class,
-            new AlwaysBuiltArtifactsProvider(artifactsToForce))
-        .addOutputGroup(TopLevelArtifactProvider.BASELINE_COVERAGE,
-            BaselineCoverageAction.getBaselineCoverageArtifacts(
-                ruleContext, instrumentedFilesProvider.getInstrumentedFiles()));
+        .addOutputGroup(TopLevelArtifactProvider.HIDDEN_TOP_LEVEL, artifactsToForce)
+        .addOutputGroup(TopLevelArtifactProvider.BASELINE_COVERAGE, BaselineCoverageAction
+                .getBaselineCoverageArtifacts(ruleContext,
+                    instrumentedFilesProvider.getInstrumentedFiles()));
 
   }
 
-  private static NestedSet<Artifact> collectArtifactsToForce(RuleContext ruleContext,
+  private static NestedSet<Artifact> collectHiddenTopLevelArtifacts(RuleContext ruleContext,
       CcCommon common, CcCompilationOutputs ccCompilationOutputs) {
     // Ensure that we build all the dependencies, otherwise users may get confused.
     NestedSetBuilder<Artifact> artifactsToForceBuilder = NestedSetBuilder.stableOrder();
     artifactsToForceBuilder.addTransitive(
         NestedSetBuilder.wrap(Order.STABLE_ORDER, common.getFilesToCompile(ccCompilationOutputs)));
-    for (AlwaysBuiltArtifactsProvider dep :
-        ruleContext.getPrerequisites("deps", Mode.TARGET, AlwaysBuiltArtifactsProvider.class)) {
-      artifactsToForceBuilder.addTransitive(dep.getArtifactsToAlwaysBuild());
+    for (TopLevelArtifactProvider dep :
+        ruleContext.getPrerequisites("deps", Mode.TARGET, TopLevelArtifactProvider.class)) {
+      artifactsToForceBuilder.addTransitive(
+          dep.getOutputGroup(TopLevelArtifactProvider.HIDDEN_TOP_LEVEL));
     }
     return artifactsToForceBuilder.build();
   }