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: in module root, go list {module-root}/... behaves differently than go list ./..., yet go mod tidy removes go.sum entry needed for former #51283

Open
dmitshur opened this issue Feb 20, 2022 · 9 comments
Assignees
Labels
GoCommand modules NeedsInvestigation Thinking
Milestone

Comments

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 20, 2022

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

$ go version
go version go1.18rc1 darwin/arm64

Does this issue reproduce with the latest release?

It reproduces with go1.18rc1.

Not really viable to test with Go 1.17.7 because it happens in a module whose go.mod file has go 1.18, so go1.17.7 mod tidy refuses to run on it:

$ go1.17.7 mod tidy
go mod tidy: go.mod file indicates go 1.18, but maximum supported version is 1.17

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/dmitri/Library/Caches/go-build"
GOENV="/Users/dmitri/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/dmitri/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/dmitri/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18rc1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_0/h0671fcn4rgb5pn9c745dx2h0000gn/T/go-build1161277448=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran go mod tidy inside a module that declares go 1.18. It removed one line from go.sum. Then I tried running go list {module-root}/... and go list ./... while being at the module root directory, without using the new workspace mode (that is, go env GOWORK output was blank).

(See the bottom of this issue for a sequence of exact commands that reproduce the issue.)

What did you expect to see?

I expected both go list {module-root}/... and go list ./... to work, and produce identical output, given the current working directory is the module root.

(I also expected that the amount of work needed to process these two relative and full import path patterns would be approximately the same. If that's not the case, I'm interested in learning what makes them different.)

What did you see instead?

Running go mod tidy causes the following line to be removed from go.sum:

golang.org/x/exp v0.0.0-20190731235908-ec7cb31e5a56 h1:estk1glOnSVeJ9tdEZZc5mAMDZk5lNJNyJ6DvrBkTEU=

Then, when I ran go list {module-root}/..., it failed:

$ go list golang.org/x/exp/shiny/...
pattern golang.org/x/exp/shiny/...: golang.org/x/exp@v0.0.0-20190731235908-ec7cb31e5a56: missing go.sum entry

Notably, it's saying the go.sum entry from golang.org/x/exp@v0.0.0-20190731235908-ec7cb31e5a56 is missing, the very same one that go mod tidy removed earlier.

go list ./... works well.

$ go list ./...
golang.org/x/exp/shiny/driver
golang.org/x/exp/shiny/driver/gldriver
[...]
golang.org/x/exp/shiny/widget/node
golang.org/x/exp/shiny/widget/theme
$ go list ./... | shasum
19cedcfaf7f294abf33a6ef54a81560517aa8a8d  -

If I manually add the missing go.sum entry back, then the longer go list invocation produces an identical result:

$ go list golang.org/x/exp/shiny/... | shasum
19cedcfaf7f294abf33a6ef54a81560517aa8a8d  -

Sequence of commands to reproduce

export GOPATH=$(mktemp -d)
cd $(mktemp -d)
git clone https://go.googlesource.com/exp && cd exp
git checkout 6cf2b201936e0673ace5b31d2dffa00dafb3ee58
cd shiny
go mod tidy
go list ./...
go list golang.org/x/exp/shiny/...
echo $?

CC @bcmills, @matloob.

@dmitshur dmitshur added NeedsInvestigation GoCommand modules labels Feb 20, 2022
@dmitshur dmitshur added this to the Go1.18 milestone Feb 20, 2022
@dmitshur
Copy link
Contributor Author

@dmitshur dmitshur commented Feb 20, 2022

Note, this issue was discovered in CL 387014 because the Go build system currently runs go test {module-root}/... invocations in each module root, rather than the shorter (and I expect more commonly used by users) go test ./... invocation. (CC @nigeltao.)

I'm not sure which format is better for TryBots to use (I thought they'd be pretty much interchangeable), and it's not hard for us to change it if desired. But that should be a separate issue.

@dmitshur
Copy link
Contributor Author

@dmitshur dmitshur commented Feb 20, 2022

Also worth noting a difference in error text between when go list is given an import path pattern vs the import path corresponding to the module root (which happens not to have a Go package in it):

$ go list golang.org/x/exp/shiny/...
pattern golang.org/x/exp/shiny/...: golang.org/x/exp@v0.0.0-20190731235908-ec7cb31e5a56: missing go.sum entry

$ go list golang.org/x/exp/shiny    
missing go.sum entry for module providing package golang.org/x/exp/shiny; to add:
	go mod download golang.org/x/exp

Notice the helpful go mod download golang.org/x/exp suggestion is included only in the latter case. Running it re-adds the go.sum entry that tidy removed.


