Skip to content

Commit 8547937

Browse files
authored
fix: do not require _lcov_merger under coverage (#2273)
Newer versions of bazel do not require it anymore: bazelbuild/bazel#4033 (comment) Removing the explicit failure: - Fixes the false-positive failures when building `js_binary` `testonly = 1` targets under `coverage` (fixes #2229). - Avoids that test rule authors targeting newer bazel versions only have to put a workaround attribute in place. This change reduces discoverability of the _lcov_merger issue for new test rule authors. However, given that it is hopefully an issue of the past soon, I think this is acceptable.
1 parent 3d749a1 commit 8547937

File tree

1 file changed

+10
-5
lines changed

1 file changed

+10
-5
lines changed

js/private/js_binary.bzl

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -575,13 +575,16 @@ def _js_binary_impl(ctx):
575575
providers = []
576576

577577
if ctx.attr.testonly and ctx.configuration.coverage_enabled:
578-
# We have to instruct rule implementers to have this attribute present.
579-
if not hasattr(ctx.attr, "_lcov_merger"):
580-
fail("_lcov_merger attribute is missing and coverage was requested")
581-
582578
# We have to propagate _lcov_merger runfiles since bazel does not treat _lcov_merger as a proper tool.
583579
# See: https://github.com/bazelbuild/bazel/issues/4033
584-
runfiles = runfiles.merge(ctx.attr._lcov_merger[DefaultInfo].default_runfiles)
580+
# This is optional because:
581+
# - We do not want to require it for js_binary targets
582+
# (but we cannot distinguish js_binary from js_test here, see #2229).
583+
# - It is not required anymore on bazel 8
584+
# (https://github.com/bazelbuild/bazel/issues/4033#issuecomment-2507162290)
585+
# TODO: Remove once bazel<8 support is dropped.
586+
if hasattr(ctx.attr, "_lcov_merger"):
587+
runfiles = runfiles.merge(ctx.attr._lcov_merger[DefaultInfo].default_runfiles)
585588
providers = [
586589
coverage_common.instrumented_files_info(
587590
ctx,
@@ -652,6 +655,8 @@ See the Bazel [Test encyclopedia](https://bazel.build/reference/test-encyclopedi
652655
the contract between Bazel and a test runner.""",
653656
implementation = js_binary_lib.implementation,
654657
attrs = dict(js_binary_lib.attrs, **{
658+
# TODO: Remove once bazel<8 support is dropped.
659+
# See comment at usage site in the rule impl for more.
655660
"_lcov_merger": attr.label(
656661
executable = True,
657662
default = Label("//js/private/coverage:merger"),

0 commit comments

Comments
 (0)