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 get -u in module mode can fail and produce output that's not easy to diagnose #30455

Closed
dmitshur opened this issue Feb 28, 2019 · 17 comments

Comments

Projects
None yet
3 participants
@dmitshur
Copy link
Member

commented Feb 28, 2019

This issue is related to issue golang/lint#436. That issue is about a problem in the lint repository. This issue is about a problem in cmd/go.

We added a go.mod file to the lint repository in CL 162913 today, following the process described in https://golang.org/issue/28136#issuecomment-462971974. We quickly learned via an issue report at golang/lint#436 that doing so caused go get -u golang.org/x/lint to begin to fail when in module mode. Previously, go get -u golang.org/x/lint succeeded in module mode. go get golang.org/x/lint, without -u continues to work.

The go.mod file specifies the module path to be "golang.org/x/lint", which matches the import path comment of the lint package. The import path comment means no current Go package can import the lint package at an import path other than golang.org/x/lint successfully. However, older versions of modules can and do.

As @thepudds shared in golang/lint#436 (comment), there is an old module version that refers to lint repository via an incorrect github.com/golang/lint path:

That latest version of the github.com/openzipkin/zipkin-go module is currently v0.1.5. That version requires the bad old grpc version:

https://github.com/openzipkin/zipkin-go/blob/v0.1.5/go.mod#L27

What ends up happening is that installing the lint module ends up pulling in many modules incidentally (x/tools module -> appengine module -> ... -> x/build module -> ...). Some module ends up pulling in the latest module of zipkin-go, which in turn pulls in the bad grpc version. Since -u flag is used, go get fetches the latest pseudo-version of the github.com/golang/lint module, which is now v0.0.0-20190227174305-5b3e6a55c961 and includes a go.mod file stating the module path to be golang.org/x/lint, causing the error:

go: github.com/golang/lint@v0.0.0-20190227174305-5b3e6a55c961: parsing go.mod: unexpected module path "golang.org/x/lint"

There are a few issues here that should be investigated. For example, the error message from go get -u can be improved to make diagnosing the situation better. Perhaps more.

/cc @bcmills @jayconrod @matloob @thepudds

@dmitshur dmitshur changed the title cmd/go: cmd/go: go get -u in module mode can fail and produce output that's not easy to diagnose Feb 28, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

the error message from go get -u can be improved to make diagnosing the situation better.

That's #28489.

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Some module ends up pulling in the latest module of zipkin-go, which in turn pulls in the bad grpc version.

One workaround might be to explicitly upgrade google.golang.org/grpc above v1.16.0 in the requirements of golang.org/x/lint. That way, when someone runs go get -u, the invalid path is no longer in the transitive import graph and its module should not be included.

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@bcmills That sounds like a valid quick fix for golang.org/x/lint. Do you think we should go ahead and implement it (say, tomorrow), or shall we wait a bit for openzipkin/zipkin-go#115 to get merged (and tagged), making it no longer necessary?

@dmitshur dmitshur added this to the Go1.13 milestone Feb 28, 2019

@thepudds

This comment has been minimized.

Copy link

commented Feb 28, 2019

@dmitshur yet another possible solution might be merging
https://golang.org/cl/160837, but (a) I haven't looked closely enough to know if that is true, and (b) that might be more work than other possible solutions already on the table.

edit: sorry, scratch that, I think that is actually not the right CL... but I had thought there was another pending CL that would further cut down on more of the golang.org/x/... dependencies, but not sure.

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@thepudds I had the same idea. I plan to prioritize getting that CL submitted tomorrow, as I think it will be very helpful for the x/tools module and everything that requires it. /cc @matloob

Edit: I think it is the right CL, is it not? It removes appengine and x/sync from required modules, leaving only x/net.

@thepudds

This comment has been minimized.

Copy link

commented Feb 28, 2019

@dmitshur if you tell me it is the right CL, I would believe you. ;-)

It does seem to be the one: https://go-review.googlesource.com/c/tools/+/160837/2/go.mod

@thepudds

This comment has been minimized.

Copy link

commented Feb 28, 2019

@bcmills in golang/lint#436 (comment) you said in part:

go get -u should only upgrade modules in the transitive import graph, so even if the problematic github.com/golang/lint module is in the module graph, as long as it is not in the import graph it should not cause any trouble. (If it does, that's a bug.)

I am not sure how to think about the transitive import graph when the main module is empty (go.mod file, but no *.go files), such as in golang/lint#436 (comment), which then fails go get -u golang.org/x/lint. Probably the transative import graph at that point effectively has pulled in golang.org/x/lint and friends?

Separately, if you start with an empty module and do go get golang.org/x/lint (no -u), things like the module build list and go list -deps seem pretty small:

$ go list -m all
tempmod
github.com/golang/protobuf v1.2.0
golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961
golang.org/x/net v0.0.0-20190213061140-3a22650c66bd
golang.org/x/sync v0.0.0-20181108010431-42b317875d0f
golang.org/x/text v0.3.0
golang.org/x/tools v0.0.0-20190226205152-f727befe758c
google.golang.org/appengine v1.4.0

$ go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' golang.org/x/lint
golang.org/x/tools/go/ast/astutil
golang.org/x/tools/go/internal/gcimporter
golang.org/x/tools/go/gcexportdata
golang.org/x/lint

But admittedly those two sample commands above alone do not provide a complete view... and also that is not in the context of appengine build tags, so maybe that explains it or maybe test dependencies explain it, but I will confess I am not sure if there is another bug here or not.

go get -u should only upgrade modules in the transitive import graph

I don't know if the fact that so much gets upgraded if you do go get -u after doing go get golang.org/x/lint on an otherwise empty module is #29702, or a variation of that, or a separate bug, or "expected" because there are actually a bunch of modules in the transitive import graph...

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

It sounds like the cmd/go part of this issue is mostly covered by #28489. The only other thing I would expect cmd/go to do is perhaps try older tags to see if the go.mod path is different in them, but in this case github.com/golang/lint is untagged.

Closing as duplicate of #28489, and let's keep the lint-specific discussion on golang/lint#436.

@bcmills bcmills closed this Feb 28, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I am not sure how to think about the transitive import graph when the main module is empty (go.mod file, but no *.go files), such as in golang/lint#436 (comment), which then fails go get -u golang.org/x/lint.

go get -u golang.org/x/lint says “add the module containing package golang.org/x/lint to the main module and upgrade the modules that provide the transitive imports of that package”. That's why it still affects the main module.

@thepudds

This comment has been minimized.

Copy link

commented Feb 28, 2019

Hi @bcmills

go get -u should only upgrade modules in the transitive import graph, so even if the problematic github.com/golang/lint module is in the module graph, as long as it is not in the import graph it should not cause any trouble. (If it does, that's a bug.)

What is an easy-ish way to see the transitive import graph, and does that "transitive import graph" as used in your quote at the top of this comment include test-only dependencies? (For example, something like go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' golang.org/x/lint I think does not include test-only dependencies).

It seems more clear how the the module graph gets so large for the current golang.org/x/lint... but when you say " go get -u should only upgrade modules in the transitive import graph", is it clear what in golang.org/x/lint is causing the transitive import graph to be so large? (Maybe I missed something obvious; less fun to look at these types of questions when some of the standard commands error out).

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

What is an easy-ish way to see the transitive import graph […]?

go list -deps -test $pkg, with whatever -f flags you like.

does that "transitive import graph" as used in your quote at the top of this comment include test-only dependencies?

Yes, I believe so.

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

is it clear what in golang.org/x/lint is causing the transitive import graph to be so large?

Yes: golang/protobuf#806.

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Well, that was one source. There are apparently others.

@thepudds

This comment has been minimized.

Copy link

commented Feb 28, 2019

FWIW, go list -deps -test also produces a small list:

$ go list -deps -test -f '{{if not .Standard}}{{.ImportPath}}{{end}}' golang.org/x/lint/...
golang.org/x/tools/go/ast/astutil
golang.org/x/tools/go/internal/gcimporter
golang.org/x/tools/go/gcexportdata
golang.org/x/lint
golang.org/x/lint/golint
golang.org/x/lint [golang.org/x/lint.test]
golang.org/x/lint.test

Separately, Bryan mentioned #26902 is potentially related.

@thepudds thepudds added the modules label Mar 1, 2019

@thepudds

This comment has been minimized.

Copy link

commented Mar 1, 2019

@dmitshur FYI, re-running the replace-based workaround that worked for me two days ago in golang/lint#436 (comment) now fails with:

go: github.com/go-resty/resty@v1.9.0: parsing go.mod: unexpected module path "gopkg.in/resty.v1"

(Perhaps better for this comment to be on golang/lint#436, but adding it here because golang/lint#436 is now a fairly high volume issue).

Commands
mkdir /tmp/scratchpad/lint-test-workaround-rerun-1
cd /tmp/scratchpad/lint-test-workaround-rerun-1
export GOPATH=/tmp/scratchpad/gopath-for-lint-workaround-rerun-1
go mod init tempmod
echo 'replace google.golang.org/grpc => google.golang.org/grpc v1.19.0' >> go.mod
echo 'replace github.com/openzipkin/zipkin-go => github.com/openzipkin/zipkin-go b722d64 // current master' >> go.mod
go get -u golang.org/x/lint
@thepudds

This comment has been minimized.

Copy link

commented Mar 1, 2019

Haven't looked at this carefully, but the resty mismatch might now be coming in via grpc-gateway:

github.com/grpc-ecosystem/grpc-gateway/@v/v1.8.0.mod:   github.com/go-resty/resty v1.9.0
github.com/grpc-ecosystem/grpc-gateway/@v/v1.8.0.mod:replace github.com/go-resty/resty => gopkg.in/resty.v1 v1.9.0

It looks like https://github.com/grpc-ecosystem/grpc-gateway released v1.8.0 about 5 hours ago. It looks like that might be the first release with go.mod. Note that it uses github.com/go-resty/resty in the require section of its go.mod, but has a replace github.com/go-resty/resty => gopkg.in/resty.v1 v1.9.0.

@thepudds

This comment has been minimized.

Copy link

commented Mar 13, 2019

Question: could this be viewed as a non-fatal error? In the original example reported in golang/lint#436, I think there were other require directives for google.golang.org/grpc for versions higher than the problematic google.golang.org/grpc@v1.16.0. In other words, the problematic
google.golang.org/grpc@v1.16.0 I think was not going to end up in the build list if the build list had been allowed to be constructed and the build had been allowed to happen instead of a fatal error.

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