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: document that go mod download -json uses exit code 1 when at least one JSON entry contains non-zero Error #35380

Open
dmitshur opened this issue Nov 5, 2019 · 6 comments

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 5, 2019

go mod download -json documentation says:

[...] The -json flag causes download to print a sequence of JSON objects to standard output, describing each downloaded module (or failure), corresponding to this Go struct:

type Module struct {
	...
	Error string // error loading module
	...
}

What did you do?

I tried to execute go mod download -json and interpret its results programmatically.

What did you expect to see?

I expected non-zero exit code to mean a fatal problem occurred and that I should not expect valid JSON output.

What did you see instead?

There are 3 possible scenarios:

  1. When there are no errors, go mod download -json emits exit code 0 and prints JSON.

  2. When there are errors downloading some modules, go mod download -json emits exit code 1 and prints JSON that has some entries with non-empty Error fields.

    Example

    An example of such an error is when GOPROXY is set to off; it results in "module lookup disabled by GOPROXY=off" errors:

    $ cat go.mod
    module example.com/m
    require golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a
    $ echo $GOPROXY
    off
    $ go mod download -json
    {
    	"Path": "golang.org/x/sys",
    	"Version": "v0.0.0-20190215142949-d0b11bdaac8a",
    	"Error": "module lookup disabled by GOPROXY=off"
    }
    $ echo $?
    1
    
  3. When there are some other type of errors, go mod download -json emits exit code 1 and does not print JSON.

    Example

    Examples of other errors include a syntax error in the current go.mod file, or a replace directive pointing to a directory without a go.mod file.

    $ cat go.mod
    bad
    $ go mod download -json
    go: errors parsing go.mod:
    /tmp/issue35380/go.mod:1: unknown directive: bad
    $ echo $?
    1
    

In two of the scenarios the exit code is 1. One of them emits JSON. That is challenging for a tool that needs to parse the output; it's hard to be sure it's safe to expect/parse JSON on stdout when exit code is non-zero.

/cc @jayconrod @bcmills @marwan-at-work

@dmitshur
Copy link
Member Author

@dmitshur dmitshur commented Nov 5, 2019

I checked scenario 3 just now, and the error it emits goes to stderr. Its stdout is empty, which is a valid sequence of JSON objects (and so it will not produce an error when trying to JSON decode it).

@dmitshur dmitshur assigned dmitshur and unassigned dmitshur Nov 5, 2019
@dmitshur
Copy link
Member Author

@dmitshur dmitshur commented Nov 5, 2019

I guess the remaining usability problem is that it's still necessary for caller to parse as JSON even when exit code is 1. And exit code 1 may mean "there are JSON objects to be read and processed", or it could mean "there was a fatal error that prevented go mod download from starting", and it's not easy to tell those apart.

@jayconrod jayconrod added this to the Backlog milestone Nov 5, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Nov 6, 2019

Related: #34485.

@bcmills
Copy link
Member

@bcmills bcmills commented Nov 6, 2019

@dmitshur, the easy way to tell those two cases apart should be to check whether the output to stderr was non-empty. (If there is any data written to stdout, then it should be valid, but we do not guarantee to write anything on error.)

@dmitshur
Copy link
Member Author

@dmitshur dmitshur commented Jan 16, 2020

I wanted to follow up here with an update. After reading the relevant source code very closely:

if *downloadJSON {
for _, m := range mods {
b, err := json.MarshalIndent(m, "", "\t")
if err != nil {
base.Fatalf("%v", err)
}
os.Stdout.Write(append(b, '\n'))
if m.Error != "" {
base.SetExitStatus(1)
}
}
} else {

I was able to actually simplify the code that consumed output from go mod download -json. I made use of the fact that if exit code is 0, then I know the Error field will always be empty, and if it's non-zero, then there will be at least one module with a non-zero Error field.

While I still think a different behavior when -json flag may have slightly preferable properties, I doubt that the benefits would outweigh the cost of making a change. Perhaps this issue should be about documenting the current behavior, so that people can rely on it without having to read the source code.

@dmitshur dmitshur changed the title cmd/go: go mod download -json with exit code 1 sometimes prints JSON and sometimes doesn't cmd/go: document that go mod download -json uses exit code 1 when at least one JSON entry contains non-zero Error Jan 16, 2020
@perillo
Copy link

@perillo perillo commented Feb 7, 2020

I think returning exit status 1 is the correct thing to do, since there was an error.
The problem is that go list and go mod download do different things and it can be confusing.

The reason it is the right thing to do is that, in case of exit status 1, the caller can simply abort the program ignoring the returned data.

A counter example is golang.org/x/tools/go/packages, where after calling Load, the caller needs to check (usually):

  1. If err != nil, in case of catastrophic error
  2. Call PrintErrors to report the errors to the user (in case of a cli tool), and check if it returns a number > 0

Another counter example is my wrapper of go mod download
https://github.com/perillo/gocmd/blob/master/modfetch/modfetch.go
where, to keep my api consistent (if there are errors, they are in the error value), I need to iterate over the returned modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants