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

Jump to bottom

cmd/go: go mod tidy does not always remove unneeded entries #33008

Closed
rogpeppe opened this issue Jul 9, 2019 · 12 comments
Closed

cmd/go: go mod tidy does not always remove unneeded entries #33008

rogpeppe opened this issue Jul 9, 2019 · 12 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@rogpeppe
Copy link
Contributor

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() {
}
@myitcv
Copy link
Member

myitcv commented Jul 9, 2019

cc @bcmills @jayconrod

@myitcv myitcv added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 9, 2019
@bcmills bcmills added this to the Go1.14 milestone Jul 9, 2019
@bcmills
Copy link
Contributor

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:

go/src/cmd/go/internal/modfetch/fetch.go

Lines 584 to 585 in 06ef108

// 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 The path to resolution is known, but the work has not been done. label Jul 9, 2019
@bcmills bcmills modified the milestones: Go1.14, Unplanned Jul 9, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 9, 2019
@jayconrod
Copy link
Contributor

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

bcmills commented Jul 9, 2019

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

@bcmills
Copy link
Contributor

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

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

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

bcmills commented Jul 9, 2019 •
edited

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

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

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.

cofyc added a commit to cofyc/tidb-operator that referenced this issue Oct 17, 2019
@cofyc
workaround for golang/go#33008
25d9894
@bcmills bcmills mentioned this issue Nov 6, 2019
Closed
@honnix honnix mentioned this issue Dec 9, 2019
Merged
@bcmills bcmills mentioned this issue Jan 22, 2020
Closed
@bcmills bcmills mentioned this issue Feb 21, 2020
Open
@jayconrod jayconrod mentioned this issue Mar 13, 2020
Closed
@camilamacedo86 camilamacedo86 mentioned this issue Apr 2, 2020
Merged
@anyasabo anyasabo mentioned this issue May 14, 2020
Merged
1 task
@bcmills bcmills mentioned this issue Jun 6, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/237017 mentions this issue: cmd/go: don't save sums for modules loaded for import resolution

@menghanl menghanl mentioned this issue Jun 22, 2020
Closed
@menghanl menghanl mentioned this issue Aug 5, 2020
Closed
@carlaKC carlaKC mentioned this issue Oct 12, 2020
Merged
@gopherbot
Copy link

Change https://golang.org/cl/262781 mentions this issue: cmd/go: save sums for zips needed to diagnose ambiguous imports

gopherbot pushed a commit that referenced this issue Oct 23, 2020
cmd/go: save sums for zips needed to diagnose ambiguous imports
4fdb98d 
Previously, we would retain entries in go.sum for .mod files in the
module graph (reachable from the main module) and for .zip files
of modules providing packages.

This isn't quite enough: when we load a package, we need the content
of each module in the build list that *could* provide the package
(that is, each module whose path is a prefix of the package's path) so
we can diagnose ambiguous imports.

For #33008

Change-Id: I0b4d9d68c1f4ca382f0983a3a7e537764f35c3aa
Reviewed-on: https://go-review.googlesource.com/c/go/+/262781
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
@bcmills bcmills modified the milestones: Unplanned, Go1.16 Oct 26, 2020
@bradleypeabody bradleypeabody mentioned this issue Jan 19, 2021
Merged
@ZzEeKkAa ZzEeKkAa mentioned this issue Apr 6, 2021
Merged
@golang golang locked and limited conversation to collaborators Oct 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
6 participants
@rogpeppe @jayconrod @av86743 @myitcv @bcmills @gopherbot