Repository: bazelbuild/bazel
Issue: _lcov_merger late-bound test attribute
Labels: type: feature request
P3
coverage
team-Rules-Server

I went to add `_lcov_merger` as an attribute for iOS tests here https://github.com/bazelbuild/rules_apple/pull/691 and there was some discussion about making this tool a late-bound attribute so we wouldn't incur overhead for this tool always. It doesn't seem like there's an API that currently allows us to do this.

There was also some discussion of if the attribute makes sense to be on every test rule or if it should be more global, maybe with a flag like `--lcov_merger` with this tool as the default.

I wanted to file this here so we could discuss this and unblock iOS tests supporting coverage.

Issue comments:
This looks like a dupe of #8736.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index ab553b460b52a1..4cfeb659323e79 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -254,6 +254,7 @@ java_library(
         "test/AnalysisTestActionBuilder.java",
         "test/BaselineCoverageAction.java",
         "test/CoverageCommon.java",
+        "test/CoverageConfiguration.java",
         "test/InstrumentedFileManifestAction.java",
         "test/InstrumentedFilesCollector.java",
         "test/TestActionBuilder.java",
@@ -287,6 +288,7 @@ java_library(
         ":config/build_options",
         ":config/config_conditions",
         ":config/config_matching_provider",
+        ":config/core_option_converters",
         ":config/core_options",
         ":config/execution_transition_factory",
         ":config/fragment",
@@ -379,6 +381,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:package_roots",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
+        "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations",
         "//src/main/java/com/google/devtools/build/lib/analysis/stringtemplate",
         "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index 861206b86c550e..6b6402f01ae4f9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -34,6 +34,7 @@
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.analysis.constraints.ConstraintConstants;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
+import com.google.devtools.build.lib.analysis.test.CoverageConfiguration;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -139,9 +140,6 @@ public static LabelLateBoundDefault<TestConfiguration> coverageSupportAttribute(
   public static final String DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE =
       "//tools/test:coverage_report_generator";
 
-  private static final String DEFAULT_COVERAGE_OUTPUT_GENERATOR_VALUE =
-      "@bazel_tools//tools/test:lcov_merger";
-
   @SerializationConstant @AutoCodec.VisibleForSerialization
   static final Resolver<TestConfiguration, Label> COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER =
       (rule, attributes, configuration) -> configuration.getCoverageReportGenerator();
@@ -152,20 +150,14 @@ public static LabelLateBoundDefault<TestConfiguration> coverageReportGeneratorAt
         TestConfiguration.class, defaultValue, COVERAGE_REPORT_GENERATOR_CONFIGURATION_RESOLVER);
   }
 
-  public static LabelLateBoundDefault<BuildConfiguration> getCoverageOutputGeneratorLabel() {
+  public static LabelLateBoundDefault<CoverageConfiguration> getCoverageOutputGeneratorLabel() {
     return LabelLateBoundDefault.fromTargetConfiguration(
-        BuildConfiguration.class, null, COVERAGE_OUTPUT_GENERATOR_RESOLVER);
+        CoverageConfiguration.class, null, COVERAGE_OUTPUT_GENERATOR_RESOLVER);
   }
 
   @SerializationConstant @AutoCodec.VisibleForSerialization
-  static final Resolver<BuildConfiguration, Label> COVERAGE_OUTPUT_GENERATOR_RESOLVER =
-      (rule, attributes, configuration) -> {
-        if (configuration.isCodeCoverageEnabled()) {
-          return Label.parseAbsoluteUnchecked(DEFAULT_COVERAGE_OUTPUT_GENERATOR_VALUE);
-        } else {
-          return null;
-        }
-      };
+  static final Resolver<CoverageConfiguration, Label> COVERAGE_OUTPUT_GENERATOR_RESOLVER =
+      (rule, attributes, configuration) -> configuration.outputGenerator();
 
   // TODO(b/65746853): provide a way to do this without passing the entire configuration
   /** Implementation for the :run_under attribute. */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java b/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java
index d2b6c6523b7c59..2908893d33c661 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java
@@ -17,6 +17,7 @@
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSet;
+import com.google.devtools.build.lib.analysis.test.CoverageConfiguration;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.analysis.test.TestTrimmingTransitionFactory;
 
