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 <[email protected]> Auto-Submit: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
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 <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
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 <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
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 <[email protected]> Reviewed-by: Bryan Mills <[email protected]>
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 <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Austin Clements <[email protected]>
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 <[email protected]> Auto-Submit: Austin Clements <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
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 <[email protected]> Reviewed-by: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
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: |
|
No change in consensus, so accepted. 🎉 Proposal details in #62067 (comment) |
|
Change https://go.dev/cl/558637 mentions this issue: |
Compare: (But I don't think that changes this decision at all.) |
This replaces the existing Shell print function callback. The interface also gives us a way to report build failures, which is the other type of event that will appear in the build -json output. This CL hooks up error reporting in two places: - In Builder.Do, where all builder errors are reported. - In load.CheckPackageErrors, where most loading errors are reported. For #62067. Change-Id: Id66a31b0d2c3786559c7d2bb376fffeffc9a66ed Reviewed-on: https://go-review.googlesource.com/c/go/+/536396 Reviewed-by: Russ Cox <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
This adds support for a "-json" flag in all build-related go subcommands. This causes build output and build failures to be reported to stdout in a machine-readable way. For #62067. Fixes #23037. Change-Id: Id045c5bd5dde9d16cc09dde6248a4b9637896a30 Reviewed-on: https://go-review.googlesource.com/c/go/+/536397 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Russ Cox <[email protected]>
Currently, under *most* circumstances, if there's a package loading error during "go test", that will get reported as a "FAIL p [setup failed]" or "FAIL p [build failed] message and won't prevent running unaffected test packages. However, if there's a loading error from a non-test file in a package listed directly on the "go test" command line, that gets reported as an immediate fatal error, without any "FAIL" line, and without attempting to run other tests listed on the command line. Likewise, certain early build errors (like a package containing no Go files) are currently immediately fatal rather than reporting a test failure. Fix this by eliminating the check that causes that immediate failure. This causes one minor follow-up problem: since load.TestPackagesAndErrors was never passed a top-level package with an error before, it doesn't currently propagate such an error to the packages it synthesizes (even though it will propagate errors in imported packages). Fix this by copying the error from the top-level package into the synthesized test package while we're copying everything else. For #62067. Change-Id: Icd563a3d9912256b53afd998050995e5260ebe5d Reviewed-on: https://go-review.googlesource.com/c/go/+/558637 Reviewed-by: Russ Cox <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Sam Thanawalla <[email protected]>
Currently, each Action tracks whether it failed, which is propagated up from dependencies. Shortly, we'll need to know the root cause if a test fails because of a build failure. To support this, replace the Failed boolean with a Failed *Action that tracks the root Action that failed and caused other Actions to fail. For #62067. Change-Id: I8f84a51067354043ae9531a4368c6f8b11d688d5 Reviewed-on: https://go-review.googlesource.com/c/go/+/536398 Reviewed-by: Russ Cox <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
Change https://go.dev/cl/628955 mentions this issue: |
Unfortunately, this is tripping up the LUCI test output processor, so we need to disable it until we can figure that out. For #70402. Updates #62067. Cq-Include-Trybots: luci.golang.try:gotip-darwin-arm64_13,gotip-linux-amd64-longtest Change-Id: I9ae722218e98b8060b8b4c46358f23381ac8537a Reviewed-on: https://go-review.googlesource.com/c/go/+/628955 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Cherry Mui <[email protected]>
|
Change https://go.dev/cl/629335 mentions this issue: |
This re-enables the behavior of CL 536399 (by effectively reverting CL 628955), so now go test -json again includes build output and failures as JSON rather than text. However, since this behavior is clearly enough to trip up some build systems, this CL includes a GODEBUG=gotestjsonbuildtext that can be set to 1 to revert to the old behavior. Fixes #70402. Updates #62067. Cq-Include-Trybots: luci.golang.try:gotip-darwin-arm64_13,gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Change-Id: I84e778cd844783dacfc83433e391b5ccb5925127 Reviewed-on: https://go-review.googlesource.com/c/go/+/629335 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Auto-Submit: Austin Clements <[email protected]>
|
Change https://go.dev/cl/635899 mentions this issue: |
go test -json has two new effects in Go 1.24: it implies go build -json, and it adds a FailedBuild field in test events. For compatibility, CL 629335 added gotestjsonbuildtext=1, which disables the implicit go build -json, but that CL didn't affect the FailedBuild field. In principle this shouldn't matter because it's just another JSON field, but just so we don't have to worry about some intermediate behavior, this CL makes gotestjsonbuildtext=1 disable the FailedBuild field as well. Updates #62067 Updates #70402 Change-Id: I006d1ea0468980ee2564495324e8b4ed082898af Reviewed-on: https://go-review.googlesource.com/c/go/+/635899 Auto-Submit: Austin Clements <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
(Edited 2024-01-18: Changed
PackageIDtoImportPath.)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
ImportPathfield 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.ImportPathandTestEvent.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: