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: tip breaks program flags in "go run" #64738

Closed
mvdan opened this issue Dec 15, 2023 · 6 comments
Closed

cmd/go: tip breaks program flags in "go run" #64738

mvdan opened this issue Dec 15, 2023 · 6 comments
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Dec 15, 2023

Go version

go version devel go1.22-d73b4322ed Thu Dec 14 22:24:40 2023 +0000 linux/amd64

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

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/mvdan/.cache/go-build'
GOENV='/home/mvdan/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/mvdan/go/pkg/mod'
GONOPROXY='github.com/cue-unity'
GONOSUMDB='github.com/cue-unity'
GOOS='linux'
GOPATH='/home/mvdan/go'
GOPRIVATE='github.com/cue-unity'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/mvdan/tip'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/mvdan/tip/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.22-d73b4322ed Thu Dec 14 22:24:40 2023 +0000'
GCCGO='gccgo'
GOAMD64='v3'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2866565515=/tmp/go-build -gno-record-gcc-switches'

What did you do?

$ go version
go version devel go1.22-d73b4322ed Thu Dec 14 22:24:40 2023 +0000 linux/amd64
$ go run cuelang.org/go/cmd/cue@v0.5.0-beta.5 import -p json
go: -p must be a positive integer: 0
$ /usr/bin/go version
go version go1.21.5 linux/amd64
$ /usr/bin/go run cuelang.org/go/cmd/cue@v0.5.0-beta.5 import -p json
$

What did you expect to see?

Same behavior between Go tip and 1.21; the flag should be passed to the cue program without any error from cmd/go.

What did you see instead?

It seems like cmd/go tries to parse the -p json flag, perhaps not realizing that go run takes an arbitrary number of flags and arguments to pass on to the main package being run:

$ go run -h
usage: go run [build flags] [-exec xprog] package [arguments...]

In my case, import -p json go after the single package argument, so cmd/go should not attempt to parse them.

cc @bcmills I suspect this might be a very recent regression due to https://go-review.googlesource.com/c/go/+/546635. I haven't bisected, but I've also never run into this error before.

@mauri870 mauri870 added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Dec 15, 2023
@mauri870
Copy link
Member

Confirmed that this is a regression from CL 546635. I see that we are also backporting it to go 1.21 in CL550055.

@prattmic prattmic added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Dec 15, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 15, 2023
@bcmills bcmills self-assigned this Dec 15, 2023
@bcmills bcmills added this to the Go1.22 milestone Dec 15, 2023
@mauri870
Copy link
Member

@bcmills This appears to be enough to fix the issue, but I'm not sure if it is the correct fix:

diff --git a/src/cmd/go/internal/toolchain/select.go b/src/cmd/go/internal/toolchain/select.go
index 84fa7f685c..835d8117cc 100644
--- a/src/cmd/go/internal/toolchain/select.go
+++ b/src/cmd/go/internal/toolchain/select.go
@@ -15,6 +15,7 @@ import (
        "os"
        "path/filepath"
        "runtime"
+       "slices"
        "strconv"
        "strings"

@@ -556,7 +557,15 @@ func goInstallVersion() bool {

        // Make a best effort to parse flags so that module flags like -modcacherw
        // will take effect (see https://go.dev/issue/64282).
-       args := os.Args[2:]
+
+       // All flags past pkg@version should apply to pkg and not to the go
+       // command itself.
+       pkgIdx := slices.Index(os.Args, arg)
+       if pkgIdx == -1 {
+               return false
+       }
+
+       args := os.Args[2:pkgIdx]
        for len(args) > 0 {
                var err error
                _, args, err = cmdflag.ParseOne(cmdFlags, args)

@bcmills
Copy link
Contributor

bcmills commented Dec 15, 2023

Thanks for the report! This is a surprising gap in our test coverage. 😅
I'll send a fix today.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/550237 mentions this issue: cmd/go/internal/toolchain: simplify flag parsing and stop at the first non-flag

@bcmills bcmills added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Dec 15, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/551215 mentions this issue: cmd/go/internal/toolchain: revert "make a best effort to parse 'go run' and 'go install' flags"

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/551255 mentions this issue: cmd/go: add test case for issue 64738

gopherbot pushed a commit that referenced this issue Jan 23, 2024
The straight revert in CL 551215 fixed this issue.
Add a test case to make sure we don't reintroduce it.

Test case copied from CL 550237 (by bcmills).

Fixes #64738.

Change-Id: I9654a1fd46fe1a1cc63ee6645a552ec21d720ad0
Reviewed-on: https://go-review.googlesource.com/c/go/+/551255
Reviewed-by: Michael Matloob <matloob@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan Mills <bcmills@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…n' and 'go install' flags"

This caused other problems, and for the purposes of the Go 1.22
release, we can just roll back to the Go 1.21 behavior and then
decide in January what the correct path forward is.

Revert of CL 546635.

Unfixes golang#64282.
Fixes golang#64738.
For golang#57001.

This reverts commit e44b8b1.

Change-Id: I78753c76dcd0bc6dbc90caa17f73248c42e5f64a
Reviewed-on: https://go-review.googlesource.com/c/go/+/551215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Than McIntosh <thanm@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
The straight revert in CL 551215 fixed this issue.
Add a test case to make sure we don't reintroduce it.

Test case copied from CL 550237 (by bcmills).

Fixes golang#64738.

Change-Id: I9654a1fd46fe1a1cc63ee6645a552ec21d720ad0
Reviewed-on: https://go-review.googlesource.com/c/go/+/551255
Reviewed-by: Michael Matloob <matloob@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
Status: Done
Development

No branches or pull requests

6 participants