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: mod download and go mod tidy conflict (1.16beta1 -> 1.16rc1 regression) #43994

Closed
howardjohn opened this issue Jan 29, 2021 · 10 comments
Closed
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@howardjohn
Copy link
Contributor

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

$ go version
go1.16rc1

Does this issue reproduce with the latest release?

Yes, it is only present in go1.16rc1, and not go1.16beta1

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/howardjohn/.cache/go-build"
GOENV="/home/howardjohn/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/howardjohn/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/howardjohn/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/howardjohn/sdk/go1.16rc1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/howardjohn/sdk/go1.16rc1/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16rc1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/howardjohn/go/src/istio.io/api/go.mod"
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-build1862906774=/tmp/go-build -gno-record-gcc-switches"

What did you do?

On 1.16beta1:

$ git status
On branch master
Your branch is up to date with 'origin/master'.

$ ~/go/bin/go1.16beta1 mod download
$ git status
On branch master
Your branch is up to date with 'origin/master'.

On 1.16rc1:

$ git status
On branch master
Your branch is up to date with 'origin/master'.
$ ~/go/bin/go1.16rc1 mod download
$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   go.sum

$ ~/go/bin/go1.16rc1 mod tidy
$ git status
On branch master
Your branch is up to date with 'origin/master'.

Repo: https://github.com/istio/api

What did you expect to see?

Either go mod download does not add to go.sum, OR go mod tidy keeps what is added by go mod download. In either case, the two tools should be consistent

What did you see instead?

go mod download adds to go.sum, which go mod tidy removes

howardjohn added a commit to howardjohn/api that referenced this issue Jan 29, 2021
@seankhliao seankhliao changed the title go mod download and go mod tidy conflict (1.16beta1 -> 1.16rc1 regression) cmd/go: mod download and go mod tidy conflict (1.16beta1 -> 1.16rc1 regression) Jan 29, 2021
@seankhliao seankhliao added GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 29, 2021
istio-testing pushed a commit to istio/api that referenced this issue Jan 29, 2021
@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2021

go mod tidy downloads source code only for those modules that are needed to provide packages transitively imported by the packages and tests in the main module (go list -deps -test ./...). go mod tidy reduces the go.sum file to contain checksums for only those modules.

In contrast, go mod download downloads source code for all modules in the module graph (go list -m all; compare #41431). As a side-effect, it should also record the checksums for those modules.

If go mod download fails spuriously due to missing checksums (or due to an error writing the go.sum file), that's probably a bug we should fix, but if it is just recording more checksums than go mod tidy does, that's an unfortunate but intentional difference between the current behavior of the two commands.

CC @jayconrod @matloob

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2021

(Note that lazy loading — #36460 — should bring go mod tidy and go mod download closer together, but even then go mod download will still potentially download more than go mod tidy.)

@howardjohn
Copy link
Contributor Author

For more details on the impact: all of our CI enforces go mod tidy is run. Some of our CI was actually running go mod tidy then go mod download, resulting in it generating a git diff which is rejected. The fix there is easy - just run go mod tidy last.

However, in day to day use its pretty annoying that if I run go mod download my go.sum file is suddenly modified, and if I accidentally check it into git my PR will be rejected. I think this is a common practice in projects as well, and not unique to us to reject spurious go.sum changes.

but even then go mod download will still potentially download more than go mod tidy.

I am not sure there is a distinction in practice, but my concern is only about modifying/reverting go.sum - I don't mind if its downloading more or not.

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2021

However, in day to day use its pretty annoying that if I run go mod download my go.sum file is suddenly modified, and if I accidentally check it into git my PR will be rejected.

Could you give some more detail about when you run go mod download in day-to-day use? I'm a little surprised that it's needed, and that may indicate that either we don't understand how people are using the tool or we need better documentation on the expected day-to-day workflow.

(As part of #36460 I've been mulling over whether we should add a go mod download flag to download only what's relevant to the main module, but today that's a little redundant with go list -test all.)

I think this is a common practice in projects as well, and not unique to us to reject spurious go.sum changes.

Agreed: we expect that a lot of projects will enforce that go mod tidy is a no-op for PRs. (See also #27005.)

@howardjohn
Copy link
Contributor Author

Using go mod download in docker seems common for better layer caching.

There are cases where I want my IDE completion to work, but need the modules first. I suppose I could just run go mod tidy instead, but go mod download worked before so I've just used that. Perhaps its not a great reason, but I am a bit confused what go mod download is intended for if not this.

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2021

For IDE completion, gopls should already be downloading the dependencies of the main module as needed. (If not, I'd suggest filing a separate issue for that.)

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2021

I am a bit confused what go mod download is intended for if not this.

The short answer, I think, is that go mod download is not really the right tool for anything. 😅
(It is optimized for “give me a short command that does at least what I probably need and is easy to remember”.)

go list all is probably the thing you want to run to prime a Docker cache, and perhaps to prime a non-gopls IDE too.

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2021

FWIW, the behavior change here was almost certainly CL 282692, for #41103.

@jayconrod
Copy link
Contributor

+1 to Bryan's comments. The behavior has changed, but I think the new behavior is correct.

go mod download without arguments fetches every module in the build list (the modules printed by go list -m all). That tends to be much more than what you need to build and test the packages in the main module because it includes things like test dependencies of test dependencies. If you're also running go mod tidy, that already fetches the modules you need. go list -test all is another good option.

In earlier versions, go mod download was inconsistent with how it updated go.sum. It would add sums needed to load the module graph (that is, it would add /go.mod hashes), but it would not add sums for module content. In 1.16, we've fixed it so that go mod download saves sums for contents of modules in the build list. That's important for resolving some errors with -mod=readonly (which is the default in 1.16): there are some cases where you need to run go mod download somemod@someversion to add a specific sum.

I think that's the new behavior you're seeing here. I checked out github.com/istio/api, ran go mod download, then ran go mod why -m on each new sum in go.sum. None of those modules provide packages imported by the main module, so go mod tidy removes the sums.

lmb added a commit to cilium/ebpf that referenced this issue Mar 1, 2021
According to golang/go#43994 go mod tidy should
be used to prime the module cache.

Updates #226
lmb added a commit to cilium/ebpf that referenced this issue Mar 1, 2021
According to golang/go#43994 go mod tidy should
be used to prime the module cache.

Updates #226
lmb added a commit to cilium/ebpf that referenced this issue Mar 1, 2021
According to golang/go#43994 go mod tidy should
be used to prime the module cache.

Updates #226
lmb added a commit to cilium/ebpf that referenced this issue Mar 1, 2021
According to golang/go#43994 go mod tidy should
be used to prime the module cache.

Updates #226
ldez added a commit to go-acme/lego that referenced this issue Mar 3, 2021
With go1.16 thete is a difference between `go mod tidy` and `go mod
download`.

The `go.sum` is not altered in the same way by both commands.

`go mod tidy` must be always called after the `go mod download`.

golang/go#43994
ldez added a commit to go-acme/lego that referenced this issue Mar 3, 2021
With go1.16 there is a difference between `go mod tidy` and `go mod
download`.
The `go.sum` is not altered in the same way by both commands.
`go mod tidy` must be always called after the `go mod download`.

golang/go#43994
@jeffwidman
Copy link
Contributor

Note that this behavior was reverted in #45332 which was backported to 1.16.5... now go mod download doesn't fight go mod tidy for go.sum changes...

lmb added a commit to cloudflare/tubular that referenced this issue Feb 17, 2022
Use go mod tidy instead of go mod download in run-tests. The latter
changes go.sum for some reason.

See golang/go#43994
@golang golang locked and limited conversation to collaborators Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants