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 -sync should trim go.sum #26381

Closed
ainar-g opened this issue Jul 14, 2018 · 7 comments
Closed

cmd/go: mod -sync should trim go.sum #26381

ainar-g opened this issue Jul 14, 2018 · 7 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Jul 14, 2018

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

go version devel +6f96cf2 Fri Jul 13 22:29:48 2018 +0000 linux/amd64

What did you do?

$ mkdir /tmp/foo && cd /tmp/foo
$ echo 'package foo // import "foo"' > main.go
$ go mod -init
$ go get github.com/lib/pq
$ cat go.sum
github.com/lib/pq v0.0.0-20180523175426-90697d60dd84/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
$ go mod -sync
$ cat go.sum

What did you expect to see?

Nothing.

What did you see instead?

github.com/lib/pq v0.0.0-20180523175426-90697d60dd84/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
@oiooj oiooj added the modules label Jul 14, 2018
@oiooj oiooj added this to the Go1.11 milestone Jul 14, 2018
@kardianos
Copy link
Contributor

Files in go.sum are never intended to be removed. It keeps a list of all modules seen.

@ainar-g
Copy link
Contributor Author

ainar-g commented Jul 14, 2018

That approach (if indeed the defined behaviour) is (1) badly documented, (2) probably wouldn't scale very well, (3) raises questions.

Regarding (1) here is what go help modules had to say about go.sum:

The go command maintains, in the main module's root directory alongside
go.mod, a file named go.sum containing the expected cryptographic checksums
of the content of specific module versions. Each time a dependency is
used, its checksum is added to go.sum if missing or else required to match
the existing entry in go.sum.

I added the emphasis. My assumptions from reading this section are:

  • "Maintaining" means adding what is used and removing what is unused
  • A dependency is "used" when one of packages in the module imports it.

By (2) I mean that when we consider a project like Hugo, with hundreds of dependencies being added, removed, and updated daily in several branches. go.sum would accumulate trash very quickly. (Ideally, go mod -sync would remove any line that is not a hash of a used package, so that resolving git conflicts is as easy as fixing go.mod and running go mod -sync.)

By (3) I simply mean that the "Go 1.11 Modules Experiment" comes with a number of tools to support modules in a project. So why not include a tool to maintain go.sum? Don't get me wrong, I can write an awk script to do that, but why, if almost any project will need something like that?

@myitcv
Copy link
Member

myitcv commented Jul 14, 2018

/cc @rsc @bcmills for confirmation of the intended semantics here.

@myitcv myitcv added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 14, 2018
@bcmills
Copy link
Contributor

bcmills commented Jul 16, 2018

I'd like to get @FiloSottile's input on what we should do here.

On the one hand, the more we leave in the go.sum file the more likely we are to catch actual verification failures, particularly if the dependencies in the go.sum are likely to be added again in the future (for example, if they are more recent versions of modules that we do still require). On the other hand, it does seem a bit strange to prune modules out of go.mod without also pruning them out of go.sum.

@FiloSottile
Copy link
Contributor

I appreciate the fact that go.sum is append only. It secures the case of reintroducing a dependency and improves future cross-pollination experiments where hashes are learned from other repositories.

One way to look at this is that go.sum is just a way (currently, the only one) to check package integrity, not really bound to go.mod. In the future we might have system-wide go.sum, or remote auditable services instead.

For projects so big it becomes a problem, there can be an external tool to trim go.sum.

@rsc
Copy link
Contributor

rsc commented Jul 17, 2018

The current "add only" behavior (the file is sorted so it's not exactly "append-only") was intended. I think that was more defensible when it was opt-in. Now that it's a required part of development (at @FiloSottile's urging), I do think we owe it to ourselves to make it a bit nicer, and that probably does include throwing away things that are no longer needed.

Right now there's too much path dependence: if you go get something, decide not to use it, and drop it, ideally you would be back where you started and instead there's scar tissue left in go.mod go.sum. In the worst case you're leaking names of things you might have tried that you don't even want people to know exist. And since the file is not exactly human-friendly it would be easy for unfortunate diffs to be missed in review.

I think you're right that go mod -sync is a good time to clean it up. It's not completely trivial but we should do it.

@rsc rsc changed the title cmd/go: go mod -sync removes a module from go.mod but not go.sum cmd/go: mod -sync should trim go.sum Jul 17, 2018
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 17, 2018
@gopherbot
Copy link

Change https://golang.org/cl/124713 mentions this issue: cmd/go: scrub go.sum during go mod -sync

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants