golang/go

cmd/go: 'mod verify' should not modify the go.mod file

jsha opened this issue · 9 comments

jsha commented 
$ 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 505ab14 (which is later than 0709b30). Running go mod verify changes Boulder's go.mod to depend on 505ab14.

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 commented

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 commented

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 commented

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

See #26850.

bcmills commented

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

haosdent commented

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

gopherbot commented

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

jayconrod commented

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.

bcmills commented

The -mod flag is a build flag, and does not affect go mod subcommands in general.

On the other hand, perhaps go mod verify should fail entirely if the go.mod file is inconsistent, regardless of the -mod flag.

  • 👍1