Skip to content

Go: Improve diagnostics when no packages are extracted #17674

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mbg
Copy link
Member

@mbg mbg commented Oct 7, 2024

This PR changes the diagnostics we emit when the extractor's call to packages.Load returns no packages:

  • The diagnostic emitted by the extractor has been downgraded from an error-level diagnostic to a warning-level diagnostic. This is because the extractor may be invoked multiple times in e.g. a multi-project repository where this is fine as long as we are able to extract packages for other modules.
  • The extractor now stores the number of extracted packages in a JSON file in the database scratch directory. The autobuilder can then load this file.
  • If the total number of packages extracted across all extractor runs is zero, then we emit the old error-level diagnostic.

There are also changes to the integration tests to exercise this behaviour:

  • The mixed-layout test now includes a module with a Go source file for which no packages are extracted, due to a build constraint.
  • There is a new no-packages-in-any-module test which has multiple Go modules, each with source files, but no packages are extracted for any of them.

Alternatives considered

I played around with using the rpc package in the standard library instead of using a file to communicate information between the extractor and the autobuilder, but this produced annoying pop-ups on macOS.

Open questions

  • The current file-based approach to exchanging information between the extractor and autobuilder assumes that the extractor is never run more than once at the same time, otherwise there may be race conditions in updating the JSON file. Can we safely make this assumption? If not, we could create a directory instead (similar to diagnostics) and then store multiple JSON files in there for each extractor run.
  • Is storing this data in the database scratch directory fine?

@mbg mbg marked this pull request as ready for review October 7, 2024 12:41
@mbg mbg requested a review from a team as a code owner October 7, 2024 12:41
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check: as written we'll get a minor regression: manual builds will only produce a warning and no error (except the top-level catch-all something-went-wrong message) if no build extracts anything. Recommended solution: our pre-finalize script should sweep for extractor reports, amalgamate them and turn them into diagnostics. Note this will require a small CLI tweak to allow a pre-finalize script to opt in to execution even when no trap files are emitted, which currently causes that script to be skipped.

@@ -5,6 +5,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")
go_library(
name = "util",
srcs = [
"extractor.go",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractionresults.go, to avoid looking like extractor/extractor.go?

// Gets the path of the JSON file in the database scratch directory that is used
// to store extraction results.
func extractionResultsPath() string {
return filepath.Join(ScratchDir(), "extraction.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: rather than have a single json file and a read-modify-write process, instead define a distinctive directory name like $SCRATCH/go-extraction-results where the extractor creates a fresh-named file (use a tempfile library for this), then have the autobuilder or pre-finalize step read whatever files were created and amalgamate them. This would mean there are no concurrency concerns except creating a fresh filename, which the tempfile library already knows how to do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is already a suggestion in the "open questions" section in the PR description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants