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

proposal: cmd/go: default to & improve -mod=readonly #40728

Open
rsc opened this issue Aug 12, 2020 · 20 comments
Open

proposal: cmd/go: default to & improve -mod=readonly #40728

rsc opened this issue Aug 12, 2020 · 20 comments
Labels
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Aug 12, 2020

For Go 1.16, I propose to default to & improve -mod=readonly, making -mod=mod completely equivalent to -mod=readonly.
This would mean that go commands other than go mod ... and go get no longer update versions in go.mod.
If go.mod needs to be fixed, these commands would report that error and stop.
And the errors in go list should be reported only for the packages that can't be resolved - the overall go list operation should succeed.

Discussed previously on golang-tools@: https://groups.google.com/g/golang-tools/c/BRCgqwWLwoY.

This is what I sent there:

As you probably know, we're trying to make Go 1.16 the release when there are no blockers remaining for anyone to adopt modules, including things like "go get -b" which Jay has been working on.

I'm wondering, as part of this polishing of modules, if we should change the default behavior and stop doing so much updating of go.mod.

Obviously I can justify the current behavior at length, and have in the past, but I think the past thinking falls apart a bit when users make mistakes.

For example, I was working in a module recently (let's call it m) and had a package (let's call it p), and I decided to rename the package (to q). I caught most of the m/p import paths and updated them to m/q. But not all, and the next time I ran "go build", the go command went off trying to find some other module to provide the no-longer-existent m/p. When m/p is a real thing that exists, automatically updating go.mod to use it makes some sense. When m/p is a mistake that needs updating, doing network work just slows down the real fix.

Another example is adding an import you didn't actually want. You might copy a file into a module m from somewhere else, and when you "go build" it adds something to go.mod silently, perhaps even changing the versions of existing require statements. In this case - since you don't want the import - it would be better for the tool to stop and tell you about it instead of doing additional damage that you will have to undo.

I suspect there are many more of these. In general, while the automatic additions to go.mod seemed good on paper, my impression is that they haven't delivered the vision we wanted: they do the wrong thing too often, they slow down the compile-edit-debug cycle for typos, and they are confusing.

We already have a mode that reports errors instead of making changes, namely -mod=readonly. That flag does not apply to "go get" or "go mod" - those commands' job is to update go.mod and they would still do that - but it stops other commands like "go list", "go build", "go test" from changing go.mod. If we made this mode the default and also cleaned up some of its rough edges (go list can probably do a better job reporting partial results, for example), that might go a long way to fixing the problems I listed above.

If we do change the default, it would probably make sense to change -mod=mod (the name for the current automatic updating) to something like -mod=autoupdate, leaving -mod=mod as an alias for that of course.

The command "go get" (with no arguments) has always done a build of the package in the current directory, including downloading any necessary dependencies. With a default -mod=readonly, "go get" would become the way to do a one-time fix-up of go.mod. If "go build" or "go list" fails because of a missing requirement or other problem with go.mod, "go get" would be the way to fix it. If "go test" fails due to an import specific to the test, we'd need to provide a way to deal with that, probably adding back "go get -t", which was the pre-modules way to download the dependencies for a test.

Along with this I think you'd want to change goimports to have a way to say what new requirements it thinks go along with the import paths. Then tooling invoking goimports could present the new imports and the new go.mod requirements together. Users opting in to that kind of automation would still have the option of both. It just wouldn't happen automatically in commands like "go build". (This listing of new go.mod requirements only matters when goimports has exhausted your module's current requirements and is grubbing around in your module cache. When it finds a match, especially a v0, you probably do want to get the version it found and not whatever the go command decides is latest from the internet. So letting goimports specify the new go.mod requirements would correct a sort of race in the current behavior anyway.)

I haven't heard many people suggesting we should default to -mod=readonly before, so maybe others haven't seen the current behavior as too much of a problem. But maybe people just figure it's not worth bringing up because it can't change at this point. I don't believe that's true - if the behavior is wrong, then Go 1.16 would be the time to get it right (and soon).

@rsc rsc added this to the Proposal milestone Aug 12, 2020
@gopherbot gopherbot added the Proposal label Aug 12, 2020
@DisposaBoy
Copy link

@DisposaBoy DisposaBoy commented Aug 12, 2020

I haven't heard many people suggesting we should default to -mod=readonly before, so maybe others haven't seen the current behavior as too much of a problem.

FWIW, I personally started complaining about this whole magic behaviour as early as #29452, but I didn't feel like anyone was taking it seriously so from that point on I essentially stopped participating in the discussion (IIRC I was even invited to comment on a proposal moving things forward which I naturally ignored).

Not only do I think the default behaviour is C@#0%, I'm 99% sure it's a security issue (which I believe I brought up somewhere else). If you make a simple typo, it goes out and pulls the package off the internet with no input from the user. I know this happens in RL because we previously ran an experiment by registering a few common typos of golang.org and observed people pulling package from it. Those requests returned 404 and was before the checksum database, so I don't know the real security impact but I feel like the fact that it was/is possible to some extent is bad enough.

TL;DR

I'm still salty, but IMO this is the best thing to happen to Go modules since their introduction and I support it fully 💯

@rsc
Copy link
Contributor Author

@rsc rsc commented Aug 16, 2020

Another compelling example:

% go run main.g
go: finding module for package main.g
^C
@rasky
Copy link
Member

@rasky rasky commented Aug 26, 2020

Actually, running go run MODULE is a very good way to have people running CLI tools written in Go. Would that be broken by this proposal for people running in this command in a folder where go.mod doesn't even exist?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 26, 2020

@rasky In module mode, when invoked outside a module, go run can't build packages outside the standard library that require a module lookup. You'll get an error like this:

$ GO111MODULE=on go run use.go
use.go:3:8: cannot find module providing package golang.org/x/mod/semver: working directory is not part of a module

That was #32027. Before that, pretty much everything required several network fetches, so builds were slow and non-reproducible. We felt it wasn't a good user experience, especially for new users.

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 27, 2020

@rsc just thinking of side effects of this change, would this change make a go mod download a necessary prestep in CI?

Consider a module which we know to have a complete go.{mod,sum}, that is being tested with an empty module cache (a regular situation on CI). Running go test will necessitate a download of modules, in order to determine the go.{mod,sum} are indeed complete. Are we saying this download will no longer automatically happen? And therefore that a go mod download will be a necessary prestep, that itself will potentially modify the go.{mod,sum} files?

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 27, 2020

@myitcv, -mod=readonly prevents resolving new modules, but it does not prevent the go command from fetching modules that are already both selected by the requirements in the go.mod file and listed in the go.sum file.

So as long as the module is already tidy (or at least complete) when it gets sent to CI, no explicit go mod download should be needed.

@rasky
Copy link
Member

@rasky rasky commented Aug 27, 2020

That was #32027. Before that, pretty much everything required several network fetches, so builds were slow and non-reproducible. We felt it wasn't a good user experience, especially for new users.

Thanks for the pointer, I had missed that discussion. I'm not really happy about the outcome but such is life.

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 27, 2020

@bcmills - thanks, I was forgetting we are actually defaulting to the semantics of -mod=readonly here, despite that being in the title. Minor brain fade. Apologies for the noise.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 31, 2020

Change https://golang.org/cl/251881 mentions this issue: cmd/go: default to -mod=readonly in most commands

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 31, 2020

Change https://golang.org/cl/251879 mentions this issue: cmd/go: let 'go list -mod=readonly' succeed if go.mod is inconsistent

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 9, 2020

Change https://golang.org/cl/253837 mentions this issue: cmd/go: update tests to work with -mod=readonly on by default

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 9, 2020

Change https://golang.org/cl/253745 mentions this issue: cmd/go: refactor modload.Import for better -mod=readonly errors

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 9, 2020

Change https://golang.org/cl/253744 mentions this issue: cmd/go: refactor -mod flag parsing

gopherbot pushed a commit that referenced this issue Sep 11, 2020
For #40728

Change-Id: Ic2b025ff75c6e73c0cb58c1737e44e2a41c71571
Reviewed-on: https://go-review.googlesource.com/c/go/+/253837
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Sep 11, 2020
Keep track of whether the -mod flag was set explicitly. When
-mod=readonly is the default, we'll want to adjust our error messages
if it's set explicitly.

Also, register the -mod, -modcacherw, and -modfile flags in functions
in internal/base instead of internal/work. 'go mod' commands that
don't load packages shouldn't depend on internal/work.

For #40728

Change-Id: I272aea9e19908ba37e151baac4ea8630e90f241f
Reviewed-on: https://go-review.googlesource.com/c/go/+/253744
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Sep 11, 2020
When -mod=readonly is set, Import will now allow imports from
replacements without explicit requirements. With -mod=mod, this would
add a new requirement but does not trigger a module lookup, so it's
determinisitic.

Before reporting an error for an unknown import with -mod=readonly,
check whether the import is valid. If there's a typo in the import,
that's more relevant.

For #40728

Change-Id: I05e138ff76ba3d0eb2e3010c15589fa363deb8d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/253745
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Sep 15, 2020
For #40728

Change-Id: I6618f1b5a632e8b353a483a83bb0cdf4ef6df72c
Reviewed-on: https://go-review.googlesource.com/c/go/+/251881
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>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@abursavich
Copy link

@abursavich abursavich commented Sep 20, 2020

Proposal feedback: My tests running against tip broke and I bisected to find dbde566 as the culprit.

TravisCI lets you specify before_script commands to run before your script/tests, which it executes with your repo as the working directory. One of my before_script commands is go install github.com/mattn/goveralls, which is used to push my coverage profile to coveralls.io but it isn't used by my module.

The new behavior (cannot find module providing package github.com/mattn/goveralls) is surprising. Admittedly, the old behavior of go install modifying your go.mod is also surprising.

I expect this will break a non-trivial number of users.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Sep 20, 2020

@abursavich that case should be covered in combination with #40276 using the side effect free go install pkg@version

@abursavich
Copy link

@abursavich abursavich commented Sep 20, 2020

I'm aware that there are workarounds. By "break" I don't mean to imply it's irreparable. My tests run against the previous minor release, the current release, and tip. I'll probably use GO111MODULE=off go install pkg and won't switch to go install pkg@version until after go 1.17 is released.

I understand the reason for this change and the behavior of the go tool is well documented in tip. However, as a longtime go user running tests against tip, minimally sophisticated enough to find the breaking change in the go tool, I'm here to tell you it's not intuitive that using go install to install a command would ever edit the go.mod. When I want to install a command, I don't want to depend on its module. I would use go get for that.

Side note: Appending a version to switch to "global" mode isn't intuitive or consistent with other go commands either.

@myitcv
Copy link
Member

@myitcv myitcv commented Sep 20, 2020

I'll probably use GO111MODULE=off go install pkg and won't switch to go install pkg@version until after go 1.17 is released.

If you're running tip, then go install pkg@version is the equivalent to what you did previously (because both will take the latest version of pkg).

Out of interest, if you are running tip, why would you delay switching to this approach until after 1.16 (I assume you meant 1.16, as opposed to 1.17)?

@abursavich
Copy link

@abursavich abursavich commented Sep 20, 2020

Out of interest, if you are running tip, why would you delay switching to this approach until after 1.16 (I assume you meant 1.16, as opposed to 1.17)?

With TravisCI the same before_script commands run against each environment (e.g. go version) you're using. I'm running against oldstable (the previous minor release), stable (the current release), and tip. Since go install pkg@version is new in go 1.16, I'll wait until go 1.15.x ages out, oldstable is 1.16.x, and stable is 1.17.x.

FWIW, I think I actually need GO111MODULE=off go get github.com/mattn/goveralls.

@abursavich
Copy link

@abursavich abursavich commented Sep 20, 2020

Maybe I should take my comments to CL 243077, but we're here for now.

I am a big proponent of differentiating get and install. However, I think there's an orthogonal discussion about whether certain commands should:

  1. modify go.mod
  2. fallback to the network

I think install should not modify go.mod and it should fallback to the network. If trying to install a command not in the current module, go install package should behave the same as go install package@latest. This would make it symmetric with the familiar behavior of get.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Sep 21, 2020

@abursavich

Thanks for the feedback on this issue. We know this is an incompatible change to the CLI. We're trying not to cause any more disruption than necessary, but we think it's more important in the long term to get the default behavior right.

I understand the reason for this change and the behavior of the go tool is well documented in tip. However, as a longtime go user running tests against tip, minimally sophisticated enough to find the breaking change in the go tool, I'm here to tell you it's not intuitive that using go install to install a command would ever edit the go.mod. When I want to install a command, I don't want to depend on its module. I would use go get for that.

In module mode (and arguably in GOPATH mode, too), go install and go get have had a lot of overlap. We'd like to distinguish them further: go install may eventually just be used for installing executables. go get may just be used for managing dependencies in go.mod.

Side note: Appending a version to switch to "global" mode isn't intuitive or consistent with other go commands either.

In short, we're trying to address two use cases with go install in module mode: installing executables in the context of the current module, and installing executables without regard for the current module (if there even is one).

Installing tools from the current module is an important use case. In fact, it's one I'd recommend in CI. You could add a requirement for some minimum version of the tool's module, then go install that. If you don't want to keep tool requirements in go.mod, you could put them in another .mod file specified with -modfile.

Of course that's not always desirable, so there needs to be a way to install tools regardless of the current module. #40276 is the main issue for that. There's been extensive discussion (and previously in #30515), so if you feel that this should work differently, please comment there.

I think install should not modify go.mod and it should fallback to the network. If trying to install a command not in the current module, go install package should behave the same as go install package@latest. This would make it symmetric with the familiar behavior of get.

This would make go install too unpredictable. What if some dependencies are in go.mod but not others? We want module commands to be repeatable: the set of versions they use should either 1) come from go.mod, 2) should be written to go.mod if something new is needed, or 3) the command line should indicate that it's not repeatable (e.g., @latest).

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
9 participants
You can’t perform that action at this time.