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

Jump to bottom

cmd/vet: GO111MODULE=off go test -count=1 cmd/vet fails #32107

Closed
cuonglm opened this issue May 17, 2019 · 21 comments
Closed

cmd/vet: GO111MODULE=off go test -count=1 cmd/vet fails #32107

cuonglm opened this issue May 17, 2019 · 21 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@cuonglm
Copy link
Member

cuonglm commented May 17, 2019 •
edited

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

$ go version
go version devel +9be2d46422 Fri May 17 06:01:17 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cuonglm/.cache/go-build"
GOENV="/home/cuonglm/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cuonglm/go"
GOPROXY="direct"
GOROOT="/home/cuonglm/sources/go"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/home/cuonglm/sources/go/pkg/tool/linux_amd64"
GCCGO="/usr/local/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build714996541=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ GO111MODULE=off go vet cmd/vet/testdata/unsafeptr
$ GO111MODULE=on go vet cmd/vet/testdata/unsafeptr
# cmd/vet/testdata/unsafeptr
cmd/vet/testdata/unsafeptr/unsafeptr.go:12:6: possible misuse of unsafe.Pointer

What did you expect to see?

go vet produces output regardless of GO111MODULE value.

What did you see instead?

go vet does not produce output when GO111MODULE=off.
@cuonglm

This comment has been minimized.

Sign in to view
@cuonglm

This comment has been minimized.

Sign in to view
@bcmills

This comment has been minimized.

Sign in to view
@cuonglm

This comment has been minimized.

Sign in to view
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure. labels May 17, 2019
@bcmills bcmills added this to the Go1.13 milestone May 17, 2019
@bcmills
Copy link
Contributor

bcmills commented May 17, 2019

@rsc has been doing some refactoring in cmd/vet. Perhaps he knows?

@bradfitz

This comment has been minimized.

Sign in to view
@bcmills

This comment has been minimized.

Sign in to view
@cuonglm
Copy link
Member Author

cuonglm commented May 17, 2019

@bradfitz I think maybe because I set GO111MODULE=off

Hope anyone haven't run go mod tidy can try this.

@bcmills

This comment has been minimized.

Sign in to view
@cuonglm cuonglm changed the title cmd/vet: failed with current master 9be2d46422c90068db8bb9d29cb025907c6100b0 cmd/vet: test failed May 17, 2019
@cuonglm
Copy link
Member Author

cuonglm commented May 17, 2019

@bcmills @bradfitz Anyway, since when go mod tidy changes the go.sum, should I send a CL to update it?

@cuonglm
Copy link
Member Author

cuonglm commented May 20, 2019

@bcmills @bradfitz The error occurs again in my local

ok  	cmd/trace	0.064s
--- FAIL: TestVet (0.90s)
    --- FAIL: TestVet/print (0.27s)
        vet_test.go:162: error check failed: 
            print.go:169: missing error "Warn call has possible formatting directive %s"
            print.go:170: missing error "Warnf call needs 1 arg but has 2 args"
            print.go:171: missing error "Warnf format %r has unknown verb r"
            print.go:172: missing error "Warnf format %#s has unrecognized flag #"
            print.go:173: missing error "Warn2 call has possible formatting directive %s"
            print.go:174: missing error "Warnf2 call needs 1 arg but has 2 args"
            print.go:175: missing error "Warnf2 format %r has unknown verb r"
            print.go:176: missing error "Warnf2 format %#s has unrecognized flag #"
    --- FAIL: TestVet/unsafeptr (0.27s)
        vet_test.go:153: vet output:
        vet_test.go:154: <nil>
FAIL
FAIL	cmd/vet	1.939s
FAIL
2019/05/20 14:30:35 Failed: exit status 1

@cuonglm cuonglm changed the title cmd/vet: test failed cmd/vet: go vet cmd/vet/testdata/unsafeptr does not produce output when GO111MODULE=off May 20, 2019
@cuonglm
Copy link
Member Author

cuonglm commented May 20, 2019

@bcmills @bradfitz I think I found the culprit, updated the issue.

@cuonglm cuonglm changed the title cmd/vet: go vet cmd/vet/testdata/unsafeptr does not produce output when GO111MODULE=off cmd/vet: go vet cmd/vet/testdata/unsafeptr does not run when GO111MODULE=off May 20, 2019
@cuonglm cuonglm changed the title cmd/vet: go vet cmd/vet/testdata/unsafeptr does not run when GO111MODULE=off cmd/vet: go vet cmd/vet/testdata/unsafeptr does not report error when GO111MODULE=off May 20, 2019
@bradfitz
Copy link
Contributor

