proposal: cmd/go: go build -json #62067
Comments
|
Thanks for the detailed proposal. I noticed currently there's no discussion of exit codes here. It's a small detail, but still fairly important in that each future consumer of the JSON will need to process the exit code before they can begin to process JSON events. Perhaps the proposal is to inherit the same exit codes as To use the existing (This is a comparatively minor detail, I hope not to distract too much from the more important aspects of the proposal.) |
I wasn't planning on differing from the go command's current exit codes. And there could be JSON output even if it exits with 0. Certainly if there are build failures, there will be JSON output and a non-zero exit code. But, for example, if you pass certain debug flags to the compiler, it will emit output that should appear in JSON even on a successful build. Also, if we do decide to emit events on "successful" builds, as I mentioned in the "Open issues" section, that would also potentially emit JSON and then exit with a 0 status, so I certainly wouldn't want to lock us in to some rule about the relationship between the exit status and emitting JSON. |
|
Change https://go.dev/cl/529120 mentions this issue: |
…son' In 'go test -json' we expect stdout to contain only JSON events, not unstructured text. Unstructured text should either go to stderr or be wrapped in a JSON event. (If we add structured build output in #62067, we can emit this output as a build event instead of a test event.) Fixes #35169. For #54378. Change-Id: Ibedd28e79b5adf8d6ae56165b9f0393b14ece9aa Reviewed-on: https://go-review.googlesource.com/c/go/+/529120 Reviewed-by: Austin Clements <austin@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
Change https://go.dev/cl/529220 mentions this issue: |
|
Change https://go.dev/cl/529218 mentions this issue: |
|
Change https://go.dev/cl/529219 mentions this issue: |
|
This proposal has been added to the active column of the proposals project |
|
Change https://go.dev/cl/534857 mentions this issue: |
|
Change https://go.dev/cl/535016 mentions this issue: |
|
Change https://go.dev/cl/536096 mentions this issue: |
|
Change https://go.dev/cl/536097 mentions this issue: |
|
Change https://go.dev/cl/536098 mentions this issue: |
|
Change https://go.dev/cl/536095 mentions this issue: |
…reportCmd Many uses of showOutput, formatOutput, and processOutput follow a very similar (somewhat complex) pattern. Places that diverge from this pattern are often minor bugs. Furthermore, the roles of formatOutput and processOutput have somewhat blurred over time; e.g., formatOutput performs directory shortening, while processOutput performs cgo demangling. This CL consolidates all of this logic into a single, new function: Builder.reportCmd. In the following CL, we'll replace all calls of the three original functions with reportCmd. In addition to being a nice cleanup, this puts us in a much better position to change how build output is formatted in order to support `go build -json`. For #62067. Change-Id: I733162825377d82d0015c8aae2820e56a1b32958 Reviewed-on: https://go-review.googlesource.com/c/go/+/529218 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The general pattern is to replace
if len(cmdOut) > 0 {
output := b.processOutput(cmdOut)
if err != nil {
err = formatOutput(b.WorkDir, dir, p.ImportPath, desc, output)
} else {
b.showOutput(a, dir, desc, output)
}
}
if err != nil {
return err
}
with
if err := b.reportCmd(a, p, desc, dir, cmdOut, err); err != nil {
return err
}
However, there is a fair amount of variation between call sites. The
most common non-trivial variation is sites where errors are an
expected outcome. In this case, often we simply pass "nil" for the
error to trigger only the printing behavior of reportCmd.
For #62067, but also a nice cleanup on its own.
Change-Id: Ie5f918017c02d8558f23ad4c38261077c0fa4ea3
Reviewed-on: https://go-review.googlesource.com/c/go/+/529219
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
This functions have been replaced with Builder.reportCmd. For #62067. Change-Id: Ifeccee720b3da3dc44c49fe11da1eca7b5f46551 Reviewed-on: https://go-review.googlesource.com/c/go/+/529220 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com>
Currently, fmtcmd may have the side effect of updating Builder.scriptDir, the logical working directory of the printed script. If it does so, it also returns a two line command consisting of both a "cd" into the new scriptDir and the original command. When fmtcmd is used as part of Showcmd, that's fine, but fmtcmd is also used in a handful of places to construct command descriptions that are ultimately passed to Builder.reportCmd. In these cases, it's surprising that fmtcmd has any side effects, but the bigger problem is that reportCmd isn't expecting a two-line description and will print it wrong in the output. One option is to fix printing multi-line descriptions in reportCmd, but we can fix the surprise side effect too by instead moving the working directory update to Showcmd. With this CL, fmtcmd merely consults the working directory to shorten it in the output and does not update it. For #62067. Change-Id: I7808b279a430551f4ba51545417adf0bb132f931 Reviewed-on: https://go-review.googlesource.com/c/go/+/534857 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Austin Clements <austin@google.com>
There are several functions that take both an Action argument and a Package argument. It takes a decent amount of work to determine that in all cases the value of the Package argument is just Action.Package. This makes these Package arguments both redundant and potentially confusing because it makes these APIs look like they have more flexibility than they actually do. Drop these unnecessary Package arguments. For #62067. Change-Id: Ibd3295cf6a79d95ceb421d60671f87e023517f8d Reviewed-on: https://go-review.googlesource.com/c/go/+/536095 Reviewed-by: Bryan Mills <bcmills@google.com> Auto-Submit: Austin Clements <austin@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Now that we've dropped the redundant Package arguments to many functions, we can see that the Package argument to reportCmd is always nil. That means we can drop it and always use a.Package. For #62067. Change-Id: I2e11e770f495d6f770047993358c76b08204e923 Reviewed-on: https://go-review.googlesource.com/c/go/+/536096 Auto-Submit: Austin Clements <austin@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
Change https://go.dev/cl/536397 mentions this issue: |
|
Change https://go.dev/cl/536395 mentions this issue: |
|
Change https://go.dev/cl/536399 mentions this issue: |
|
Change https://go.dev/cl/536398 mentions this issue: |
|
Change https://go.dev/cl/536396 mentions this issue: |
This CL separates running shell commands and doing shell-like operations out of the Builder type and into their own, new Shell type. Shell is responsible for tracking output streams and the Action that's running commands. Shells form a tree somewhat like Context, where new Shells can be derived from a root shell to adjust their state. The primary intent is to support "go build -json", where we need to flow the current package ID down to the lowest level of command output printing. Shell gives us a way to easily flow that context down. However, this also puts a clear boundary around how we run commands, removing this from the rather large Builder abstraction. For #62067. Change-Id: Ia9ab2a2d7cac0269ca627bbb316dbd9610bcda44 Reviewed-on: https://go-review.googlesource.com/c/go/+/535016 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com> Auto-Submit: Austin Clements <austin@google.com>
…reportCmd Many uses of showOutput, formatOutput, and processOutput follow a very similar (somewhat complex) pattern. Places that diverge from this pattern are often minor bugs. Furthermore, the roles of formatOutput and processOutput have somewhat blurred over time; e.g., formatOutput performs directory shortening, while processOutput performs cgo demangling. This CL consolidates all of this logic into a single, new function: Builder.reportCmd. In the following CL, we'll replace all calls of the three original functions with reportCmd. In addition to being a nice cleanup, this puts us in a much better position to change how build output is formatted in order to support `go build -json`. For golang#62067. Change-Id: I733162825377d82d0015c8aae2820e56a1b32958 Reviewed-on: https://go-review.googlesource.com/c/go/+/529218 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The general pattern is to replace
if len(cmdOut) > 0 {
output := b.processOutput(cmdOut)
if err != nil {
err = formatOutput(b.WorkDir, dir, p.ImportPath, desc, output)
} else {
b.showOutput(a, dir, desc, output)
}
}
if err != nil {
return err
}
with
if err := b.reportCmd(a, p, desc, dir, cmdOut, err); err != nil {
return err
}
However, there is a fair amount of variation between call sites. The
most common non-trivial variation is sites where errors are an
expected outcome. In this case, often we simply pass "nil" for the
error to trigger only the printing behavior of reportCmd.
For golang#62067, but also a nice cleanup on its own.
Change-Id: Ie5f918017c02d8558f23ad4c38261077c0fa4ea3
Reviewed-on: https://go-review.googlesource.com/c/go/+/529219
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
This functions have been replaced with Builder.reportCmd. For golang#62067. Change-Id: Ifeccee720b3da3dc44c49fe11da1eca7b5f46551 Reviewed-on: https://go-review.googlesource.com/c/go/+/529220 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com>
Currently, fmtcmd may have the side effect of updating Builder.scriptDir, the logical working directory of the printed script. If it does so, it also returns a two line command consisting of both a "cd" into the new scriptDir and the original command. When fmtcmd is used as part of Showcmd, that's fine, but fmtcmd is also used in a handful of places to construct command descriptions that are ultimately passed to Builder.reportCmd. In these cases, it's surprising that fmtcmd has any side effects, but the bigger problem is that reportCmd isn't expecting a two-line description and will print it wrong in the output. One option is to fix printing multi-line descriptions in reportCmd, but we can fix the surprise side effect too by instead moving the working directory update to Showcmd. With this CL, fmtcmd merely consults the working directory to shorten it in the output and does not update it. For golang#62067. Change-Id: I7808b279a430551f4ba51545417adf0bb132f931 Reviewed-on: https://go-review.googlesource.com/c/go/+/534857 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Austin Clements <austin@google.com>
There are several functions that take both an Action argument and a Package argument. It takes a decent amount of work to determine that in all cases the value of the Package argument is just Action.Package. This makes these Package arguments both redundant and potentially confusing because it makes these APIs look like they have more flexibility than they actually do. Drop these unnecessary Package arguments. For golang#62067. Change-Id: Ibd3295cf6a79d95ceb421d60671f87e023517f8d Reviewed-on: https://go-review.googlesource.com/c/go/+/536095 Reviewed-by: Bryan Mills <bcmills@google.com> Auto-Submit: Austin Clements <austin@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Now that we've dropped the redundant Package arguments to many functions, we can see that the Package argument to reportCmd is always nil. That means we can drop it and always use a.Package. For golang#62067. Change-Id: I2e11e770f495d6f770047993358c76b08204e923 Reviewed-on: https://go-review.googlesource.com/c/go/+/536096 Auto-Submit: Austin Clements <austin@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL separates running shell commands and doing shell-like operations out of the Builder type and into their own, new Shell type. Shell is responsible for tracking output streams and the Action that's running commands. Shells form a tree somewhat like Context, where new Shells can be derived from a root shell to adjust their state. The primary intent is to support "go build -json", where we need to flow the current package ID down to the lowest level of command output printing. Shell gives us a way to easily flow that context down. However, this also puts a clear boundary around how we run commands, removing this from the rather large Builder abstraction. For golang#62067. Change-Id: Ia9ab2a2d7cac0269ca627bbb316dbd9610bcda44 Reviewed-on: https://go-review.googlesource.com/c/go/+/535016 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com> Auto-Submit: Austin Clements <austin@google.com>
|
Austin has prepared a stack of CLs and is now on leave. To move forward with the proposal I think we need someone to summarize the new JSON form. |
I propose we add structured JSON output to
go build, similar to and compatible with the structured JSON output ofgo test -json. This JSON output will be enabled by a newgo build -jsonflag, and also implicitly enabled for all builds done on behalf ofgo test -json. This proposal aims to address #23037 and #35169, as well as improve structured all.bash output (#37486).The main motivation for this proposal is to enable build output that is consistent and compatible with
go test -jsonoutput. Currently, if a test or imported package fails to build duringgo test -json, the build error text will be interleaved with the JSON output of tests. Furthermore, there’s currently no way to reliably associate a build error with the test package or packages it affected. This creates unnecessary friction and complexity in tools that consume thego test -jsonoutput.For reference, the
go test -jsonformat is as follow:A detailed description can be found in
go doc test2json.Proposal
There are two related parts to this proposal.
-jsonflag togo buildthat suppresses the text build errors written to stderr and instead writes a JSON-encoded stream to stdout described by the following type:The
PackageIDfield gives the package ID of the package being built. This matches the somewhat misnamedPackage.ImportPathfield ofgo list -jsonand what golang.org/x/tools/go/packages calls the “ID”. This differs from theTestEvent.Packagefield, which is a plain import path, but using the full package ID is important for disambiguating errors from different builds and for consistency with the rest of the build process.The
Actionfield is one of the following:The
Outputfield is set forAction == "build-output"and is defined exactly the same way as theTestEvent.Outputfield. A single event may contain one or more lines of output and there may be more than one output event for a given package ID.This struct is designed so that parsers can distinguish interleaved TestEvents and BuildEvents by inspecting the
Actionfield. Furthermore, as with TestEvent, parsers can simply concatenate theOutputfields of all events to reconstruct the text format output, as it would have appeared fromgo buildwithout the-jsonflag. (As a consequence, this means the output includes all post-processing done by the go tool, including path rewriting and adding the “# package” header before each package.)Environmental errors (e.g., bad GOOS values, file system errors) and module-level errors (e.g., syntax errors in go.mod) will still be printed in plain text to stderr by the go command. These errors cause an immediate exit and happen very early, so they should never appear with JSON output.
FailedBuildfield toTestEventto connect tests that fail because of build errors back to the build error. When a test fails due to a build error, the go command would emit the usual "start" event for the test package, then an "output" event giving the "FAIL package name [build failed]" or equivalent message, followed by a "fail" event with the newFailedBuildfield set to the package ID of the package that failed to build.This same approach can be used for setup failures as well, which print a "FAIL package name [setup failed]" message. Setup failures reflect errors that prevent the go command from even invoking the compiler, such as errors in imports, but they are still logically build failures.
This approach to reporting build failures that affect tests tries to balance several considerations:
Example
Given a test package with a simple build error, currently
go test -jsonemits the following:This is, in fact, identical to the output without the
-jsonflag.With the proposal,
go test -jsonwould instead print:Open issues and questions
Should we emit BuildEvents for successful package builds, like we emit TestEvents for passing tests? Those could include useful information like timing. If we think of this as akin to verbose output, do we emit start events, too? I did not include these in the proposal because they aren’t necessary to solve the immediate problems of processing
go test -jsonoutput, but we could add these events in the future.We could emit environmental and module-level errors in JSON. Above I proposed that these continue to be printed in text to stderr because they are fatal and happen before any build errors that would be printed in JSON, making them easy for tooling to process as text. From an implementation standpoint, there are also simply a huge number of possible causes for early, fatal errors, not all of which are even under the control of the go command, making it difficult to capture all of them. If we wanted to capture more errors, we would have to complicate
BuildEventto specify their sources, probably by making thePackagefield optional and adding an optionalModulefield. Module-level errors would populate theModulefield and environmental errors would omit both fields. This all seems like unnecessary complexity.BuildEvent.PackageIDandTestEvent.Packageare defined slightly differently. There are good reasons for this (given above), and I chose different field names to help make this clear, but it may be confusing for consumers.Finally, we could make the JSON build output much more structured, for example by encoding path and line information of errors as JSON fields. We chose to simply wrap the text output of the compiler because of the degree of variation in compiler output, including sub-errors and optional debug output.
The text was updated successfully, but these errors were encountered: