Add support for bazel coverage with BwoB

[fmeum]

With --experimental_split_coverage_postprocessing and experimental_fetch_all_coverage_outputs, all that is needed to get coverage runs to succeed with --remote_download_minimal is to explicitly request the raw and processed coverage files. This is made possible by the machinery introduced for
--experimental_remote_download_regex.

Work towards #4685

[fmeum]

@coeuvre @c-mita Could you take a look? It's a kind of a cross-team PR and I would really like someone to check my regexes actually match all coverage files (and as few unrelated files as possible).

[coeuvre]

I am not sure whether using regex to pass the information around is the best option. As I mentioned in #6862 (comment), BwoB will be able to download output groups, can we somehow define an output group for those files?

[fmeum]

I am not sure whether using regex to pass the information around is the best option. As I mentioned in #6862 (comment), BwoB will be able to download output groups, can we somehow define an output group for those files?

I agree that regexes are less than ideal. They have the benefit of working with minimal though - would we be able to force downloading a particular output group even with that setting or would it require toplevel?

I will look into the output group approach, I can't say yet whether it's feasible.

[coeuvre]

the problem with minimal is the requested files (either through regex or output groups) are only downloaded if the generating actions didn't hit the skyframe cache.

In an incremental build, if you only change the requested regex, no additional intermediate files will be downloaded. However, toplevel always works. We can force minimal to download requested output groups but it has the same problem with regex.

[fmeum]

It seems to be possible for the coverage reports created for individual tests to be included in an output group. We may need to explicitly fetch bazel-out/_coverage/_coverage_report.dat as that seems to be requested right from within Skyframe/BuildView and isn't associated with a target, but it would be a single static path rather than a regex.

@coeuvre Do you have a branch with a prototype of the output group fetching functionality that I could use to test this approach with?

[coeuvre]

not yet, but I plan to create a PR for that in a few days.

[fmeum]

not yet, but I plan to create a PR for that in a few days.

Great, thanks. Just ping me when you have something. I will set this PR to draft for now.

[coeuvre]

The PR #16524 is merged. It now listens to TestAttempt event and downloads all the available test outputs.

You can play with it but it may already be able to download coverage outputs in which case can you add an integration test for it?

[fmeum]

@coeuvre Just tested it, but the coverage outputs aren't downloaded. How would I go about registering them as test outputs? I have a commit that adds them to a new output group, but how would I register that with the downloader?

[coeuvre]

The TestAttempt event is fired here. You can follow the testOutputs to find where to register them.

If you think coverage outputs should be always downloaded by default if they exist, maybe just add them to the test output instead of using output group.

[fmeum]

It looks like coverage artifacts and reports are already declared outputs of the test action. However, the spawn that is supposed to take the coverage artifacts and generates a report from them fails to do so as it doesn't see the coverage artifacts.

@coeuvre If a remotely run SimpleSpawn declares a tree artifact as an input that is a declared output of a remotely run test, does it need to do anything special to "see" the files in the tree artifact? The files are there in RemoteExecutionService#injectRemoteArtifacts, but aren't available on the remote executor when the SimpleSpawn is executed. How is the relation between the individual injected files and the original tree artifact tracked?

[coeuvre]

It looks like coverage artifacts and reports are already declared outputs of the test action.

Yes, I remember it is the case so I said it may already works, but apparently, there are some other issues.

does it need to do anything special to "see" the files in the tree artifact?

I think it should just work otherwise there are some bugs in RemoteActionFileSystem.

Can you please share a minimal repro so I can look into?

/cc @tjgq

[fmeum]

The integration tests added by this PR are reproducers (even if you replace --remote_download_minimal with --remote_download_toplevel).

I debugged this for a bit and think that the problem is the way the coverage tree artifact is created without properly registering it with Skyframe. That means that SpawnInputExpander is not able to expand it and then remote execution stages it as an empty directory. Doesn't seem to be an issue on the RBE side.

[fmeum]

Adding slightly more detail: You assessed the situation in #15276 (comment). Either the spawn manually expands the tree artifact and doesn't pick up files that only exist remotely, or it sends a tree artifact that SpawnInputExpander doesn't know about und thus can't expand.

Not sure what the right direction is. Maybe looking into how tree artifacts created during the execution of the action could be injected into SpawnInputExpander?

[coeuvre]

I have a working patch that just read tree artifacts from action fs. I will create a PR soon.

[fmeum]

Superseded by #16556