Skip to content

cmd/vet/all: get golang.org/x/tools/go/analysis/cmd/vet via a go.mod file, not via coordinator #31040

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.

Sign up for GitHub

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

Jump to bottom
Closed
bradfitz opened this issue Mar 25, 2019 · 16 comments
Closed

cmd/vet/all: get golang.org/x/tools/go/analysis/cmd/vet via a go.mod file, not via coordinator #31040

bradfitz opened this issue Mar 25, 2019 · 16 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@bradfitz
Copy link
Contributor

The cmd/vet/all program right now builds golang.org/x/tools/go/analysis/cmd/vet using the x/tools repo head tree places there by the x/build/cmd/coordinator.

Several problems with that, including:

  • it doesn't work on release branches
  • it's extra complication in the coordinator

It'd be nicer if cmd/vet/all just use modules mechanism & its versioning to get at x/tools/go/analysis/cmd/vet instead.

And then the coordinator could be simplified (https://golang.org/cl/169198).

/cc @dmitshur @bcmills @josharian
@bradfitz bradfitz added help wanted Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done. labels Mar 25, 2019
@bradfitz bradfitz added this to the Go1.13 milestone Mar 25, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/169198 mentions this issue: cmd/coordinator: remove special cases for misc-vet-vetall

@dmitshur
Copy link
Member

cmd/vet/all is inside the cmd module, and its go.mod file specifies a version of the golang.org/x/tools module. Is it not being used?

go/src/cmd/go.mod

Line 11 in e6df179

golang.org/x/tools v0.0.0-20190320160634-b6b7807791df

@bradfitz
Copy link
Contributor Author

Maybe! We could remove all the code in the coordinator for it and disable it on Go 1.12 and earlier release branches and see what happens.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/169237 mentions this issue: cmd/vet/all: don't use the x/tools vet in GOPATH

@dmitshur
Copy link
Member

dmitshur commented Mar 25, 2019 •
edited
Loading

And update the go.mod to latest code with the bug fixed.

FWIW, the x/tools version currently specified in the cmd module go.mod file predates both CL 168404 (which caused #30971) and CL 168803 (which fixed it), so it should pass.

When updating to a newer version, we should use golang/tools@6aabc1c or newer.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/169239 mentions this issue: cmd: update go.mod versions for vetall bug

gopherbot pushed a commit that referenced this issue Mar 26, 2019
@bradfitz
cmd: update go.mod versions for vetall bug
0ec302c 
Updates #30971
Updates #31040

Change-Id: I305fbddb6f79cbe3d7e29225841309ab00b1e7dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/169239
Run-TryBot: Brad Fitzpatrick <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
gopherbot pushed a commit to golang/build that referenced this issue Mar 26, 2019
@bradfitz
cmd/coordinator: remove special cases for misc-vet-vetall
6293144 
See plan at golang/go#30971 (comment)

Updates golang/go#31040

Change-Id: I160d671670094c3ab4f9d7ba825235a865ad3376
Reviewed-on: https://go-review.googlesource.com/c/build/+/169198
Reviewed-by: Dmitri Shuralyov <[email protected]>
gopherbot pushed a commit that referenced this issue Mar 26, 2019
@bradfitz
cmd/vet/all: don't use the x/tools vet in GOPATH
db7e746 
Updates #31040

Change-Id: I76e3044b2cc992e63194654a825e70307075eff3
Reviewed-on: https://go-review.googlesource.com/c/go/+/169237
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/169339 mentions this issue: dashboard: only run misc-vet-vetall for master

gopherbot pushed a commit to golang/build that referenced this issue Mar 26, 2019
@bradfitz
dashboard: only run misc-vet-vetall for master
c75979e 
As of CL 169198 for golang/go#31040, the coordinator no longer supports
placing the x/tools files out for Go 1.11 and Go 1.12's cmd/vet/all to
find, so their misc-vet-vetall builds will start failing.

We could in theory still either backport CL 169237 to
release-branch.go1.11 and release-branch.go1.12, and/or we could keep
running the misc-vet-vetall builder for the future Go 1.13 release
branch, but it's not obviously even worth it; the vetall builder is
most useful during development anyway. The tree is pretty stable once
it's release branch time. The main use of misc-vet-vetall is catching
assembly vet failures (since go test now does most the other vet
checks), and assembly doesn't often change in release branches.

Updates golang/go#31040

Change-Id: I7b827ecbcd206f3dcf63e04cc94fb78854befd7d
Reviewed-on: https://go-review.googlesource.com/c/build/+/169339
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@bradfitz bradfitz modified the milestones: Go1.13, Unreleased Mar 26, 2019
@bradfitz
Copy link
Contributor Author

All submitted for master.

Over to @dmitshur if he wants to backport the cmd/vet/all changes to Go 1.11 and Go 1.12. (They're builder-only things and don't affect end users, where the tests are disabled by default anyway)

@bcmills
Copy link
Contributor

bcmills commented Mar 27, 2019 •
edited
Loading

Over to @dmitshur if he wants to backport the cmd/vet/all changes to Go 1.11 and Go 1.12.

The cmd/vet/all changes seem to require the presence of cmd/go.mod, which may be difficult to backport.

(We can always just try it and see, but many tests in cmd had to be updated for #30228: search for cmd/ in the gopherbot mention-links.)

On the other hand, it might be straightforward to add a cmd/vet/all/go.mod in those releases.

@bradfitz
Copy link
Contributor Author

On the other hand, it might be straightforward to add a cmd/vet/all/go.mod in those releases.

That was my thought. It should be a very basic go.mod file with a single line.

@laboger
Copy link
Contributor

laboger commented Mar 29, 2019

I'm not sure if this is the right place to report this, but I'm seeing trybot failures from cmd/vet/all in my CL https://go-review.googlesource.com/c/go/+/168478. At first I thought it was unrelated to my change (which is only for crypto/elliptic asm on ppc64le so I don't understand how that could affect vet running other platforms) but after I rebased and reran, it gave me some errors in my asm which I fixed. After fixing those errors, I uploaded the fix, reran the trybots, and I am seeing some other odd errors in vet/freebsd/arm.

How can a run the equivalent of misc-vet-all for testing before I submit a change? I don't see it run from ./all.bash.

@bcmills
Copy link
Contributor

bcmills commented Mar 29, 2019 •
edited
Loading

How can a run the equivalent of misc-vet-all for testing before I submit a change?

cd $(go env GOROOT)/src/cmd/vet/all && go run main.go -all

@ianlancetaylor
Copy link
Contributor

I think you just need to rebase forward. I hope.

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 29, 2019 via email

@laboger
Copy link
Contributor

laboger commented Mar 29, 2019 •
edited
Loading

OK, thanks. I thought I had rebased once yesterday but looks like I just uploaded a new patch.

It was the different failures that threw me.

@bradfitz
Copy link
Contributor Author

bradfitz commented Apr 3, 2019

This happened in the CLs above and is working well.

@bradfitz bradfitz closed this as completed Apr 3, 2019
@golang golang locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @dmitshur @ianlancetaylor @bcmills @gopherbot @laboger