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: go mod tidy does not always remove unneeded entries #33008

Open
rogpeppe opened this issue Jul 9, 2019 · 10 comments
Labels
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jul 9, 2019

$ go version
go version devel +d410642f49 Mon Jul 1 21:30:23 2019 +0000 linux/amd64

There was an unnecessary entry in a go.sum file that wasn't being removed by go mod tidy.

Here's a testscript script that reproduces the issue:

go mod tidy
grep -count=1 'fastuuid v[0-9.]+ ' go.sum

# When we use the go.sum file with an extra
# fastuuid entry, it should remove it, but it doesn't.
cp go.sum-extra-fastuuid go.sum
go mod tidy
grep -count=1 'fastuuid v[0-9.]+ ' go.sum

-- go.mod --
module m

go 1.13

require (
	github.com/heetch/sqalx v0.4.0
	github.com/rogpeppe/fastuuid v1.2.0
)
-- go.sum-extra-fastuuid --
github.com/rogpeppe/fastuuid v1.1.0 h1:INyGLmTCMGFr6OVIb977ghJvABML2CMVjPoRfNDdYDo=
github.com/rogpeppe/fastuuid v1.1.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
github.com/rogpeppe/fastuuid v1.2.0 h1:Ppwyp6VYCF1nvBTXL3trRso7mXMlRrw9ooo375wvi2s=
github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
-- m.go --
package main

import (
	_ "github.com/heetch/sqalx"
	_ "github.com/rogpeppe/fastuuid"
)

func main() {
}
@rogpeppe rogpeppe added the modules label Jul 9, 2019
@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jul 9, 2019

@bcmills bcmills added this to the Go1.14 milestone Jul 9, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

Yeah, the whole abstraction around the go.sum file is a bit unfortunate. In this case the problem is with the modfetch.TrimGoSum API, which does not distinguish between module dependencies and code dependencies:

// TrimGoSum trims go.sum to contain only the modules for which keep[m] is true.
func TrimGoSum(keep map[module.Version]bool) {

@bcmills bcmills added the NeedsFix label Jul 9, 2019
@bcmills bcmills modified the milestones: Go1.14, Unplanned Jul 9, 2019
@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jul 9, 2019

It looks like in this particular case, the entry for github.com/rogpeppe/fastuuid v1.1.0/go.mod is necessary. go mod graph shows this line:

github.com/heetch/sqalx@v0.4.0 github.com/rogpeppe/fastuuid@v1.1.0

The go command loads the go.mod file from that version in order to apply constraints, and we need a sum for that go.mod file.

I think this is working as intended, but we should document it better.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

We should fix this, but it's lower priority than a lot of other bugs.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

In particular, go mod tidy should ideally remove this line:

github.com/rogpeppe/fastuuid v1.1.0 h1:INyGLmTCMGFr6OVIb977ghJvABML2CMVjPoRfNDdYDo=
@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

@rogpeppe rogpeppe commented Jul 9, 2019

It looks like in this particular case, the entry for github.com/rogpeppe/fastuuid v1.1.0/go.mod is necessary.

Yes, that entry is necessary, but there is a bug here, as @bcmills reiterates, which is that the non-go.mod line is still there and shouldn't be.

In general, AIUI, the go.sum entry after running go mod tidy should be entirely independent of the original contents of go.sum, but in this case it is not.

@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Jul 9, 2019

@bcmills

Yeah, the whole abstraction around the go.sum file is a bit unfortunate...

Perhaps you could elaborate on the subject a bit more? Thank you in advance.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

@av86743, the go.sum logic is deeply integrated into the cmd/go/internal/modfetch package, but the format of that file and the logic for fetching its contents are not tightly coupled to the procedure for fetching modules (from version control or from a proxy).

Following the advice of Parnas '72, which I believe to be generally sound, the go.sum abstraction should be isolated to its own package.

@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Jul 9, 2019

@bcmills

Any intentions (or perhaps an open CL) to refactor go.sum abstraction, the way you have outlined it?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

@av86743, I had a CL started for the 1.13 cycle but missed the freeze. I may dust it off for 1.14. Mostly it moved a bunch of functions from modfetch to a new package.

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