@@ -33,6 +34,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
     builder.setShouldInvalidateCacheForOptionDiff(
         TestConfiguration.SHOULD_INVALIDATE_FOR_OPTION_DIFF);
     builder.addConfigurationFragment(TestConfiguration.class);
+    builder.addConfigurationFragment(CoverageConfiguration.class);
     builder.addTrimmingTransitionFactory(new TestTrimmingTransitionFactory());
     builder.addRuleDefinition(new BaseRuleClasses.NativeBuildRule());
     builder.addRuleDefinition(new BaseRuleClasses.NativeActionCreatingRule());


Test patch:
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageConfiguration.java
new file mode 100644
index 00000000000000..7aaccfa4d4eec2
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageConfiguration.java
@@ -0,0 +1,78 @@
+// Copyright 2022 The Bazel Authors. 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.test;
+
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelConverter;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
+import com.google.devtools.build.lib.analysis.config.Fragment;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.RequiresOptions;
+import com.google.devtools.build.lib.analysis.starlark.annotations.StarlarkConfigurationField;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.starlarkbuildapi.test.CoverageConfigurationApi;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDocumentationCategory;
+import com.google.devtools.common.options.OptionEffectTag;
+import javax.annotation.Nullable;
+
+/** The coverage configuration fragment. */
+@Immutable
+@RequiresOptions(options = {CoreOptions.class, CoverageConfiguration.CoverageOptions.class})
+public class CoverageConfiguration extends Fragment implements CoverageConfigurationApi {
+
+  /** Command-line options. */
+  public static class CoverageOptions extends FragmentOptions {
+
+    @Option(
+        name = "coverage_output_generator",
+        converter = LabelConverter.class,
+        defaultValue = "@bazel_tools//tools/test:lcov_merger",
+        documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
+        effectTags = {
+          OptionEffectTag.CHANGES_INPUTS,
+          OptionEffectTag.AFFECTS_OUTPUTS,
+          OptionEffectTag.LOADING_AND_ANALYSIS
+        },
+        help =
+            "Location of the binary that is used to postprocess raw coverage reports. This must "
+                + "currently be a filegroup that contains a single file, the binary. Defaults to "
+                + "'//tools/test:lcov_merger'.")
+    public Label coverageOutputGenerator;
+  }
+
+  private final CoverageOptions coverageOptions;
+
+  public CoverageConfiguration(BuildOptions buildOptions) {
+    if (!buildOptions.get(CoreOptions.class).collectCodeCoverage) {
+      this.coverageOptions = null;
+      return;
+    }
+    this.coverageOptions = buildOptions.get(CoverageOptions.class);
+  }
+
+  @Override
+  @StarlarkConfigurationField(
+      name = "output_generator",
+      doc = "Label for the coverage output generator.")
+  @Nullable
+  public Label outputGenerator() {
+    if (coverageOptions == null) {
+      return null;
+    }
+    return coverageOptions.coverageOutputGenerator;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/BUILD b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/BUILD
index 7ea3da44281863..a295e6e29c4a4a 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/BUILD
@@ -30,5 +30,6 @@ java_library(
         "//src/main/java/net/starlark/java/annot",
         "//src/main/java/net/starlark/java/eval",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
+        "//third_party:jsr305",
     ],
 )
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/CoverageConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/CoverageConfigurationApi.java
new file mode 100644
index 00000000000000..cb342e938313ea
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/CoverageConfigurationApi.java
@@ -0,0 +1,50 @@
+// Copyright 2022 The Bazel Authors. 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.starlarkbuildapi.test;
+
+import com.google.devtools.build.docgen.annot.DocCategory;
+import com.google.devtools.build.lib.cmdline.Label;
+import javax.annotation.Nullable;
+import net.starlark.java.annot.StarlarkBuiltin;
+import net.starlark.java.annot.StarlarkMethod;
+import net.starlark.java.eval.StarlarkValue;
+
+/** Coverage configuration fragment API. */
+@StarlarkBuiltin(
+    name = "coverage",
+    category = DocCategory.CONFIGURATION_FRAGMENT,
+    doc = "A configuration fragment representing the coverage configuration.")
+public interface CoverageConfigurationApi extends StarlarkValue {
+
+  @StarlarkMethod(
+      name = "output_generator",
+      allowReturnNones = true,
+      structField = true,
+      doc =
+          "Returns the label pointed to by the"
+              + " <a href=\"../../user-manual.html#flag--coverage_output_generator\">"
+              + "<code>--coverage_output_generator</code></a> option if coverage collection is"
+              + " enabled, otherwise returns <code>None</code>. Can be accessed with"
+              + " <a href=\"globals.html#configuration_field\"><code>configuration_field"
+              + "</code></a>:<br/>"
+              + "<pre>attr.label(<br/>"
+              + "    default = configuration_field(<br/>"
+              + "        fragment = \"coverage\",<br/>"
+              + "        name = \"output_generator\"<br/>"
+              + "    )<br/>"
+              + ")</pre>")
+  @Nullable
+  Label outputGenerator();
+}
diff --git a/src/test/shell/bazel/bazel_coverage_starlark_test.sh b/src/test/shell/bazel/bazel_coverage_starlark_test.sh
index 0ea7e032ef5dad..e97c1500c5e14c 100755
--- a/src/test/shell/bazel/bazel_coverage_starlark_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_starlark_test.sh
@@ -134,4 +134,90 @@ EOF
         || fail "Coverage report did not contain evidence of custom lcov_merger."
 }
 
+function test_starlark_rule_with_configuration_field_lcov_merger_coverage_enabled() {
+
+    cat <<EOF > lcov_merger.sh
+for var in "\$@"
+do
+    if [[ "\$var" == "--output_file="* ]]; then
+        path="\${var##--output_file=}"
+        mkdir -p "\$(dirname \$path)"
+        echo lcov_merger_called >> \$path
+        exit 0
+    fi
+done
+EOF
+chmod +x lcov_merger.sh
+
+    cat <<EOF > rules.bzl
+def _impl(ctx):
+    output = ctx.actions.declare_file(ctx.attr.name)
+    ctx.actions.write(output, "", is_executable = True)
+    return [DefaultInfo(executable=output)]
+
+custom_test = rule(
+    implementation = _impl,
+    test = True,
+    attrs = {
+        "_lcov_merger": attr.label(
+            default = configuration_field(fragment = "coverage", name = "output_generator"),
+            cfg = "exec"
+        ),
+    },
+    fragments = ["coverage"],
+)
+EOF
+
+    cat <<EOF > BUILD
+load(":rules.bzl", "custom_test")
+
+sh_binary(
+    name = "lcov_merger",
+    srcs = ["lcov_merger.sh"],
+)
+custom_test(name = "foo_test")
+EOF
+
+    bazel coverage --test_output=all //:foo_test --combined_report=lcov --coverage_output_generator=//:lcov_merger > $TEST_log \
+        || fail "Coverage run failed but should have succeeded."
+
+    local coverage_file_path="$( get_coverage_file_path_from_test_log )"
+    cat $coverage_file_path
+    grep "lcov_merger_called"  "$coverage_file_path" \
+        || fail "Coverage report did not contain evidence of custom lcov_merger."
+}
+
+function test_starlark_rule_with_configuration_field_lcov_merger_coverage_disabled() {
+
+    cat <<EOF > rules.bzl
+def _impl(ctx):
+    if ctx.attr._lcov_merger:
+        fail("Expected _lcov_merger to be None if coverage is not collected")
+    output = ctx.actions.declare_file(ctx.attr.name)
+    ctx.actions.write(output, "", is_executable = True)
+    return [DefaultInfo(executable=output)]
+
+custom_test = rule(
+    implementation = _impl,
+    test = True,
+    attrs = {
+        "_lcov_merger": attr.label(
+            default = configuration_field(fragment = "coverage", name = "output_generator"),
+            cfg = "exec"
+        ),
+    },
+    fragments = ["coverage"],
+)
+EOF
+
+    cat <<EOF > BUILD
+load(":rules.bzl", "custom_test")
+
+custom_test(name = "foo_test")
+EOF
+
+    bazel test --test_output=all //:foo_test > $TEST_log \
+        || fail "Test run failed but should have succeeded."
+}
+
 run_suite "test tests"