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: 'vet' and 'test' duplicate flags from GOFLAGS if they are also specified explicitly #32471

Closed
elagergren-spideroak opened this issue Jun 6, 2019 · 17 comments
Assignees
Labels
Milestone

Comments

@elagergren-spideroak
Copy link

@elagergren-spideroak elagergren-spideroak commented Jun 6, 2019

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

$ go version
1.12.5

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

go env Output
$ go env
darwin/amd64

What did you do?

Set GOFLAGS=-vet=off then ran go test -vet=all.

What did you expect to see?

go test ... to not error out.

What did you see instead?

$ GOFLAGS=-test.vet=off go test -test.vet all
# _/tmp/bar
./bar_test.go:6:2: Logf format %f has arg X of wrong type string
FAIL	_/tmp/bar [build failed]
$ GOFLAGS=-vet=off go test -vet all
go test: vet flag may be set only once
run "go help test" or "go help testflag" for more information
$ GOFLAGS=-test.vet=off go test -vet all
# _/tmp/bar
./bar_test.go:6:2: Logf format %f has arg X of wrong type string
FAIL	_/tmp/bar [build failed]
$ GOFLAGS=-vet=off go test -test.vet all
go test: vet flag may be set only once
run "go help test" or "go help testflag" for more information
@elagergren-spideroak
Copy link
Author

@elagergren-spideroak elagergren-spideroak commented Jun 6, 2019

Similarly, GOFLAGS="-vet=all -vet=off" go test -v does not work. Related to #29053

@dmitshur dmitshur added this to the Go1.14 milestone Jun 10, 2019
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 10, 2019

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 10, 2019

What happens for the other flags with the test. prefix, such as -test.v and -test.run?

We should probably at least make -vet consistent with those.

@elagergren-spideroak
Copy link
Author

@elagergren-spideroak elagergren-spideroak commented Jun 11, 2019

$ GOFLAGS=-test.v go test -test.v=false
# _/tmp/bar
./bar_test.go:6:2: Logf format %f has arg X of wrong type string
FAIL	_/tmp/bar [build failed]
$ GOFLAGS=-test.v go test -v=false
# _/tmp/bar
./bar_test.go:6:2: Logf format %f has arg X of wrong type string
FAIL	_/tmp/bar [build failed]
$ GOFLAGS=-v go test -v=false
go test: v flag may be set only once
run "go help test" or "go help testflag" for more information
$ GOFLAGS=-v go test -test.v=false
go test: v flag may be set only once
run "go help test" or "go help testflag" for more information
@elagergren-spideroak
Copy link
Author

@elagergren-spideroak elagergren-spideroak commented Jun 11, 2019

-test.run exhibits the same behavior as -test.v and -test.vet.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@agnivade
Copy link
Contributor

@agnivade agnivade commented Nov 11, 2019

The errors seem to be consistent for all flags.

@elagergren-spideroak - It's not clear from the issue description, but for GOFLAGS=-vet=off go test -vet all, did you want the vet=all value to override the vet=off value set in GOFLAGS ?

@amenzhinsky
Copy link
Contributor

@amenzhinsky amenzhinsky commented Nov 11, 2019

I'm having the same problem with persistent environment variables, even if I set the corresponding env var to an empty string:

go env -w GOFLAGS="-trimpath"
GOFLAGS= go test -trimpath # or simply "go test -trimpath"

go test: trimpath flag may be set only once
run "go help test" or "go help testflag" for more information
@amenzhinsky
Copy link
Contributor

@amenzhinsky amenzhinsky commented Nov 11, 2019

It seems only go test and go vet have the issue, everything is alright with the others.

@elagergren-spideroak
Copy link
Author

@elagergren-spideroak elagergren-spideroak commented Nov 11, 2019

@agnivade Yeah, sorry. From the docs:

GOFLAGS
	...
        Flags listed on the command line
	are applied after this list and therefore override it.
@agnivade
Copy link
Contributor

@agnivade agnivade commented Nov 12, 2019

@amenzhinsky - Rightly noted. This was introduced in CL 126656.

Only go/internal/test/testflag.go has f, value, extraWord := cmdflag.Parse(cmd, usage, testFlagDefn, args, i)

and go/internal/vet/vetflag.go has f, value, extraWord := cmdflag.Parse("vet", usage, vetFlagDefn, args, i) due to which this happens.

The args come from modification with cmdflag.FindGOFLAGS(vetFlagDefn)

The other go subcommands have a different code path for flag processing.

Perhaps @rsc @bcmills will know why is this.

@amenzhinsky
Copy link
Contributor

@amenzhinsky amenzhinsky commented Nov 12, 2019

I think the easiest fix would be removing this:

if f.Present {
SyntaxError(cmd, f.Name+" flag may be set only once")
}

Moreover all subcommands and tools accept flags overriding (correct me if I'm wrong, checked on that vert quickly).

@bcmills bcmills changed the title cmd/go: vet flag may be set only once cmd/go: 'vet' and 'test' duplicate flags from GOFLAGS if they are also specified explicitly Nov 26, 2019
@bcmills bcmills self-assigned this Nov 26, 2019
@bcmills bcmills modified the milestones: Backlog, Go1.14 Nov 26, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 5, 2019

@bcmills Is this going to happen for 1.14?

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 5, 2019

Yes.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210937 mentions this issue: cmd/go: restore default vet analyzers for targets in GOROOT

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210940 mentions this issue: cmd/go: allow arguments to 'go test' and 'go vet' to duplicate or override flags from GOFLAGS

gopherbot pushed a commit that referenced this issue Dec 11, 2019
…rride flags from GOFLAGS

This is a minimal fix for Go 1.14, but this parsing logic is much too
complex and seems like it will cause more trouble going forward.

I intend to mail a followup change to refactor this logic for 1.15.

Updates #32471

Change-Id: I00ed07dcf3a23c9cd4ffa8cf764921fb5c18bcd6
Reviewed-on: https://go-review.googlesource.com/c/go/+/210940
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@bcmills
Copy link
Member

@bcmills bcmills commented Dec 11, 2019

A minimal fix for 1.14 is now merged. I plan to follow that up with a cleaner refactoring for 1.15.

@bcmills bcmills removed this from the Go1.14 milestone Dec 11, 2019
@bcmills bcmills added this to the Go1.15 milestone Dec 11, 2019
@bcmills bcmills added the NeedsFix label Dec 11, 2019
gopherbot pushed a commit that referenced this issue Dec 12, 2019
This fixes a regression introduced in CL 209498,
found while investigating #32471.

Also fix $WORK replacement in cmd/go/internal/work.(*Builder).Showcmd
when b.WorkDir includes a backslash and appears in a quoted string.
That fix is needed in order to write a precise test that passes under Windows,
since Windows directories nearly always include backslashes.

Updates #35837

Change-Id: I5fddc5435d5d283a3e598989209d873b59b0a39c
Reviewed-on: https://go-review.googlesource.com/c/go/+/210937
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
JAORMX added a commit to JAORMX/operator-sdk that referenced this issue Dec 12, 2019
-mod=vendor is not needed if it's already set in GOFLAGS. So lets not
set it unless it's needed.

This is an issue for commands such as `go test` and `go vet` (
see: golang/go#32471 ). This avois such an
issue.
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 13, 2019

Change https://golang.org/cl/211358 mentions this issue: cmd/go/internal/{test,vet}: use a standard flag.FlagSet to parse flags

estroz added a commit to operator-framework/operator-sdk that referenced this issue Jan 10, 2020
* Only add explicit -mod=vendor if needed

-mod=vendor is not needed if it's already set in GOFLAGS. So lets not
set it unless it's needed.

This is an issue for commands such as `go test` and `go vet` (
see: golang/go#32471 ). This avois such an
issue.

* Add comments for -mod=vendor flag addition

This takes into us the suggestions from @estroz and adds the relevant comments for the addition of the -mod=vendor GOFLAG

Co-authored-by: Eric Stroczynski <estroczy@redhat.com>
@gopherbot gopherbot closed this in 583419e Feb 25, 2020
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
8 participants
You can’t perform that action at this time.