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' should not modify the go.mod file #31372

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

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

jsha opened this issue Apr 9, 2019 · 9 comments
Labels
modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@jsha
Copy link

jsha 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 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
Copy link
Author

jsha 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
Copy link
Contributor

bcmills 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
Copy link
Contributor

bcmills commented Apr 9, 2019

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

See #26850.

@bcmills
Copy link
Contributor

bcmills 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 bcmills added modules NeedsFix The path to resolution is known, but the work has not been done. labels Apr 10, 2019
@bcmills
Copy link
Contributor

bcmills commented Apr 10, 2019

CC @jayconrod

@haosdent
Copy link
Contributor

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

@gopherbot
Copy link

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

@jayconrod
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Sep 19, 2019

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.

@bcmills bcmills changed the title cmd/go: 'mod verify' does not respect -mod=readonly cmd/go: 'mod verify' should not modify the go.mod file Sep 26, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@jayconrod jayconrod removed their assignment Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants