-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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", |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
This PR changes the diagnostics we emit when the extractor's call to
packages.Load
returns no packages:There are also changes to the integration tests to exercise this behaviour:
mixed-layout
test now includes a module with a Go source file for which no packages are extracted, due to a build constraint.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