-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
all: run 'go vet std' during run.bash, disable vetall builder #31916
Comments
For what it's worth, where the vetall builder always catches me is other |
What Ian said. I think that's why Josh added it, too. Don't we already run /cc @josharian |
In addition to what Ian said, 'go vet' doesn't run all the vet checks; the vetall builder does. That's why there is still a set of whitelists, despite 'go vet' running on all packages in std as part of 'go test'. To be clear, I'm definitely in favor of fixing things so that the whitelists eventually disappear, moving vet checks to places that they always get run (#17544), etc. These comments about why we still have a vetall builder are to help clarify scope. |
Change https://golang.org/cl/176104 mentions this issue: |
Change https://golang.org/cl/176111 mentions this issue: |
Change https://golang.org/cl/176106 mentions this issue: |
Change https://golang.org/cl/176105 mentions this issue: |
Change https://golang.org/cl/176099 mentions this issue: |
Change https://golang.org/cl/176100 mentions this issue: |
Change https://golang.org/cl/176102 mentions this issue: |
Change https://golang.org/cl/176103 mentions this issue: |
Change https://golang.org/cl/176110 mentions this issue: |
Change https://golang.org/cl/176108 mentions this issue: |
Change https://golang.org/cl/176107 mentions this issue: |
Change https://golang.org/cl/176101 mentions this issue: |
Change https://golang.org/cl/176109 mentions this issue: |
'go vet' does run all the vet checks, exactly the same ones that the misc-vet-vetall builder does.
I am thinking that either we make
Sure, but that's true of all the builders. If the vet checks are running as part of all.bash, then the builder responsible for the GOOS/GOARCH you broke will report it, like any other system-specific breakage. |
Change https://golang.org/cl/176097 mentions this issue: |
Change https://golang.org/cl/176177 mentions this issue: |
Added note in top comment about concrete plan based on discussion. |
asmdecl: - MOVW $x+0(FP) is OK if x is big, because $x is an address (happens in internal/cpu, golang.org/x/sys/cpu, runtime) - ignore TEXT lines in comments (happens in runtime/internal/atomic) - wasm's CallImport instruction writes return results (happens in syscall) - allow write out-of-bounds (SP) references in NOFRAME functions (happens in runtime) - recognize "NOP SP" as an SP "write" to disable SP bounds checking - 'go test' in passes/asmdecl was not testing all architectures; fix that stdmethods: - ignore WriteTo if obviously not io.WriterTo (as in go/types and runtime/pprof) errorsas: - don't complain about package errors testing invalid calls structtag: - don't complain about encoding/json and encoding/xml testing invalid tags unmarshal: - don't complain about encoding/gob, encoding/json, encoding/xml testing invalid calls For golang/go#31916. Fixes golang/go#25822. Change-Id: I322c08b5991ffc4995112b8ea945161a4c5193ce Reviewed-on: https://go-review.googlesource.com/c/tools/+/176097 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org>
The vetall builder runs vet straight out of golang.org/x/tools, so submiting CL 176097 in that repo will break the builder by making all these whitelist entries stale. Submiting this CL will fix it, by removing them. The addition of the gcWriteBarrier declaration in runtime/stubs.go is necessary because the diagnostic is no longer emitted on arm, so it must be removed from all.txt. Adding it to runtime is better than adding it to every-other-goarch.txt. For #31916. Change-Id: I432f6049cd3ee5a467add5066c440be8616d9d54 Reviewed-on: https://go-review.googlesource.com/c/go/+/176177 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Renaming the method makes clear, both to readers and to vet, that this method is not the implementation of io.Seeker: it cannot fail. Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. For #31916. Change-Id: I3e6ad7264cb0121b4b76935450cccb71d533e96b Reviewed-on: https://go-review.googlesource.com/c/go/+/176108 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Renaming the method makes clear, both to readers and to vet, that this method is not the implementation of io.ByteWriter. Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. For #31916. Change-Id: I79da062ca6469b62a6b9e284c6cf2413c7425249 Reviewed-on: https://go-review.googlesource.com/c/go/+/176109 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
Renaming the method makes clear, both to readers and to vet, that this method is not the implementation of io.ByteWriter. Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. For #31916. Change-Id: I5b509eb7f0118d5f2d3c6e352ff2849cd5a3071e Reviewed-on: https://go-review.googlesource.com/c/go/+/176110 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
These ridiculous tags are testing what happens with very long tags. There is no need for them to be malformed as well. Fix them to make vet happier. Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. For #31916. Change-Id: I100aed5230fcde41676c79c7074c69c16ea4b96d Reviewed-on: https://go-review.googlesource.com/c/go/+/176111 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "GOOS=linux GOARCH=386 go vet -unsafeptr=false runtime" happy, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: I3e5586a7ff6e359357350d0602c2259493280ded Reviewed-on: https://go-review.googlesource.com/c/go/+/176099 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "GOOS=linux GOARCH=amd64 go vet -unsafeptr=false runtime" happy, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: I4ca1acb02f4666b102d25fcc55fac96b8f80379a Reviewed-on: https://go-review.googlesource.com/c/go/+/176100 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "GOOS=linux GOARCH=arm go vet -unsafeptr=false runtime" happy, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: Ifae75b832320b5356ac8773cf85055bfb2bd7214 Reviewed-on: https://go-review.googlesource.com/c/go/+/176101 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
…*, linux/s390x Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "go vet -unsafeptr=false runtime" happy for these GOOS/GOARCHes, except for an unresolved complaint on mips/mipsle that is a bug in vet, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: I6ef7e982a2fdbbfbc22cee876ca37ac54d8109e5 Reviewed-on: https://go-review.googlesource.com/c/go/+/176102 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "go vet -unsafeptr=false runtime" happy for these GOOS/GOARCHes, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: Ic64f7f4034695dd4c32c9b7f258960faf3742a83 Reviewed-on: https://go-review.googlesource.com/c/go/+/176103 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "go vet -unsafeptr=false runtime" happy for these GOOSes, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: Id2e1223497bd0cd6e880cd81f3ece6363e58219f Reviewed-on: https://go-review.googlesource.com/c/go/+/176104 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "go vet -unsafeptr=false runtime" happy for these GOOSes, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: I63c4805bdd44b301072da66c77086940e2a2765e Reviewed-on: https://go-review.googlesource.com/c/go/+/176105 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "go vet -unsafeptr=false runtime" happy for windows/*, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: If37ab2b3f6fca4696b8a6afb2ef11ba6c4fb42e0 Reviewed-on: https://go-review.googlesource.com/c/go/+/176106 Reviewed-by: Austin Clements <austin@google.com>
Working toward making the tree vet-safe instead of having so many exceptions in cmd/vet/all/whitelist. This CL makes "go vet -unsafeptr=false runtime" happy for nacl/*, while keeping "GO_BUILDER_NAME=misc-vetall go tool dist test" happy too. For #31916. Change-Id: I6adb4a7b0c2b03d901fba37f9c05e74e5b7b6691 Reviewed-on: https://go-review.googlesource.com/c/go/+/176107 Reviewed-by: Austin Clements <austin@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Why do we run vet in cmd/compile/internal/gc/testdata? That seems like it would be a place full of things vet would not like (and it is, of course). What are we trying to catch by running vet? |
Historical accident, I believe. When I wrote
I believe that We should probably now do I what I should have done ages ago and systematically ignore |
Change https://golang.org/cl/176397 mentions this issue: |
OK, thanks. When the check moves to 'go test' (or 'go vet std cmd' in buildall.bash for misc-compile) we'll ignore that directory at no extra charge. As of right now it's the only thing left in the whitelist directory. |
The new prototypes of duffzero and duffcopy must be accompanied by functions. Otherwise buildmode=shared (in particular, misc/cgo/testshared) has missing symbols. The right fix, of course, is to implement these on s390x. For #31916. Change-Id: I3efff5e3011956341e1b26223a4847a8a91a0453 Reviewed-on: https://go-review.googlesource.com/c/go/+/176397 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Change https://golang.org/cl/176440 mentions this issue: |
Change https://golang.org/cl/176439 mentions this issue: |
Change https://golang.org/cl/176438 mentions this issue: |
Change https://golang.org/cl/176357 mentions this issue: |
Change https://golang.org/cl/176599 mentions this issue: |
…yzers Otherwise the specific set of gob registrations varies according to the command line, which makes it impossible for a narrow analysis run (for example, just one analyzer) to read fact files written by less narrow runs (for example, all the analyzers). This will start mattering in the standard repo vet. For golang/go#31916. Change-Id: I6fa90b3dfdf28ede6f995db3904211b6be68bb73 Reviewed-on: https://go-review.googlesource.com/c/tools/+/176357 Reviewed-by: Alan Donovan <adonovan@google.com> Reviewed-by: Michael Matloob <matloob@golang.org>
Updates golang/go#31916 Change-Id: I38c08955bdb4ff2b0963d5c91c6e8f78267b8004 Reviewed-on: https://go-review.googlesource.com/c/build/+/176599 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Vet needs export data for the imports of the package it is analyzing. Vet does not need export data for the package itself, since vet will do its own type checking. Assuming that vet is just as good as the compiler at detecting invalid programs, don't run the compiler unnecessarily. This especially matters for tests without external test files or for which the external test files do not import the test-augmented original package. In that case, the test-augmented original package need not be compiled at all. Cuts time for 'go clean -cache && go vet -x cmd/compile/internal/ssa' from 7.6r 24.3u 2.8s to 3.5r 8.5u 1.9s, by not running the compiler on the augmented test package. There is still more to be done here - if we do need to build a test-augmented package, we rerun cgo unnecessarily. But this is a big help. Cuts time for 'go vet std cmd' by about 30%. For #31916. Change-Id: If6136b4d384f1da77aed90b43f1a6b95f09b5d86 Reviewed-on: https://go-review.googlesource.com/c/go/+/176438 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Now that the main tree complies with 'go vet', enable all vet checks during 'go test' in the main tree. This helps surface helpful errors while developing, instead of having to wait for the misc-vet-vetall builder. During 'go test', the additional vet checks are essentially free: the vet invocations themselves take only 8 seconds total for the entire tree. Also update buildall.bash (used by the misc-compile builders) to run 'go vet std cmd' for each GOOS/GOARCH pair. This is not as free, since in general it can require recompiling packages with their tests included before invoking vet. (That compilation was going on anyway in the 'go test' case.) On my Mac laptop, ./buildall.bash freebsd used to take 68+16+17+18 = 119 seconds for make.bash and then the builds of the three freebsd architectures. Now it takes 68+16+23+17+23+18+24 = 189 seconds, 60% longer. Some of this is spent doing unnecessary cgo work. Still, this lets us shard the vet checks and match all.bash. Fixes #20119. For #31916. Change-Id: I6b0c40bac47708a688463c7fca12c0fc23ab2751 Reviewed-on: https://go-review.googlesource.com/c/go/+/176439 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
What the vetall builder does - run go vet on std and tell us about problems - is valuable, but hiding that in a builder makes it too easy to be surprised when it breaks. It would be better if we ran 'go vet' during run.bash (maybe during 'go test', maybe separately), so that the problems are surfaced before you get all the way to sending CLs out. Doing this would require first cleaning up std to be vet-safe.
I intend to chip away at this.
Update: Based on discussion below, my concrete suggestion is:
This will help people catch problems on their native GOOS/GOARCH during regular development.
This will help keep detecting build breakages, which is a side effect of the current vetall builder.
The text was updated successfully, but these errors were encountered: