Remove BaselineCoverageArtifactsProvider in favor of an output group.

The only slightly different thing here is that now, instead of using target.getConfiguration().isCodeCoverageEnabled() we use BuildRequest.Options.collectCodeCoverage, but the only place where this is not the same I can think of is InputFileCT, which does not have baseline coverage files anyway.

--
MOS_MIGRATED_REVID=86682774

11 files changed, 69 insertions, 98 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaselineCoverageArtifactsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/BaselineCoverageArtifactsProvider.java
deleted file mode 100644
index ab74581dfe..0000000000
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaselineCoverageArtifactsProvider.java
+++ /dev/null
@@ -1,42 +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.common.collect.ImmutableList;
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-
-/**
- * A {@link TransitiveInfoProvider} that has baseline coverage artifacts.
- */
-@Immutable
-public final class BaselineCoverageArtifactsProvider implements TransitiveInfoProvider {
-  private final ImmutableList<Artifact> baselineCoverageArtifacts;
-
-  public BaselineCoverageArtifactsProvider(ImmutableList<Artifact> baselineCoverageArtifacts) {
-    this.baselineCoverageArtifacts = baselineCoverageArtifacts;
-  }
-
-  /**
-   * Returns a set of baseline coverage artifacts for a given set of configured targets.
-   *
-   * <p>These artifacts represent "empty" code coverage data for non-test libraries and binaries and
-   * used to establish correct baseline when calculating code coverage ratios since they would cover
-   * completely non-tested code as well.
-   */
-  public ImmutableList<Artifact> getBaselineCoverageArtifacts() {
-    return baselineCoverageArtifacts;
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 59adaf7892..91a92560ce 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -40,6 +40,7 @@ import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
@@ -767,17 +768,18 @@ public class BuildView {
         artifactsToBuild, parallelTests, exclusiveTests, topLevelOptions);
   }
 
-  private static ImmutableSet<Artifact> getBaselineCoverageArtifacts(
+  private static NestedSet<Artifact> getBaselineCoverageArtifacts(
       Collection<ConfiguredTarget> configuredTargets) {
-    Set<Artifact> baselineCoverageArtifacts = Sets.newHashSet();
+    NestedSetBuilder<Artifact> baselineCoverageArtifacts = NestedSetBuilder.stableOrder();
     for (ConfiguredTarget target : configuredTargets) {
-      BaselineCoverageArtifactsProvider provider =
-          target.getProvider(BaselineCoverageArtifactsProvider.class);
+      TopLevelArtifactProvider provider = target.getProvider(TopLevelArtifactProvider.class);
       if (provider != null) {
-        baselineCoverageArtifacts.addAll(provider.getBaselineCoverageArtifacts());
+        baselineCoverageArtifacts.addTransitive(provider.getOutputGroup(
+            TopLevelArtifactProvider.BASELINE_COVERAGE
+        ));
       }
     }
-    return ImmutableSet.copyOf(baselineCoverageArtifacts);
+    return baselineCoverageArtifacts.build();
   }
 
   private void addExtraActionsIfRequested(BuildView.Options viewOptions,
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 f0742ea96f..daa24ec380 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
@@ -52,7 +52,6 @@ import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
 
-import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -419,15 +418,6 @@ public final class RuleConfiguredTargetBuilder {
   }
 
   /**
-   * Set the baseline coverage Artifacts.
-   */
-  public RuleConfiguredTargetBuilder setBaselineCoverageArtifacts(
-      Collection<Artifact> artifacts) {
-    return add(BaselineCoverageArtifactsProvider.class,
-        new BaselineCoverageArtifactsProvider(ImmutableList.copyOf(artifacts)));
-  }
-
-  /**
    * Set the mandatory stamp files.
    */
   public RuleConfiguredTargetBuilder setMandatoryStampFiles(ImmutableList<Artifact> files) {
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 70510f702c..b6cbdf91e0 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
@@ -118,8 +118,12 @@ public final class TopLevelArtifactHelper {
       for (String outputGroup : context.outputGroups()) {
         NestedSet<Artifact> results = topLevelArtifactProvider.getOutputGroup(outputGroup);
         if (results != null) {
-          importantBuilder.addTransitive(results);
-        }         
+          if (outputGroup.startsWith(TopLevelArtifactProvider.HIDDEN_OUTPUT_GROUP_PREFIX)) {
+            allBuilder.addTransitive(results);
+          } else {
+            importantBuilder.addTransitive(results);
+          }
+        }
       }
     }
 
@@ -157,29 +161,8 @@ public final class TopLevelArtifactHelper {
       }
     }
 
-    allBuilder.addAll(getCoverageArtifacts(target, context));
-
     NestedSet<Artifact> importantArtifacts = importantBuilder.build();
     allBuilder.addTransitive(importantArtifacts);
     return new ArtifactsToBuild(importantArtifacts, allBuilder.build());
   }
-
-  private static Iterable<Artifact> getCoverageArtifacts(TransitiveInfoCollection target,
-                                                         TopLevelArtifactContext topLevelOptions) {
-    if (!topLevelOptions.compileOnly() && !topLevelOptions.compilationPrerequisitesOnly()
-        && topLevelOptions.shouldRunTests()) {
-      // Add baseline code coverage artifacts if we are collecting code coverage. We do that only
-      // when running tests.
-      // It might be slightly faster to first check if any configuration has coverage enabled.
-      if (target.getConfiguration() != null
-          && target.getConfiguration().isCodeCoverageEnabled()) {
-        BaselineCoverageArtifactsProvider provider =
-            target.getProvider(BaselineCoverageArtifactsProvider.class);
-        if (provider != null) {
-          return provider.getBaselineCoverageArtifacts();
-        }
-      }
-    }
-    return ImmutableList.of();
-  }
 }
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 bab5d4a1de..c91b90f3dd 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
@@ -17,6 +17,8 @@ package com.google.devtools.build.lib.analysis;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 
 /**
@@ -27,17 +29,30 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
  * <p>The artifacts are grouped into "output groups". Which output groups are built is controlled
  * by the {@code --output_groups} undocumented command line option, which in turn is added to the
  * command line at the discretion of the build command being run.
+ *
+ * <p>Output groups starting with an underscore are "not important". This means that artifacts built
+ * because such an output group is mentioned in a {@code --output_groups} command line option are
+ * not mentioned on the output.
  */
 @Immutable
 public final class TopLevelArtifactProvider implements TransitiveInfoProvider {
+  public static final String HIDDEN_OUTPUT_GROUP_PREFIX = "_";
+  public static final String BASELINE_COVERAGE = HIDDEN_OUTPUT_GROUP_PREFIX + "_baseline_coverage";
+
   private final ImmutableMap<String, NestedSet<Artifact>> outputGroups;
 
   TopLevelArtifactProvider(ImmutableMap<String, NestedSet<Artifact>> outputGroups) {
     this.outputGroups = outputGroups;
   }
 
-  /** Return the artifacts in a particular output group. */
+  /** Return the artifacts in a particular output group.
+   *
+   * @return the artifacts in the output group with the given name. The return value is never null.
+   *     If the specified output group is not present, the empty set is returned.
+   */
   public NestedSet<Artifact> getOutputGroup(String outputGroupName) {
-    return outputGroups.get(outputGroupName);
+    return outputGroups.containsKey(outputGroupName)
+        ? outputGroups.get(outputGroupName)
+        : NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER);
   }
 }
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 30d9a83c33..49336877fd 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
@@ -24,6 +24,8 @@ import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.analysis.BuildView;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactProvider;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Options;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
@@ -43,6 +45,8 @@ import com.google.devtools.common.options.OptionsProvider;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
 
@@ -512,7 +516,20 @@ public class BuildRequest implements OptionsClassProvider {
         getBuildOptions().compileOnly, getBuildOptions().compilationPrerequisitesOnly,
         getBuildOptions().buildDefaultArtifacts,
         getOptions(ExecutionOptions.class).testStrategy.equals("exclusive"),
-        ImmutableSet.<String>copyOf(getBuildOptions().outputGroups), shouldRunTests());
+        determineOutputGroups(), shouldRunTests());
+  }
+
+  private ImmutableSet<String> determineOutputGroups() {
+    Set<String> current = new TreeSet<>();
+    current.addAll(getBuildOptions().outputGroups);
+    if (getOptions(Options.class).collectCodeCoverage
+        && !getBuildOptions().compileOnly
+        && !getBuildOptions().compilationPrerequisitesOnly
+        && runTests) {
+      current.add(TopLevelArtifactProvider.BASELINE_COVERAGE);
+    }
+
+    return ImmutableSet.copyOf(current);
   }
 
   public String getSymlinkPrefix() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index b0882c677d..7c3f2ca036 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -30,6 +30,7 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.Util;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
@@ -303,11 +304,11 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
             CppDebugPackageProvider.class,
             new CppDebugPackageProvider(strippedFile, executable, explicitDwpFile))
         .setRunfilesSupport(runfilesSupport, executable)
-        .setBaselineCoverageArtifacts(createBaselineCoverageArtifacts(
-            ruleContext, common, ccCompilationOutputs, fake))
         .addProvider(LipoContextProvider.class, new LipoContextProvider(
             cppCompilationContext, ImmutableMap.copyOf(scannableMap)))
         .addProvider(CppLinkAction.Context.class, linkContext)
+        .addOutputGroup(TopLevelArtifactProvider.BASELINE_COVERAGE,
+            createBaselineCoverageArtifacts(ruleContext, common, ccCompilationOutputs, fake))
         .build();
   }
 
@@ -623,7 +624,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     return builder.build();
   }
 
-  private static ImmutableList<Artifact> createBaselineCoverageArtifacts(
+  private static NestedSet<Artifact> createBaselineCoverageArtifacts(
       RuleContext context, CcCommon common, CcCompilationOutputs compilationOutputs,
       boolean fake) {
     if (!TargetUtils.isTestRule(context.getRule()) && !fake) {
@@ -632,7 +633,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
       return BaselineCoverageAction.getBaselineCoverageArtifacts(context,
           common.getInstrumentedFilesProvider(objectFiles).getInstrumentedFiles());
     } else {
-      return ImmutableList.of();
+      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
     }
   }
 }
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 428eedb6e2..7dc5d80fc9 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
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactProvider;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -262,12 +263,14 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
         .add(RunfilesProvider.class, RunfilesProvider.withData(staticRunfiles, sharedRunfiles))
         // Remove this?
         .add(CppRunfilesProvider.class, new CppRunfilesProvider(staticRunfiles, sharedRunfiles))
-        .setBaselineCoverageArtifacts(BaselineCoverageAction.getBaselineCoverageArtifacts(
-            ruleContext, instrumentedFilesProvider.getInstrumentedFiles()))
         .add(ImplementedCcPublicLibrariesProvider.class,
             new ImplementedCcPublicLibrariesProvider(getImplementedCcPublicLibraries(ruleContext)))
         .add(AlwaysBuiltArtifactsProvider.class,
-            new AlwaysBuiltArtifactsProvider(artifactsToForce));
+            new AlwaysBuiltArtifactsProvider(artifactsToForce))
+        .addOutputGroup(TopLevelArtifactProvider.BASELINE_COVERAGE,
+            BaselineCoverageAction.getBaselineCoverageArtifacts(
+                ruleContext, instrumentedFilesProvider.getInstrumentedFiles()));
+
   }
 
   private static NestedSet<Artifact> collectArtifactsToForce(RuleContext ruleContext,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
index c55a74e0bb..627040ddad 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.analysis.FilesToCompileProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.Util;
@@ -491,10 +492,9 @@ public class JavaCommon {
         .add(JavaExportsProvider.class, new JavaExportsProvider(collectTransitiveExports()));
 
     if (!TargetUtils.isTestRule(ruleContext.getTarget())) {
-      ImmutableList<Artifact> baselineCoverageArtifacts =
+      builder.addOutputGroup(TopLevelArtifactProvider.BASELINE_COVERAGE,
           BaselineCoverageAction.getBaselineCoverageArtifacts(ruleContext,
-          instrumentedFilesCollector.getInstrumentedFiles());
-      builder.setBaselineCoverageArtifacts(baselineCoverageArtifacts);
+              instrumentedFilesCollector.getInstrumentedFiles()));
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
index 6a19f92def..b341b32856 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
@@ -24,6 +24,9 @@ import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Util;
 import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -117,7 +120,7 @@ public class BaselineCoverageAction extends AbstractFileWriteAction
    * Returns collection of baseline coverage artifacts associated with the given target.
    * Will always return 0 or 1 elements.
    */
-  public static ImmutableList<Artifact> getBaselineCoverageArtifacts(RuleContext ruleContext,
+  public static NestedSet<Artifact> getBaselineCoverageArtifacts(RuleContext ruleContext,
       Iterable<Artifact> instrumentedFiles) {
     // Baseline coverage artifacts will still go into "testlogs" directory.
     Artifact coverageData = ruleContext.getAnalysisEnvironment().getDerivedArtifact(
@@ -126,7 +129,7 @@ public class BaselineCoverageAction extends AbstractFileWriteAction
     ruleContext.registerAction(new BaselineCoverageAction(
         ruleContext.getActionOwner(), instrumentedFiles, coverageData));
 
-    return ImmutableList.of(coverageData);
+    return NestedSetBuilder.create(Order.STABLE_ORDER, coverageData);
   }
 
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/CoverageReportActionFactory.java b/src/main/java/com/google/devtools/build/lib/rules/test/CoverageReportActionFactory.java
index 9354d4340b..3a1a6137d8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/CoverageReportActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/CoverageReportActionFactory.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 
 import java.util.Collection;
-import java.util.Set;
 
 import javax.annotation.Nullable;
 
@@ -70,6 +69,6 @@ public interface CoverageReportActionFactory {
   @Nullable
   public CoverageReportActionsWrapper createCoverageReportActionsWrapper(
       Collection<ConfiguredTarget> targetsToTest,
-      Set<Artifact> baselineCoverageArtifacts,
+      Iterable<Artifact> baselineCoverageArtifacts,
       ArtifactFactory artifactFactory, ArtifactOwner artifactOwner);
 }