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

Java coverage: fix handling of external files #13377

Closed
wants to merge 1 commit into from

Conversation

ulfjack
Copy link
Contributor

@ulfjack ulfjack commented Apr 19, 2021

When java_test rules are run with coverage, then Bazel writes a file
containing the runtime classpath using root-relative paths.

The collect_coverage.sh script then processes that file to generate
another file containing absolute paths by prefixing each of the paths
with the runfiles directory and the workspace name. It then runs the
singlejar tool on this list of jar files to merge them into a single
file.

This happens to work for all jar files in the main repository because
the runfiles path is the runfiles directory plus the workspace name
plus the root-relative path.

However, it is broken if some of the jar files come from an external
repository. It appears that singlejar errors out on the first such jar
file, generating a partial output jar file without a central directory.
The error is swallowed by the collect_coverage.sh script. Presence of
coverage results may thus depend on the order of classpath entries.

In order to fix this, we change the runtime classpath file to contain
runfiles-relative paths, and the coverage script to prefix them with
the runfiles directory.

Note that we reuse SourceManifestAction here, i.e., the identical code
that is also responsible for generating the runfiles directory in the
first place. This is the only reliable way to get the correct paths
into the test action.

There are more places that use LazyWritePathsFileAction and I suspect
that all of them are broken:

  • persistent test runner support in BazelJavaSemantics (AFAIK, this
    doesn't work in Bazel anyway)
  • coverage w/ metadata jars in BazelJavaSemantics (not used by
    JavaBinary)
  • coverage source list file ('-paths-for-coverage.txt'); this probably
    results in non-functioning coverage for source files in external
    repositories

Fixes #13376.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352

When java_test rules are run with coverage, then Bazel writes a file
containing the runtime classpath using root-relative paths.

The collect_coverage.sh script then processes that file to generate
another file containing absolute paths by prefixing each of the paths
with the runfiles directory and the workspace name. It then runs the
singlejar tool on this list of jar files to merge them into a single
file.

This happens to work for all jar files in the main repository because
the runfiles path is the runfiles directory plus the workspace name
plus the root-relative path.

However, it is broken if some of the jar files come from an external
repository. It appears that singlejar errors out on the first such jar
file, generating a partial output jar file without a central directory.
The error is swallowed by the `collect_coverage.sh` script. Presence of
coverage results may thus depend on the order of classpath entries.

In order to fix this, we change the runtime classpath file to contain
runfiles-relative paths, and the coverage script to prefix them with
the runfiles directory.

Note that we reuse SourceManifestAction here, i.e., the identical code
that is also responsible for generating the runfiles directory in the
first place. This is the only reliable way to get the correct paths
into the test action.

There are more places that use `LazyWritePathsFileAction` and I suspect
that all of them are broken:

- persistent test runner support in BazelJavaSemantics (AFAIK, this
  doesn't work in Bazel anyway)
- coverage w/ metadata jars in BazelJavaSemantics (not used by
  JavaBinary)
- coverage source list file ('-paths-for-coverage.txt'); this probably
  results in non-functioning coverage for *source files* in external
  repositories

Fixes bazelbuild#13376.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352
@ulfjack ulfjack requested a review from lberki as a code owner April 19, 2021 22:26
@google-cla google-cla bot added the cla: yes label Apr 19, 2021
@lberki lberki requested review from c-mita and removed request for lberki April 20, 2021 06:37
@lberki
Copy link
Contributor

lberki commented Apr 20, 2021

Can I haz test case?

@bazel-io bazel-io closed this in 8ba48ad Apr 21, 2021
@ulfjack ulfjack deleted the java-coverage branch April 26, 2021 19:38
katre pushed a commit that referenced this pull request Jul 12, 2021
When java_test rules are run with coverage, then Bazel writes a file
containing the runtime classpath using root-relative paths.

The collect_coverage.sh script then processes that file to generate
another file containing absolute paths by prefixing each of the paths
with the runfiles directory and the workspace name. It then runs the
singlejar tool on this list of jar files to merge them into a single
file.

This happens to work for all jar files in the main repository because
the runfiles path is the runfiles directory plus the workspace name
plus the root-relative path.

However, it is broken if some of the jar files come from an external
repository. It appears that singlejar errors out on the first such jar
file, generating a partial output jar file without a central directory.
The error is swallowed by the `collect_coverage.sh` script. Presence of
coverage results may thus depend on the order of classpath entries.

In order to fix this, we change the runtime classpath file to contain
runfiles-relative paths, and the coverage script to prefix them with
the runfiles directory.

Note that we reuse SourceManifestAction here, i.e., the identical code
that is also responsible for generating the runfiles directory in the
first place. This is the only reliable way to get the correct paths
into the test action.

There are more places that use `LazyWritePathsFileAction` and I suspect
that all of them are broken:

- persistent test runner support in BazelJavaSemantics (AFAIK, this
  doesn't work in Bazel anyway)
- coverage w/ metadata jars in BazelJavaSemantics (not used by
  JavaBinary)
- coverage source list file ('-paths-for-coverage.txt'); this probably
  results in non-functioning coverage for *source files* in external
  repositories

Fixes #13376.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352

Closes #13377.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352
PiperOrigin-RevId: 369687409
katre pushed a commit to katre/bazel that referenced this pull request Jul 13, 2021
When java_test rules are run with coverage, then Bazel writes a file
containing the runtime classpath using root-relative paths.

The collect_coverage.sh script then processes that file to generate
another file containing absolute paths by prefixing each of the paths
with the runfiles directory and the workspace name. It then runs the
singlejar tool on this list of jar files to merge them into a single
file.

This happens to work for all jar files in the main repository because
the runfiles path is the runfiles directory plus the workspace name
plus the root-relative path.

However, it is broken if some of the jar files come from an external
repository. It appears that singlejar errors out on the first such jar
file, generating a partial output jar file without a central directory.
The error is swallowed by the `collect_coverage.sh` script. Presence of
coverage results may thus depend on the order of classpath entries.

In order to fix this, we change the runtime classpath file to contain
runfiles-relative paths, and the coverage script to prefix them with
the runfiles directory.

Note that we reuse SourceManifestAction here, i.e., the identical code
that is also responsible for generating the runfiles directory in the
first place. This is the only reliable way to get the correct paths
into the test action.

There are more places that use `LazyWritePathsFileAction` and I suspect
that all of them are broken:

- persistent test runner support in BazelJavaSemantics (AFAIK, this
  doesn't work in Bazel anyway)
- coverage w/ metadata jars in BazelJavaSemantics (not used by
  JavaBinary)
- coverage source list file ('-paths-for-coverage.txt'); this probably
  results in non-functioning coverage for *source files* in external
  repositories

Fixes bazelbuild#13376.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352

Closes bazelbuild#13377.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352
PiperOrigin-RevId: 369687409
katre pushed a commit to katre/bazel that referenced this pull request Jul 13, 2021
When java_test rules are run with coverage, then Bazel writes a file
containing the runtime classpath using root-relative paths.

The collect_coverage.sh script then processes that file to generate
another file containing absolute paths by prefixing each of the paths
with the runfiles directory and the workspace name. It then runs the
singlejar tool on this list of jar files to merge them into a single
file.

This happens to work for all jar files in the main repository because
the runfiles path is the runfiles directory plus the workspace name
plus the root-relative path.

However, it is broken if some of the jar files come from an external
repository. It appears that singlejar errors out on the first such jar
file, generating a partial output jar file without a central directory.
The error is swallowed by the `collect_coverage.sh` script. Presence of
coverage results may thus depend on the order of classpath entries.

In order to fix this, we change the runtime classpath file to contain
runfiles-relative paths, and the coverage script to prefix them with
the runfiles directory.

Note that we reuse SourceManifestAction here, i.e., the identical code
that is also responsible for generating the runfiles directory in the
first place. This is the only reliable way to get the correct paths
into the test action.

There are more places that use `LazyWritePathsFileAction` and I suspect
that all of them are broken:

- persistent test runner support in BazelJavaSemantics (AFAIK, this
  doesn't work in Bazel anyway)
- coverage w/ metadata jars in BazelJavaSemantics (not used by
  JavaBinary)
- coverage source list file ('-paths-for-coverage.txt'); this probably
  results in non-functioning coverage for *source files* in external
  repositories

Fixes bazelbuild#13376.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352

Closes bazelbuild#13377.

Change-Id: Ie9bcc92344f06e190efcb192a3b6ef9905aea352
PiperOrigin-RevId: 369687409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coverage for java_test doesn't handle external jars
2 participants