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: readonly does not affect all go mod sub-commands #26850

Closed
dsnet opened this issue Aug 7, 2018 · 9 comments

Comments

Projects
None yet
8 participants
@dsnet
Copy link
Member

commented Aug 7, 2018

Using go.11beta3

In https://golang.org/cl/127916, I'm working a test.bash script that is intended to be used a pre-submit and post-submit hook. In that script, I rely on go mod tidy and go mod vendor to manage dependencies (I use "vendor" so that I can use the same set of dependencies when testing with Go1.9 and Go1.10).

One of the properties of the test.bash script is the ability to detect when the go.mod and go.sum files are out of sync with the source code (e.g., a user submitted code that changed the dependency tree but forgot to also submit the updated go.mod file).

Currently, since -mod=readonly does not affect go mod tidy or go mod vendor, I need to shell out to git diff to check whether the go.mod or go.sum files were altered. I would have expected -mod=readonly to affect "tidy" and "vendor" in a way such that:

  • If the current files on disk are exactly what it would have outputted, it does nothing
  • If they would have had to make changes, they would fail with a non-zero error code.

\cc @bcmills

@bcmills

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

@bcmills bcmills added this to the Go1.11 milestone Aug 7, 2018

@thepudds

This comment has been minimized.

Copy link

commented Aug 8, 2018

@gopherbot please add modules label

@gopherbot gopherbot added the modules label Aug 8, 2018

@myitcv

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

I use "vendor" so that I can use the same set of dependencies when testing with Go1.9 and Go1.10

Nice approach.

Switches to using that in CI

I rely on go mod tidy...

Just to confirm, you're using this as a step to check that the committed state of go.{mod,sum} is "current" with respect to the module's code, correct?

i.e. it's independent to the step of go mod vendor which you're using to pre-warm a vendor directory that is then used to ensure Go 1.9 and 1.10 pass in CI.

(Just a side question; go mod vendor will work for everything but tool dependencies as far as I can tell; assuming that's your experience/expectation?)

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

This is working as documented. From go help modules:

If invoked with -mod=readonly, the go command is disallowed from the implicit
automatic updating of go.mod described above. Instead, it fails when any changes
to go.mod are needed. This setting is most useful to check that go.mod does
not need updates, such as in a continuous integration and testing system.
The "go get" command remains permitted to update go.mod even with -mod=readonly,
and the "go mod" commands do not take the -mod flag (or any other build flags).

The idea is that you can set GOFLAGS=-mod=readonly
and then do your ordinary work and then not have to clear it
when you invoke a command whose entire reason to exist
is to update go.mod, like go get or go mod edit or go mod tidy.
go mod vendor is a bit of a corner case but still seems OK as is.

In any event, it's very clear that these commands don't pay attention
to -mod=readonly since they fail if you try that:

$ go mod tidy -mod=readonly
flag provided but not defined: -mod
usage: go mod tidy [-v]
Run 'go help mod tidy' for details.
$ go mod vendor -mod=readonly
flag provided but not defined: -mod
usage: go mod vendor [-v]
Run 'go help mod vendor' for details.
$ 

I think this is working the right way.

You could use 'go list all -mod=readonly' to check that at least for the current GOOS/GOARCH no changes are needed.

@natefinch

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

The "go get" command remains permitted to update go.mod even with -mod=readonly

Could we not? go get remains useful even with the flag respected. It would be like go mod download except it gets transitive dependencies, too (useful if you're about to get on a plane). It would also serve as sort of a fix for the bug (which I can't currently find) where go get used to install a tool globally, then adds that repo to the current project's go.mod.

It could also serve as a fix for #27643 - for people that don't like having implicit changes to their go.mod, they could just set GOFLAGS=-mod=readonly and go on about their business.

I think it would be useful to have a flag to tell tidy to just report if it would change the go.mod without doing so. I don't think telling people to use go list is a good fix for that, because it's quite unintuitive. What people are usually going to be checking for is "did someone forget to run go mod tidy?" And I think the command to do that should be a variation of go mod tidy... not a conceptually unrelated command.

@myitcv

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

I think the crux of this issue is covered by #27005, hence I'm going to close this.

If someone feels like we need to keep this issue open for go mod vendor please shout and we can re-open.

@jsha

This comment has been minimized.

Copy link

commented Apr 9, 2019

Over in #31372, I have another use case for this issue. Specifically go mod verify (and go mod why and go mod graph) don't support the -mod=readonly flag, but it would make a lot of sense for them to support it. These are notionally read-only commands that tell you something about the state of your dependencies. They don't fall under the category of "command[s] whose entire reason to exist
is to update go.mod."

@jsha

This comment has been minimized.

Copy link

commented Apr 20, 2019

@myitcv Based on my comment above, would you be willing to reopen this issue?

@myitcv

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@jsha - I would raise a new issue. Apologies, I'm fairly out the loop on all this stuff, but as a rule closed issues don't get many eyes looking at them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.