The Go 1.18 draft release notes mention:

The go mod tidy command now retains additional checksums in the go.sum file for modules whose source code is needed to verify that each imported package is provided by only one module in the build list.

Since x/exp/shiny only recently became a separate module (previously all of its packages were inside x/exp), and neither x/exp nor x/exp/shiny modules provide a package at import path golang.org/x/exp/shiny, this makes me think that's possibly related. Maybe the fix for that tidy issue (#47738) is not handling this edge case?

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 22, 2022

go list ./... stops at module boundaries, whereas go list $(GOWORK=off go list -m)/... does not.
(#43733 proposes to change that, but that's still deep in the backlog and may or may not actually happen.)

Since x/exp/shiny only recently became a separate module (previously all of its packages were inside x/exp), and neither x/exp nor x/exp/shiny modules provide a package at import path golang.org/x/exp/shiny, this makes me think that's possibly related.

Indeed. The reason go list golang.org/x/exp/shiny/... fails is because the go command is trying to determine whether the required version of the x/exp module (which may be arbitrarily different from the version contained in the parent of the working directory) provides any package matching golang.org/x/exp/shiny/....

However, go mod tidy does not preserve the checksum for golang.org/x/exp because it isn't needed: all of the x/exp packages actually imported within x/exp/shiny/... are contained within the shiny module itself, and x/exp is only in the module graph of x/exp/shiny at all as an otherwise-irrelevant transitive dependency of x/mobile. Because x/exp isn't explicitly required and x/exp/shiny is at go 1.18, lazy module loading takes effect and the irrelevant dependency is not used at all during package loading, so its missing checksum doesn't cause any problems when building packages within the module.

So unfortunately, I'm not entirely sure about the right path forward here. It seems wasteful in the general case to add a checksum for the selected version of x/exp, since that module isn't actually relevant to the build and (especially if the repo is private) the checksum could potentially be fairly expensive to compute. On the other hand, it also seems wrong for go list to potentially under-report the set of packages matching the requested pattern. 🤔

@bcmills bcmills removed this from the Go1.18 milestone Feb 22, 2022
@bcmills bcmills added this to the Go1.19 milestone Feb 22, 2022
@bcmills bcmills self-assigned this Feb 22, 2022
@dmitshur
Copy link
Contributor Author

@dmitshur dmitshur commented Feb 22, 2022

go list ./... stops at module boundaries, whereas go list $(GOWORK=off go list -m)/... does not.

Ah, this distinction is very helpful to be aware of and certainly explains this, thanks.

Do you know if it is already documented somewhere? I tried looking at https://pkg.go.dev/cmd/go#hdr-Relative_import_paths (and the rest of that page) and https://go.dev/ref/mod, but couldn't spot it so far. (Perhaps documenting it is a part of future work, if so, that's not a problem.)

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 22, 2022

I expected the difference to be mentioned in https://pkg.go.dev/cmd/go@master#hdr-Package_lists_and_patterns, but it doesn't appear to be there. 🤔

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Mar 3, 2022

So is this all Working As Intended?

Note, this issue was discovered in CL 387014

Getting back to this CL, if "go mod tidy" is still a no-op in my x/exp/shiny directory, should I manually edit the go.sum file, in order to pass the trybots?

@dmitshur
Copy link
Contributor Author

@dmitshur dmitshur commented Mar 3, 2022

@nigeltao Take a look at #51283 (comment). I suggest using go mod download golang.org/x/exp to update the go.sum file, instead of manually editing it.

Since this issue is still open, I understand there's a chance something will change in the future, but existing Go versions are working as described above.

I've opened #51455 to consider changing the TryBot behavior to test packages in each module via the "./..." import path pattern instead of "{module-root}/...".

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

I think #51455 is clearly the correct short-term fix.

I do not see manual edits to the go.sum file as a viable workflow, and I also don't think it's viable to make drastic changes to the cmd/go pattern-match semantics at this time — especially not that could be backported to Go 1.18 and earlier.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

For a longer-term fix, we could consider adding a go.work file at the root of x/exp and including all of its modules.

However, that would lose some test fidelity: it would test each package against the MVS solution for the x/exp workspace overall instead of the requirements declared in each x/exp module's go.mod file individually, and the selected versions for the workspace overall may be substantially upgraded as compared to the selected versions for each individual module.

I think if we did that we would also need the TryBots to check that go work sync is a no-op for the workspace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand modules NeedsInvestigation Thinking
Projects
None yet
Development

No branches or pull requests

3 participants