Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rules_go coverage tests broken with 0.27.0 #8670

Closed
jayconrod opened this issue Jun 18, 2019 · 9 comments
Closed

rules_go coverage tests broken with 0.27.0 #8670

jayconrod opened this issue Jun 18, 2019 · 9 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Rules-Java Issues for Java rules type: bug

Comments

@jayconrod
Copy link
Contributor

Description of the problem / feature request:

Several rules_go tests that run bazel coverage are failing with Bazel 0.27.0. They pass with 0.26.1, and there haven't been any recent changes to rules_go coverage functionality.

https://buildkite.com/bazel/rules-go-golang/builds/1197#7fba7969-7a88-4201-b0a4-8982d9ebf8d0 is set of test results for an unrelated change. It looks like this is also failing on the "Bazel@HEAD + Downstream" pipeline.

The failures include a lot of bash debugging lines (running with -x?). I think this is the relevant part.

+ BULK_COVERAGE_RUN=1
+ for name in '"$LCOV_MERGER"'
+ [[ ! -e '' ]]
+ echo --
--
+ echo Coverage runner: cannot locate file
Coverage runner: cannot locate file

I'm not sure what file is missing, but something to do with LCOV_MERGER maybe?

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

git clone https://github.com/bazelbuild/rules_go
cd rules_go
bazel coverage //tests/core/coverage:coverage_test

What operating system are you running Bazel on?

macOS 10.14.5

Tests are failing on all platforms though.

What's the output of bazel info release?

release 0.27.0

Have you found anything relevant by searching the web?

#7529 has the same error message, but I'm not sure if it's related.

bazelbuild/rules_go#2100 is corresponding issue in rules_go.

Any other information, logs, or outputs that you want to share?

rules_go coverage support is not completely integrated with Bazel coverage. We use the information exposed to Starlark rules by bazel coverage in order to add coverage instrumentation to sources that need it. However, the coverage data file is written in Go's format (not lcov) so that Go tools can analyze it.

Tests do write to the file named by COVERAGE_OUTPUT_FILE, and I've verified that such a file is written by manually running a test binary built with coverage. So I don't think that's the missing file.

@jayconrod
Copy link
Contributor Author

cc @iirina who I think is the coverage expert. Any idea what might be wrong?

@benjaminp
Copy link
Collaborator

I believe this is a consequence of 384e1cb, which broke the #6293 (comment) workaround. My current hack is --test_env=LCOV_MERGER=/bin/true.

@jayconrod
Copy link
Contributor Author

Gentle ping

@laurentlb laurentlb added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jun 21, 2019
@laurentlb
Copy link
Contributor

P1, because it's a regression in Bazel 0.27 (also it breaks tests in rules_go).

@iirina
Copy link
Contributor

iirina commented Jun 24, 2019

Sorry for the trouble, I came back from vacation today. 384e1cb renamed the attribute from $lcov_merger to :lcov_merger, which is a breaking change. For the time being I'll change the test runner to check both attribute names and then introduce an incompatible flag to remove the former.

@jayconrod
Copy link
Contributor Author

@iirina Thanks for fixing this! This means _lcov_merger can still be used from Starlark rules, right? I think that's equivalent to $lcov_merger for Starlark rules. go_test currently defines _lcov_merger as a workaround for #6293.

Speaking of #6293, could you comment on what rules_go + Bazel + CI should do here? I understand that Bazel needs a tool to merge lcov files from multiple tests with coverage enabled, and that's /usr/bin/lcov by default. However, /usr/bin/lcov isn't installed on most systems (including images used on BuildKite). It looks like there are some lcov utilities in @bazel_tools. Will /usr/bin/lcov not be needed in the future? Should test rules still define the _lcov_merger attribute, or will there be another default behavior in the future?

@benjaminp
Copy link
Collaborator

Yeah, can we fix coverage to not try to invoke lcov if the rule doesn't have the lcov attribute?

@iirina
Copy link
Contributor

iirina commented Jun 25, 2019

This means _lcov_merger can still be used from Starlark rules, right? I think that's equivalent to $lcov_merger for Starlark rules. go_test currently defines _lcov_merger as a workaround for #6293.

Yes, _lcov_merger can still be used from Starlark rules. I'm not sure how the conversion from Starlark _lcov_merger to native $lcov_merger or :lcov_merger is happening. @laurentlb how can a Starlark private attribute be converted to a native late bound attribute (_x -> :x)?

I'll address the other questions on #6293.

@laurentlb
Copy link
Contributor

This depends on the default value of the attribute. See createAttribute in SkylarkAttr.java.

cc @c-parsons

laurentlb pushed a commit that referenced this issue Jun 27, 2019
…Builder.

384e1cb renamed an attribute from `$lcov_merger` to `:lcov_merger`, which is a breaking change and broke rules_go. This PR changes the test runner to check both attribute names for backwards compatibility. The next step is to introduce an incompatible flag to remove the former.

Fixes #8670

Closes #8709.

PiperOrigin-RevId: 254915099
siberex pushed a commit to siberex/bazel that referenced this issue Jul 4, 2019
…Builder.

384e1cb renamed an attribute from `$lcov_merger` to `:lcov_merger`, which is a breaking change and broke rules_go. This PR changes the test runner to check both attribute names for backwards compatibility. The next step is to introduce an incompatible flag to remove the former.

Fixes bazelbuild#8670

Closes bazelbuild#8709.

PiperOrigin-RevId: 254915099
laurentlb pushed a commit that referenced this issue Jul 8, 2019
…Builder.

384e1cb renamed an attribute from `$lcov_merger` to `:lcov_merger`, which is a breaking change and broke rules_go. This PR changes the test runner to check both attribute names for backwards compatibility. The next step is to introduce an incompatible flag to remove the former.

Fixes #8670

Closes #8709.

PiperOrigin-RevId: 254915099
irengrig pushed a commit to irengrig/bazel that referenced this issue Jul 15, 2019
…Builder.

384e1cb renamed an attribute from `$lcov_merger` to `:lcov_merger`, which is a breaking change and broke rules_go. This PR changes the test runner to check both attribute names for backwards compatibility. The next step is to introduce an incompatible flag to remove the former.

Fixes bazelbuild#8670

Closes bazelbuild#8709.

PiperOrigin-RevId: 254915099
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Rules-Java Issues for Java rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants