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: go directive is not only added during go mod init, but also under other conditions that are hard to deduce #30790

Closed
dmitshur opened this issue Mar 12, 2019 · 14 comments
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Mar 12, 2019

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

$ go version
go version go1.12 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/dmitshur/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/dmitshur/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3m/rg2zm24d1jg40wb48wr0hdjw00jwcj/T/go-build766457690=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run the following commands in one module that has no module requirements:

$ cat go.mod 
module m1

$ go mod tidy

$ cat go.mod 
module m1

go 1.12

Now run the same commands in another module that has 1 or more module requirements:

$ cat go.mod
module m2

require rsc.io/quote v1.0.0

$ go mod tidy

$ cat go.mod
module m2

require rsc.io/quote v1.0.0

Additionally, if the source code in module m2 is modified to no longer import anything from the rsc.io/quote module, and go mod tidy is run, the first time it removes the require rsc.io/quote v1.0.0 line, leaving just module m2. The second time it adds a go directive.

What did you expect to see?

For modules that already exist, and do not already contain a go directive in their go.mod file, the go directive should either be always added automatically (i.e., during any of go build, go test, go mod tidy, etc., operations), or it should never be added.

What did you see instead?

Sometimes it's added, sometimes it isn't added.

Based on investigating the code with @julieqiu and @heschik, we've found that the go directive is always added to go.mod on any go build, go test, go mod tidy, etc., operation whenever the go.mod file has exactly one statement: the module statement. If the go.mod file has at least 1 require, or replace, or exclude directive, in addition to the module statement, then various go operations do not add a go directive.

This behavior is surprising and hard to predict (without looking into the source code to figure it out). I suspect it's an unintentional behavior. The commit message of CL 147281 that implemented this behavior says:

cmd/go: add go statement when initializing go.mod

When creating a go.mod file, add a go statement mentioning the current Go version.

Based on that, I suspect the originally intended behavior was to make it so that a go directive is automatically inserted only when creating a new Go module (via go mod init), but not when working with an existing module that has a go.mod file. I will assign this to @ianlancetaylor to confirm if this is a bug, and perhaps make the decision on how and whether this issue should be resolved.

I think we should fix this and even consider backporting to Go 1.12.1. Doing so should would make the logic of when a go directive is added easier to predict and understand, improving the user experience when in module mode.

/cc @bcmills @matloob

@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go modules labels Mar 12, 2019
@dmitshur dmitshur added this to the Go1.13 milestone Mar 12, 2019
@thepudds
Copy link
Contributor

Likely related: #30043 (comment)

@ianlancetaylor
Copy link
Contributor

Before thinking about what should happen, why is the unpredictable go directive "quite disruptive to the user experience when in module mode"? Where does the disruption occur?

@ianlancetaylor
Copy link
Contributor

The intent of the CL was to add a "go" statement to a go.mod file, if one did not already exist, when running either go mod init or go mod tidy. Adding a "go" statement when running go mod tidy was suggested by @bcmills during CL review.

So it sounds like there is a bug: if the existing go.mod file contains anything other than just a "module" statement, then running go mod tidy does not add a "go" directive.

@dmitshur
Copy link
Contributor Author

dmitshur commented Mar 12, 2019

why is the unpredictable go directive "quite disruptive to the user experience when in module mode"?

The behavior I've observed is that various people are either uncertain about when the go directive is actually added (e.g., they describe it as "sometimes"), or they have an incorrect idea of when it's added (e.g., "always added by go mod tidy in Go 1.12" or "only added by go mod init").

If people choose to not add the go directive until figuring out what value it should be (related to #30791), it means they sometimes have to remove the go directive added (e.g., after running go test on a module with 0 required modules) but not other times (e.g., after running go test on a module with 1+ required modules). This encourages people to add the directive with an arbitrary value before understanding the short-term and long-term consequences of doing so.

Where does the disruption occur?

I've observed it occur during code review, and while people are running go commands inside various modules.

Edit: I've also seen it occur during testing of module-related code, as demonstrated by the commit message of CL 168757.

@dmitshur
Copy link
Contributor Author

dmitshur commented Mar 12, 2019

The intent of the CL was to add a "go" statement to a go.mod file, if one did not already exist, when running either go mod init or go mod tidy. Adding a "go" statement when running go mod tidy was suggested by @bcmills during CL review.

Currently, it also gets added when running go build or go test in a module with a go.mod file that has zero module requirements (and no exclude, replace statements):

$ cat go.mod
module m3
$ go test
PASS
ok  	m3	0.041s
$ cat go.mod
module m3

go 1.12

Is that a bug or intended behavior?

So it sounds like there is a bug: if the existing go.mod file contains anything other than just a "module" statement, then running go mod tidy does not add a "go" directive.

Making both go mod init and go mod tidy add the go directive, if it's not already present, regardless of whether the go.mod file contains just a module statement or more statements, sounds like a valid resolution to this issue.

I've considered suggesting that we change the behavior such that go directive is added only during go mod init and none of the other commands, so that existing modules made in Go 1.11 (without the go directive) don't suddenly start gaining a go directive after users upgrade to Go 1.12. However, if the go directive will ever become mandatory rather than optional, then some version of Go will have to start adding it to existing modules in addition to new modules. So it might as well be Go 1.12.

@dmitshur dmitshur changed the title cmd/go: go directive is not only added during go mod init, but also under specific and surprising conditions cmd/go: go directive is not only added during go mod init, but also under other conditions that are hard to deduce Mar 13, 2019
@ianlancetaylor
Copy link
Contributor

Currently, it also gets added when running go build or go test in a module with a go.mod file that has zero module requirements (and no exclude, replace statements):
Is that a bug or intended behavior?

I don't know. I don't really understand how go build and go test are supposed to modify the go.mod file. This likely shouldn't happen. Clearly the inconsistency shouldn't happen, and likely the "go" directive should not be added at all in this case. But I could easily be convinced otherwise.

@bcmills
Copy link
Contributor

bcmills commented Mar 13, 2019

The empty-requirements behavior is probably a side effect of this case:

if len(f.Syntax.Stmt) == 1 && f.Module != nil {
// Entire file is just a module statement.
// Populate require if possible.
legacyModInit()
}

The go directive is added here:

Perhaps that call should be moved inside the modFile == nil condition.

@bcmills
Copy link
Contributor

bcmills commented Mar 13, 2019

Or, going in the other direction, perhaps we should always add the go directive if it is missing. That ensures that a module that was successfully built and tested with, say, Go 1.13, continues to build (with the same meaning) with Go 1.14.

@thepudds
Copy link
Contributor

CC @jayconrod

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/168757 mentions this issue: go/packages/testdata: add go directives to fake module files

gopherbot pushed a commit to golang/tools that referenced this issue Mar 22, 2019
Stop fighting the behavior of the go tool when run in these directories.

Updates golang/go#30790

Change-Id: I32dfeb0bafa3ed3664500f1768b2293e5257d09b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/168757
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/169877 mentions this issue: cmd/go/internal/modload: do not implicitly add a 'go' directive to an existing go.mod file

@bcmills
Copy link
Contributor

bcmills commented Mar 28, 2019

@gopherbot, please backport to 1.12: the current behavior is confusing, and the fix is small

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #31117 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/176925 mentions this issue: cmd/go: always add 'go' directive to the go.mod file if missing

gopherbot pushed a commit that referenced this issue May 16, 2019
Updates #30790
Fixes #31960

Change-Id: Ib3ac074cf1f98fe69f53af82d2a0441582116570
Reviewed-on: https://go-review.googlesource.com/c/go/+/176925
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants