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/go: most 'go mod' subcommands should not update go.mod, go.sum #45551

Closed
jayconrod opened this issue Apr 13, 2021 · 10 comments
Closed

cmd/go: most 'go mod' subcommands should not update go.mod, go.sum #45551

jayconrod opened this issue Apr 13, 2021 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.18

Comments

@jayconrod
Copy link
Contributor

As of Go 1.16, all go mod subcommands can update go.mod and go.sum. Some commands are intended to update these files, and that shouldn't change. However, most commands may still perform an automatic update, which may be surprising. Internally, go mod subcommands run in -mod=mod mode, even though they don't have a -mod command line flag.

For some commands, updating go.mod and go.sum is expected:

  • go mod edit is supposed to edit go.mod.
  • go mod init creates go.mod and may import information from another dependency management tool.
  • go mod tidy is supposed to edit go.mod and go.sum.

For other commands, updating go.mod or go.sum may be quite unexpected. The go command should report an error if go.mod or go.sum need to be updated.

  • go mod graph is a query tool. It shouldn't have side effects.
  • go mod vendor is expected to update vendor but not go.mod.
  • go mod verify also shouldn't have side effects.
  • go mod why, same deal.

go mod download occupies an awkward middle ground. It has always been able to update go.mod (probably should not), but it may also add sums of modules in the build list to go.sum. This is usually unexpected and undesired when go mod download is invoked without arguments: users want something like go list all that populates the module cache with modules needed to build packages in the main module, not including unused dependencies of dependencies. However, we shouldn't entirely prevent go mod download from updating go.sum, since it's the best way to add a specific sum for a necessary but unused module (for disambiguating an imported package).

I'd suggest the following change in behavior for go mod download:

  • go mod download always loads the build list and reports an error if go.mod needs to be updated instead of updating it automatically.
  • go mod download only adds a hash to go.sum if the corresponding module is explicitly named on the command line (not simply matched by all or a wildcard or implied by a lack of arguments) without an explicit version or at the version in the build list.

cc @bcmills @matloob
@jayconrod jayconrod added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 13, 2021
@jayconrod jayconrod added this to the Backlog milestone Apr 13, 2021
@zikaeroh
Copy link
Contributor

Out of curiosity, would this proposal change the behavior of go mod download when go.mod says one thing, but the code says another? I'm not personally clear what "build list" is defined as, but IIRC it may be determined by the code present in the module.

My main use case for go mod download is docker image caching, where I often do something like:

COPY ./go.mod ./go.sum ./
RUN go mod download

COPY ./ ./

RUN go build

Such that I can download the dependencies given in go.mod into a layer, then bring the code in; that way when I modify the code my module cache is preserved and I don't need to rebuild the image.

In this case, I cannot use go list all because that requires the code to be present, meaning that when the code is changed, docker invalidates the layer (as the files changed), and the module cache is gone.

@jayconrod
Copy link
Contributor Author

@zikaeroh I think this proposal would improve things for you, at least a bit.

To clarify, go mod download doesn't read code, either before or after this proposal. When invoked without arguments or with the argument all, it downloads all the modules in the build list (the set of modules and versions selected by MVS). This is the same set of modules shown by go list -m all.

go mod download without arguments generally downloads modules you don't need (#41431). This proposal won't change that, but lazy loading (#36460) should help.

The difference here is that go mod download without arguments would not write go.sum, so you won't get a bunch unwanted changes there.

@zikaeroh
Copy link
Contributor

To clarify, go mod download doesn't read code, either before or after this proposal. When invoked without arguments or with the argument all, it downloads all the modules in the build list (the set of modules and versions selected by MVS). This is the same set of modules shown by go list -m all.

Thanks for confirming. I'm alright with even a bad version of go mod download that over-downloads, since downloading some 2GB of dependencies once is better than redownloading 1GB every time I do a build. 🙂

@gopherbot
Copy link

Change https://golang.org/cl/310073 mentions this issue: cmd/go: explicitly set cfg.BuildMod in commands without -mod flag

@zikaeroh zikaeroh mentioned this issue May 25, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/330913 mentions this issue: cmd/go: call modload.WriteGoMod explicitly, once in each command

@gopherbot
Copy link

Change https://golang.org/cl/336151 mentions this issue: [dev.cmdgo] cmd/go: make fewer 'go mod' commands update go.mod

@gopherbot
Copy link

Change https://golang.org/cl/336152 mentions this issue: [dev.cmdgo] cmd/go: call modload.WriteGoMod explicitly, once in each command

gopherbot pushed a commit that referenced this issue Jul 27, 2021
[dev.cmdgo] cmd/go: make fewer 'go mod' commands update go.mod
2c8acf6 
'go mod graph', 'go mod vendor', 'go mod verify', and 'go mod why'
will no longer edit go.mod or go.sum.

'go mod graph', 'go mod verify', and 'go mod why' may still fetch
files and look up packages as if they were able to update
go.mod. They're useful for debugging and should still work when go.mod
is a little broken.

This is implemented in modload.setDefaultBuildMod based on command
name for now. Super gross. Sorry. This should be fixed with a larger
refactoring for #40775.

Fixes #45551

Change-Id: If5f225937180d32e9a5dd252c78d988042bbdedf
Reviewed-on: https://go-review.googlesource.com/c/go/+/336151
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/341933 mentions this issue: cmd/go: make fewer 'go mod' commands update go.mod

@gopherbot
Copy link

Change https://golang.org/cl/347150 mentions this issue: cmd/go: call modload.WriteGoMod explicitly, once in each command

@bcmills bcmills mentioned this issue Feb 17, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/387920 mentions this issue: doc/go1.18: add release note for changes to automatic go.mod and go.sum updates

@dmitshur dmitshur modified the milestones: Backlog, Go1.18 Feb 24, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Feb 24, 2022
gopherbot pushed a commit that referenced this issue Feb 25, 2022
doc/go1.18: note changes to automatic go.mod and go.sum updates
55e5b03 
Fixes #51242
Updates #45551

Change-Id: Iba6e6acd9a94d24e26fcdd125f1022430723ada7
Reviewed-on: https://go-review.googlesource.com/c/go/+/387920
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Feb 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
4 participants
@jayconrod @dmitshur @gopherbot @zikaeroh