-
Notifications
You must be signed in to change notification settings - Fork 18k
x/tools/go/packages: publish DriverRequest, DriverResponse #64608
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
Comments
Change https://go.dev/cl/547977 mentions this issue: |
Fixes golang/go#34341 Updates golang/go#64608 Change-Id: I3b41660c12942fdb15b8ea6f1f3c0b77ae91b897 Reviewed-on: https://go-review.googlesource.com/c/tools/+/547977 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Matloob <matloob@golang.org> Auto-Submit: Alan Donovan <adonovan@google.com>
Based on the discussion above, this proposal seems like a likely accept. Details: Export the DriverRequest and DriverResponse structs that already exist (unexported) in the package and are the definition of the wire format. |
No change in consensus, so accepted. 🎉 Details: Export the DriverRequest and DriverResponse structs that already exist (unexported) in the package and are the definition of the wire format. |
Change https://go.dev/cl/557056 mentions this issue: |
Proposal Details
According to #34341 (comment), the lack of public documentation on GOPACKAGESDRIVER is merely an oversight, that I plan to fix in https://go.dev/cl/547977. Internal documentation was added in CL https://go.dev/cl/184943, but the Go types that define the JSON protocol are unexported, meaning users must copy them (e.g. here https://github.com/bazelbuild/rules_go/wiki/Editor-and-tool-integration)
I propose to export them by capitalizing their names and tidying their documentation:
Note: the current internal documentation and behavior support both
os.Getenv("GOPACKAGESDRIVER")
andos.LookPath("gopackagesdriver")
as ways of locating the driver. I'm not convinced that the latter mechanism is desirable (though it may be in current use within Google for Blaze; I should check) and I propose that we drop it.The text was updated successfully, but these errors were encountered: