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/vet: fix documentation of go vet error code #24477

Open
ad-si opened this Issue Mar 21, 2018 · 20 comments

Comments

Projects
None yet
7 participants
@ad-si

ad-si commented Mar 21, 2018

Following documentation seems to be switched.
As far as I can see go vet returns 1 on program errors and 2 if issues were found in the checked code.

go/src/cmd/vet/doc.go

Lines 22 to 23 in 0c0aed3

Vet's exit code is 2 for erroneous invocation of the tool, 1 if a
problem was reported, and 0 otherwise. Note that the tool does not

@gopherbot

This comment has been minimized.

gopherbot commented Mar 21, 2018

Change https://golang.org/cl/101876 mentions this issue: cmd/vet: fix wrong exit code

@tklauser tklauser changed the title from Fix documentation of go vet error code to cmd/vet: fix documentation of go vet error code Mar 21, 2018

@namusyaka

This comment has been minimized.

Member

namusyaka commented Mar 21, 2018

@ad-si Confirmed. Thanks for your report. I sent a CL for fixing the docs.
/cc @mvdan @randall77 @aclements

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 21, 2018

Are we positive that the code is right and the docs are wrong? In general, I'd assume that the docs are right; exit code 2 when invoking a tool in the incorrect way is what you would expect.

@ad-si

This comment has been minimized.

ad-si commented Mar 21, 2018

@mvdan No, I think you would expect that if a program fails it returns 1. That's what most cli tools do.
E.g. mkdir asdf/asdf/asdf fails with error code 1 as it was used incorrectly.
If you want the errors to have a special meaning you'd use larger error numbers.
In the case of go vet with the meaning: "The tool was executed correctly, but the checked file contains errors.

@namusyaka

This comment has been minimized.

Member

namusyaka commented Mar 21, 2018

Receiving @mvdan's comment, I retried to vet in my locally, and then get 1 code when passing incorrect arguments. However, it's not vet error, but go error. My research seems wrong.

@ad-si Could you give a concrete example to this problem?
Please use the github template.

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 21, 2018

By invoking a tool incorrectly, I mean cases such as bad flags and incorrect arguments. For example:

$ go build -wrongflag; echo $?
flag provided but not defined: -wrongflag
usage: build [-o output] [-i] [build flags] [packages]
Run 'go help build' for details.
2
$ go vet -wrongflag; echo $?
vet: flag "-wrongflag" not defined
Run "go help vet" for more information
2

This particular case seems in line with the doc you quote above.

@ad-si

This comment has been minimized.

ad-si commented Mar 21, 2018

Uhm, then there is something fundamentally wrong.
This is a correct usage of the program and it returns 2

go vet flawed-file.go; echo $?
# command-line-arguments
source/tools/warn-govet/tests/fixtures/dirty.go:4:25: undefined: String
source/tools/warn-govet/tests/fixtures/dirty.go:10:4: no new variables on left side of :=
source/tools/warn-govet/tests/fixtures/dirty.go:11:1: missing return at end of function
2

go vet -wrongflag and go vet flawed-file.go should definitely not return the same code ...

@mvdan

This comment has been minimized.

Member

mvdan commented Mar 21, 2018

I agree that this is a bit confusing. The docs could technically be right, because what you got there isn't vet problems being reported, rather errors when loading and type-checking the program.

And this can be useful; if vet returns code 2, it didn't function properly. If it returns 1, it ran properly and found some issues. And if it returns 0, it ran properly and found no issues. I assume that it's a bit late to change this behavior, since people could depend on it.

Perhaps the docs could be clarified, to avoid confusion. /cc @robpike who originally wrote the docs in question.

@ad-si

This comment has been minimized.

ad-si commented Mar 21, 2018

Ahhh, I should actually be running go tool vet flawed-file.go, because go vet is expecting a path to a package and not just to a file. Then it works correctly (as documented).
Very confusing ^^

@aclements

This comment has been minimized.

Member

aclements commented Mar 21, 2018

@ad-si, those are actually compile errors rather than vet errors. For example, if you run go vet -x flawed-file.go it shows that the go tool hasn't actually invoked go tool vet at that point. I'm not sure why that's exiting with 2, but it's not in the vet tool.

We should probably make this exit 1 so it's consistent with vet errors and the documentation. But, also, let's not overthink this. :)

@aclements

This comment has been minimized.

Member

aclements commented Mar 21, 2018

New proposal: the tools really aren't trying to convey information beyond "success" or "failure" and it's not worth the effort to try to distinguish types of failure because approximately nobody cares about which non-zero status it exited with, so let's just change the documentation to say "zero" and "non-zero".

@ad-si

This comment has been minimized.

ad-si commented Mar 21, 2018

Uhm, I care (a lot). If go vet runs correctly, I can process the warnings, if it crashed I have to rethrow the error and log / process it.

@gopherbot

This comment has been minimized.

gopherbot commented Mar 21, 2018

Change https://golang.org/cl/101918 mentions this issue: cmd/vet: only zero/non-zero exit status matters

@aclements

This comment has been minimized.

Member

aclements commented Mar 21, 2018

@ad-si, this is the first time vet crashing has been mentioned. Could you clarify your use case?

@ad-si

This comment has been minimized.

ad-si commented Mar 21, 2018

We're running go vet in our build pipeline and if go vet finds errors in go code I redirect them to be displayed at another position, but it does not fail our build.

However, if the program itself crashes, my build pipeline is obviously broken and I need to fix it.

@aclements

This comment has been minimized.

Member

aclements commented Mar 21, 2018

Perhaps I'm still unclear. If go vet crashes, it should be pretty obvious to the person looking at the vet output that the tool crashed rather than giving vet output. Also, this would indicate that go vet is broken, not that your build pipeline is broken, so I would want a person in the loop to report that crash to us. If you're not going to fail the build on vet failures, I don't see why you would fail the build on vet crashes.

I would certainly hope vet crashes aren't something that happen regularly enough that you need special tooling around them.

@ad-si

This comment has been minimized.

ad-si commented Mar 21, 2018

Well, there is no human in the loop. That's the point =D.

Also, this would indicate that go vet is broken, not that your build pipeline is broken

Could also mean that go is broken, or that that the file does not exist, or that there was a problem while opening it, or thousands of other things, right? ^^
It sure would be great to know that go vet ran as intended and found problems in the file.

I would certainly hope vet crashes aren't something that happen regularly enough that you need special tooling around them.

Well, I'd certainly hope so too, but I work in software quality / security, and folks have hoped many things ;-)

@andybons

This comment has been minimized.

Member

andybons commented Mar 26, 2018

go vet should not crash if a file doesn’t exist, or if there was a problem opening it, etc. It should exit cleanly with a non-zero exit code:

$ go vet foo.go
stat foo.go: no such file or directory
$ echo $?
1

Maybe there’s a misunderstanding of “crash” vs “exit with a non-zero error code.”

A crash would mean a panic, segfault, etc. Are you seeing crashes? What do they look like?

@andybons andybons added this to the Unplanned milestone Mar 26, 2018

@ad-si

This comment has been minimized.

ad-si commented Mar 27, 2018

Yeah, it shouldn't crash, but it's not guaranteed. So I need to handle the case that it crashes in unforeseeable ways, although it hasn't happened to me yet.

@robpike

This comment has been minimized.

Contributor

robpike commented Mar 28, 2018

If https://go-review.googlesource.com/c/go/+/101918 would land this would be taken care of. Ping @aclements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment