Skip to content
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

cmd/go: go mod download -json does not output json when sumdb fails #34485

Closed
marwan-at-work opened this issue Sep 24, 2019 · 7 comments
Closed

cmd/go: go mod download -json does not output json when sumdb fails #34485

marwan-at-work opened this issue Sep 24, 2019 · 7 comments
Milestone

Comments

@marwan-at-work
Copy link
Contributor

@marwan-at-work marwan-at-work commented Sep 24, 2019

Summary

go mod download -json still returns a valid json when the command fails.

For example, if you run go mod download -json github.com/alsdkfjjsdkfjk/alskdfjksdjf@v1.0.0 then Stdout receives a valid json with the error message saying "module not found".

However, say you have a private repo you'd like to go mod download -json, but you haven't configured GOPRIVATE/GONOSUMDB correctly, then the command fails as expected, but the output is not json.

Digging a little more, I see that most sumdb-checking functions just call base.Fatalf and not actually return an error.

Here's what I had to do to fix it: marwan-at-work@f143bce

I'd be happy to submit the commit above as a CL if it looks okay as a fix

What version of Go are you using (go version)?

1.13

Does this issue reproduce with the latest release?

Yes

What did you do?

GONOSUMDB='' go mod download -json <private-repo>@<version>

What did you expect to see?

A valid json with the Error field describing that the sumdb check has failed.

What did you see instead?

A non-valid-json with the expected error message

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 24, 2019

That seems reasonable to me, but I'd like to confirm with @FiloSottile or @rsc .

(CC @jayconrod)

@bcmills bcmills added this to the Go1.14 milestone Sep 24, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 24, 2019

There are always going to be ways the go command fails and prints to standard error without producing JSON on standard output. Why not treat this failure like any other such failure? Why does it have to be encoded in JSON on standard output?

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 24, 2019

Another option is maybe base.Fatalf should be printing the JSON during -json mode. That would be a much smaller patch.

@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

@marwan-at-work marwan-at-work commented Sep 24, 2019

There are always going to be ways the go command fails and prints to standard error without producing JSON on standard output

Does that mean that go proxies should always try to parse json from stdout (on failure and success) and if the parsing fails they need to read stderr as the alternate failure message?

Why not treat this failure like any other such failure? Why does it have to be encoded in JSON on standard output?

That's kind of what I expected. "Any other such failure" does produce valid json, therefore I expected the sumdb failure to also behave similarly.

For example the following command fails, but produces valid json:

go mod download -json github.com/asdfasdf/asdfasdfasdf@v1.1.0
go: finding github.com/asdfasdf/asdfasdfasdf v1.1.0
go: finding github.com/asdfasdf/asdfasdfasdf v1.1.0
{
	"Path": "github.com/asdfasdf/asdfasdfasdf",
	"Version": "v1.1.0",
	"Error": "github.com/asdfasdf/asdfasdfasdf@v1.1.0: invalid version: unknown revision v1.1.0"
}

While the same exact command fails (for a different reason), but does not produce a valid json:

go mod download -json github.com/marwan-at-work/umlaut@latest
go: finding github.com/marwan-at-work/umlaut latest
verifying github.com/marwan-at-work/umlaut@v0.0.0-20180801060001-9b04c86ca3e2/go.mod: github.com/marwan-at-work/umlaut@v0.0.0-20180801060001-9b04c86ca3e2/go.mod: reading https://sum.golang.org/lookup/github.com/marwan-at-work/umlaut@v0.0.0-20180801060001-9b04c86ca3e2: 410 Gone

umlaut is a test private repo in my github account.

Another option is maybe base.Fatalf should be printing the JSON during -json mode. That would be a much smaller patch.

The issue is that some json data will be lost or may not even be the same shape, for example: go mod download -json returns a json of this shape https://github.com/golang/go/blob/master/src/cmd/go/internal/modcmd/download.go#L59:L70

While go list can return either of these:
https://github.com/golang/go/blob/master///src/cmd/go/internal/load/pkg.go#L51:6
https://github.com/golang/go/blob/master///src/cmd/go/internal/modinfo/info.go#L12:6

So I imagine base.Fatalf will probably have its own json shape that describes the error and potential extra information.

That sounds good but the issue is that at this moment, running go mod download -json and running go list -m may "fail" through a base.Fatalf but may also fail through a propagated return err. Which means go proxies will have to continue guessing as to what the json shape will be.

If the patch is to change all errors to use base.Fatalf shape, then that will be consistent, but I imagine that's an equally large change to the codebase.

But maybe you had something else in mind for how the go proxies should deal with go mod download -json and go list -m failures?

Thanks for the quick response!

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 24, 2019

I agree that proxies should be robust to missing JSON output.

On the other hand, I suspect that we will need to convert many calls to base.Fatalf to return structured errors anyway, both so that they can be diagnosed more easily (the point of failure doesn't always have enough contextual information, such as the import or requirement path by which the failed module was reached), and so that explicitly error-tolerant tools such as gopls can degrade gracefully when working on incomplete or inconsistent code.

So I'm not sure that go mod download -json in particular is a good motivating example, but we may need to fix it incidentally anyway.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Sep 24, 2019

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 25, 2019

Change https://golang.org/cl/197062 mentions this issue: cmd/go: consistent output for -json failures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.