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

remote_coverage_tools/Main reports success on I/O error #21982

Closed
sputt opened this issue Apr 12, 2024 · 3 comments
Closed

remote_coverage_tools/Main reports success on I/O error #21982

sputt opened this issue Apr 12, 2024 · 3 comments
Labels
coverage P2 We'll consider working on this in future. (Assignee optional) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug

Comments

@sputt
Copy link
Contributor

sputt commented Apr 12, 2024

Description of the bug:

If an I/O error occurs when writing coverage.dat via LcovPrinter.java, the exception is swallowed and the return code is not checked in Main.

The exception is converted to a bool here:

The return value is ignored here:

LcovPrinter.print(new FileOutputStream(outputFile), coverage);

This situation can be particularly bad if remote caching is enabled, since a truncated or otherwise invalid lcov database can be uploaded to the cache, since the action succeeded.

Which category does this issue belong to?

Core

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

I originally confirmed the issue by overriding remote_coverage_tools and patching LcovPrinter.print to return false unconditionally, which sure enough, produces a 0 byte lcov database but doesn't fail the action.

Which operating system are you running Bazel on?

Any

What is the output of bazel info release?

release 7.1.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

https://github.com/bazelbuild/bazel.git
d7a9bb5b1e345097820a89fcda106dc7eb7d3fa0

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

This was always a problem.

Have you found anything relevant by searching the web?

No response

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

I suggest we modify LcovPrinter.print with throws IOException. See commit here:
sputt@9897443

@github-actions github-actions bot added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Apr 12, 2024
@fmeum
Copy link
Collaborator

fmeum commented Apr 12, 2024

@c-mita That looks like a simple oversight to me, do you agree?

@c-mita c-mita added P2 We'll consider working on this in future. (Assignee optional) coverage and removed untriaged labels Apr 12, 2024
@c-mita
Copy link
Member

c-mita commented Apr 12, 2024

Yeah that looks like a simple oversight. Probably simplest to just leave the exceptions intact and handle them in the Main class; there doesn't seem to be any value in converting to a boolean; especially as the code seems to be trying to do this twice within the call stack but not actually handling returned boolean.

bazel-io pushed a commit to bazel-io/bazel that referenced this issue Apr 18, 2024
The calling code expects the exception, not a return boolean.

Fixes bazelbuild#21982.

Closes bazelbuild#21987.

PiperOrigin-RevId: 626086576
Change-Id: I4abd7a253715c84c323e036dfbdb2fcb94a4825d
github-merge-queue bot pushed a commit that referenced this issue Apr 18, 2024
The calling code expects the exception, not a return boolean.

Fixes #21982.

Closes #21987.

PiperOrigin-RevId: 626086576
Change-Id: I4abd7a253715c84c323e036dfbdb2fcb94a4825d

Commit
f8277cf

Co-authored-by: Spencer Putt <sputt@alumni.iu.edu>
Kila2 pushed a commit to Kila2/bazel that referenced this issue May 13, 2024
The calling code expects the exception, not a return boolean.

Fixes bazelbuild#21982.

Closes bazelbuild#21987.

PiperOrigin-RevId: 626086576
Change-Id: I4abd7a253715c84c323e036dfbdb2fcb94a4825d
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.2.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.2.0rc1. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage P2 We'll consider working on this in future. (Assignee optional) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug
Projects
None yet
Development

No branches or pull requests

6 participants