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 verify' does not respect -mod=readonly #31372

Open
jsha opened this issue Apr 9, 2019 · 8 comments

Comments

@jsha
Copy link

commented Apr 9, 2019

$ go version
go version go1.12 linux/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="/home/jsha/.cache/go-build"
GOEXE=""
GOFLAGS="-mod=vendor"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jsha/gopkg"
GOPROXY=""
GORACE=""
GOROOT="/home/jsha/go1.12"
GOTMPDIR=""
GOTOOLDIR="/home/jsha/go1.12/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jsha/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-build911932552=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Checked out https://github.com/letsencrypt/boulder/
  2. Ran go mod init to create a go.mod file.
  3. Committed the go.mod file (git commit go.mod -m 'Use modules')
  4. Ran go mod verify
  5. Ran git diff

What did you expect to see?

No output.

What did you see instead?

Several lines changed.

Specifically, what happened here is that Boulder depends on a number of packages. Some of those packages have opted into modules, and have their own dependencies listed in go.mod.

For instance, as of right now, Boulder has golang.org/x/crypto vendored at 0709b304 using godep. go mod init correctly picks up that version and puts it in go.mod. Boulder also depends on challtestsrv v1.0.2. challtestsrv v1.0.2 has a go.mod that depends on golang.org/x/crypto 505ab145 (which is later than 0709b304). Running go mod verify changes Boulder's go.mod to depend on 505ab145.

It seems like go mod verify is performing the minimal version selection algorithm and updating go.mod. I would expect that go mod verify doesn't change anything, based on the documentation, which says:

Verify checks that the dependencies of the current module, which are stored in a local downloaded source cache, have not been modified since being downloaded. If all the modules are unmodified, verify prints "all modules verified." Otherwise it reports which modules have been changed and causes 'go mod' to exit with a non-zero status.

@jsha

This comment has been minimized.

Copy link
Author

commented Apr 9, 2019

A helpful soul linked me to https://golang.org/cmd/go/#hdr-The_go_mod_file, which explains:

Because the module graph defines the meaning of import statements, any commands that load packages also use and therefore update go.mod, including go build, go get, go install, go list, go test, go mod graph, go mod tidy, and go mod why.

This seems like a very important piece of information (various go commands modify go.mod as a side effect). It should probably be duplicated or linked to from https://github.com/golang/go/wiki/Modules and the go cmd documentation (https://godoc.org/cmd/go#hdr-Verify_dependencies_have_expected_content).

Also, it seems like this is not really the right behavior. For commands that are meant to answer questions rather than change state (go mod verify, go mod why,go mod graph), it seems like there's no reason to edit go.mod.

Relatedly, the various build-related commands (go get, go build, etc) observe the -mod=readonly flag, but the go mod series of commands doesn't take that flag. It seems like it would be good to consistently support -mod=readonly.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

go mod init converts requirements from other files, but doesn't check them for consistency or recursively resolve their dependencies: that's left to go build, go test, go mod tidy, etc., so that if there is a problem you can start from the go mod init output and hand-edit to bring things up to a reasonable state.

On the other hand, commands that answer questions need to at least resolve enough of the dependency graph to answer the question that was asked. For example, go mod verify needs to know which of the already-downloaded modules in the cache are dependencies of the current module, and once it has resolved that information it updates go.mod to incorporate what it has learned.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

but the go mod series of commands doesn't take that flag

See #26850.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Probably go mod verify should at least accept -mod=readonly, since it isn't a command that specifically modifies dependencies.

@bcmills bcmills changed the title cmd/go: mod verify changes go.mod cmd/go: 'mod verify' does not respect -mod=readonly Apr 10, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@bcmills bcmills added this to the Go1.13 milestone Apr 10, 2019

@jayconrod jayconrod self-assigned this Apr 10, 2019

@haosdent

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Hi, @jayconrod If I add a cfg.BuildMod == "readonly" check in func WriteGoSum() {, it should resolve this issues, right?

@gopherbot

This comment has been minimized.

Copy link

commented Apr 28, 2019

Change https://golang.org/cl/174258 mentions this issue: cmd/go: respect -mod=readonly when performing 'mod verify'

@jayconrod

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I wasn't able to reproduce this using the original instructions, but after replacing the crypto requirement with golang.org/x/crypto v0.0.0-20180904163835-0709b304e793, I see this happening.

go mod verify doesn't actually load packages. It loads go.mod files for the main module and all transitively required modules. It performs MVS in order to resolve conflicting requirements. If the main module requires an older version of a module, it will write back the selected version of the module to go.mod. I think -mod=readonly should fail if this kind of update would be performed for consistency with other commands.

More importantly though, -mod=readonly should prevent go.sum updates (#30667 suggests doing this for go build). If a sum is missing, go mod verify -mod=readonly should fail.

We're in code freeze now, so I don't think this will be in for 1.13.

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