@bcmills @bradfitz Anyway, since when go mod tidy changes the go.sum, should I send a CL to update it?

No, no need.

@cuonglm
Copy link
Member Author

cuonglm commented May 20, 2019

I believe the problem comes from this line https://github.com/golang/tools/blob/9558fe4a7893c491f71cd402ea24d4a9366b8918/go/analysis/internal/analysisflags/flags.go#L56

When GO111MODULE=on, cmd/vet/testdata/unsafeptr is treated as a package path, so the flag for unsafeptr is not set. When GO111MODULE=off, cmd/vet/testdata/unsafeptr is treated as a flag toggle, the flag for unsafeptr set to false, causing it never run.

Is it a flag.Parse() bug? cc @bradfitz @bcmills

@rsc
Copy link
Contributor

rsc commented May 28, 2019

It's unclear that go vet cmd/vet/testdata/unsafeptr was ever meant to be useful. The testdata directories are explicitly excluded from matching patterns like all and cmd and cmd/.... If this bug is only about the behavior on that specific argument, I'm not particularly worried.

If this is really about a real package, please give an example that is not testing a testdata directory.

@rsc
Copy link
Contributor

rsc commented May 28, 2019

OK, I see that the real problem is GO111MODULE=off go test -count=1 cmd/vet failing. I will adjust the title here.

@rsc rsc changed the title cmd/vet: go vet cmd/vet/testdata/unsafeptr does not report error when GO111MODULE=off cmd/vet: GO111MODULE=off go test -count=1 cmd/vet fails May 28, 2019
@rsc
Copy link
Contributor

rsc commented May 28, 2019

Separately, it may be that we should special-case GO111MODULE and include it in the test cache key hash. I am seeing GO111MODULE=on go test cmd/vet apparently fill a cache result used by GO111MODULE=off go test cmd/vet.

@cuonglm
Copy link
Member Author

cuonglm commented May 28, 2019

@rsc GO111MODULE=off go test -count=1 cmd/vet is actually what I reported in the first time. I changed to go vet cmd/vet/testdata/unsafeptr because when I did some debug, I saw that the flag in this line https://github.com/golang/tools/blob/9558fe4a7893c491f71cd402ea24d4a9366b8918/go/analysis/internal/analysisflags/flags.go#L56 was set to false for unsafeptr if GO111MODULE=off.

I did not look much into it since #32107 (comment), would take another look when I have time.

@cuonglm
Copy link
Member Author

cuonglm commented May 28, 2019

Separately, it may be that we should special-case GO111MODULE and include it in the test cache key hash. I am seeing GO111MODULE=on go test cmd/vet apparently fill a cache result used by GO111MODULE=off go test cmd/vet.

Filled #32285

@bcmills
Copy link
Contributor

bcmills commented May 29, 2019

Separately, it may be that we should special-case GO111MODULE and include it in the test cache key hash.

The test's dependency on GO111MODULE comes from the combination of being in the standard library (and hence not having any module dependencies), plus executing cmd/go as a subprocess (which adds the dependency on module-mode behavior).

Instead of hard-coding GO111MODULE in particular, I think we should make internal/testenv.GoToolPath touch all of the environment variables listed in cmd/go/internal/cfg.knownEnv. That makes the connection between the variables and the test results more explicit and much more robust: tests that execute the go command are potentially sensitive to all of the variables that the go command reads.

@bcmills bcmills mentioned this issue May 29, 2019
Closed
@andybons andybons removed this from the Go1.13 milestone Jul 8, 2019
@andybons andybons added this to the Go1.14 milestone Jul 8, 2019
@cuonglm cuonglm mentioned this issue Aug 6, 2019
Closed
@cuonglm cuonglm mentioned this issue Aug 28, 2019
Closed
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@cuonglm
Copy link
Member Author

cuonglm commented Feb 13, 2020

This was fixed in 902d5aa

@cuonglm cuonglm closed this as completed Feb 13, 2020
@golang golang locked and limited conversation to collaborators Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@bradfitz @rsc @andybons @cuonglm @bcmills @gopherbot