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: "get -u" stumbles over github.com/golang/lint #30831

Open
broady opened this Issue Mar 14, 2019 · 15 comments

Comments

Projects
None yet
6 participants
@broady
Copy link
Member

broady commented Mar 14, 2019

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

$ go version
go version go1.12 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/cbro/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/cbro/"
GOPROXY=""
GORACE=""
GOROOT="/Users/cbro/sdk/go1.12"
GOTMPDIR=""
GOTOOLDIR="/Users/cbro/sdk/go1.12/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/var/folders/h0/kdykkxh94y1fdv1_yz98yr_w003lw8/T/tmp.ULmCh1W7/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/h0/kdykkxh94y1fdv1_yz98yr_w003lw8/T/go-build671297704=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

While investigating googleapis/google-cloud-go#1359, golang/lint#436, and opening census-instrumentation/opencensus-go#1064, I've found it impossible to figure out how to get almost any of these packages to build and successfully run go get -u.

It seems to be a never-ending loop of things depending on slightly older things, which ultimately end up depending on the invalid github.com/golang/lint.

For a lot, but not all (see below) of these packages, I was able to track the chain from github.com/golang/lint up to its depender using go mod graph.

Of course, because of MVS, the old version of grpc (and therefore github.com/golang/lint) is never used in the actual build, but for some reason still blocks a successful run of go get -u.

$ cd $(mktemp -d)
T/tmp.ULmCh1W7 $ ls

T/tmp.ULmCh1W7 $ go mod init m
go: creating new go.mod: module m

T/tmp.ULmCh1W7 $ go get google.golang.org/grpc
go: downloading golang.org/x/net v0.0.0-20180826012351-8a410e7b638d
go: downloading google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8
go: extracting golang.org/x/net v0.0.0-20180826012351-8a410e7b638d
go: downloading golang.org/x/text v0.3.0
go: extracting google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8
go: extracting golang.org/x/text v0.3.0

T/tmp.ULmCh1W7 $ cat go.mod
module m

go 1.12

require google.golang.org/grpc v1.19.0 // indirect

T/tmp.ULmCh1W7 $ go get -u
go: finding github.com/golang/glog latest
go: finding google.golang.org/genproto latest
go: finding golang.org/x/sync latest
go: finding honnef.co/go/tools latest
go: finding golang.org/x/lint latest
go: finding golang.org/x/oauth2 latest
go: finding golang.org/x/net latest
go: finding golang.org/x/sys latest
go: finding github.com/shurcooL/issuesapp latest
go: finding github.com/google/pprof latest
go: finding golang.org/x/time latest
go: finding golang.org/x/exp latest
go: finding github.com/anmitsu/go-shlex latest
go: finding golang.org/x/tools latest
go: finding github.com/shurcooL/issues latest
go: finding github.com/tarm/serial latest
go: finding golang.org/x/build latest
go: finding github.com/neelance/astrewrite latest
go: finding dmitri.shuralyov.com/state latest
go: finding github.com/shurcooL/httpgzip latest
go: finding github.com/shurcooL/gopherjslib latest
go: finding github.com/shurcooL/htmlg latest
go: finding github.com/shurcooL/component latest
go: finding github.com/google/btree latest
go: finding github.com/shurcooL/reactions latest
go: finding github.com/shurcooL/github_flavored_markdown latest
go: finding github.com/jellevandenhooff/dkim latest
go: finding grpc.go4.org latest
go: finding github.com/shurcooL/highlight_go latest
go: finding dmitri.shuralyov.com/html/belt latest
go: finding go4.org latest
go: finding github.com/shurcooL/notifications latest
go: finding github.com/shurcooL/users latest
go: finding github.com/coreos/go-systemd latest
go: finding golang.org/x/mobile latest
go: finding github.com/BurntSushi/xgb latest
go: finding github.com/shurcooL/webdavfs latest
go: finding github.com/neelance/sourcemap latest
go: finding github.com/shurcooL/go latest
go: finding golang.org/x/crypto latest
go: finding github.com/prometheus/client_model latest
go: finding github.com/flynn/go-shlex latest
go: finding golang.org/x/perf latest
go: finding github.com/prometheus/procfs latest
go: finding dmitri.shuralyov.com/service/change latest
go: finding github.com/jstemmer/go-junit-report latest
go: finding github.com/shurcooL/httpfs latest
go: finding github.com/shurcooL/httperror latest
go: finding github.com/sourcegraph/syntaxhighlight latest
go: finding github.com/beorn7/perks latest
go: finding gopkg.in/check.v1 latest
go: finding github.com/gopherjs/gopherjs latest
go: finding dmitri.shuralyov.com/app/changes latest
go: finding github.com/shurcooL/highlight_diff latest
go: finding github.com/shurcooL/home latest
go: finding github.com/shurcooL/octicon latest
go: finding github.com/sourcegraph/annotate latest
go: finding github.com/gonum/blas latest
go: finding github.com/gonum/internal latest
go: finding github.com/gonum/lapack latest
go: finding github.com/GoogleCloudPlatform/cloudsql-proxy latest
go: finding github.com/gregjones/httpcache latest
go: finding golang.org/x/image latest
go: finding github.com/shurcooL/events latest
go: finding github.com/bradfitz/go-smtpd latest
go: finding github.com/shurcooL/go-goon latest
go: finding github.com/shurcooL/gofontwoff latest
go: finding github.com/golang/lint latest
go: finding github.com/alecthomas/units latest
go: finding github.com/gonum/matrix latest
go: github.com/golang/lint@v0.0.0-20190313153728-d0100b6bd8b3: parsing go.mod: unexpected module path "golang.org/x/lint"
go: finding github.com/eapache/go-xerial-snappy latest
go: finding github.com/mwitkow/go-conntrack latest
go: finding github.com/rcrowley/go-metrics latest
go: finding github.com/aclements/go-moremath latest
go: finding github.com/gonum/floats latest
go get: error loading module requirements
Exit code 1

T/tmp.ULmCh1W7 $ go mod graph | grep golang/lint
Exit code 1

T/tmp.ULmCh1W7 $ go mod graph
m google.golang.org/grpc@v1.19.0
google.golang.org/grpc@v1.19.0 cloud.google.com/go@v0.26.0
google.golang.org/grpc@v1.19.0 github.com/BurntSushi/toml@v0.3.1
google.golang.org/grpc@v1.19.0 github.com/client9/misspell@v0.3.4
google.golang.org/grpc@v1.19.0 github.com/golang/glog@v0.0.0-20160126235308-23def4e6c14b
google.golang.org/grpc@v1.19.0 github.com/golang/mock@v1.1.1
google.golang.org/grpc@v1.19.0 github.com/golang/protobuf@v1.2.0
google.golang.org/grpc@v1.19.0 golang.org/x/lint@v0.0.0-20181026193005-c67002cb31c3
google.golang.org/grpc@v1.19.0 golang.org/x/net@v0.0.0-20180826012351-8a410e7b638d
google.golang.org/grpc@v1.19.0 golang.org/x/oauth2@v0.0.0-20180821212333-d2e6202438be
google.golang.org/grpc@v1.19.0 golang.org/x/sync@v0.0.0-20180314180146-1d60e4601c6f
google.golang.org/grpc@v1.19.0 golang.org/x/sys@v0.0.0-20180830151530-49385e6e1522
google.golang.org/grpc@v1.19.0 golang.org/x/text@v0.3.0
google.golang.org/grpc@v1.19.0 golang.org/x/tools@v0.0.0-20190114222345-bf090417da8b
google.golang.org/grpc@v1.19.0 google.golang.org/appengine@v1.1.0
google.golang.org/grpc@v1.19.0 google.golang.org/genproto@v0.0.0-20180817151627-c66870c02cf8
google.golang.org/grpc@v1.19.0 honnef.co/go/tools@v0.0.0-20190102054323-c2f93a96b099

What did you expect to see?

go get -u succeeds, no mention of github.com/golang/lint because it shouldn't be included due to MVS rules.

What did you see instead?

@broady

This comment has been minimized.

Copy link
Member Author

broady commented Mar 14, 2019

My theory (sorry, it's late, so not sure if it's obviously right or wrong... also, this bug report is poorly written but I'm pretty frustrated and confused by all of this)

Because we are allowed circular dependencies on modules, we have two libraries:

A@0.1 requires github.com/golang/lint
A@0.1 requires B@v0.0.0-...

B@0.1 requires A@0.1

A fixes its import to golang.org/x/lint, tags 0.2

B@0.2 updates its A dependency, and now requires A@0.2.

But A@0.2 still requires B@0.1 which requires A@0.1 which requires B@v0.0.0-... which requires github.com/golang/lint.

Of course, due to MVS, we only choose one version of the dependency at build time. However, go get -u doesn't seem to use MVS in the same way. It appears to touch every single version in the never-ending chain of dependent versions, even if they aren't used in the actual build.

So the only to break this cycle is to simultaneously tag new versions of A and B, and have them require each other. (incompatible with CI)

In reality, we have more than just two things depending on each other, there's far more. golang.org/x/oauth2, google.golang.org/api/grpc, cloud.google.com/go, google.golang.org/api, go.opencensus.io basically all have dependencies on each other.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Mar 14, 2019

/cc @bcmills

We've discussed this general issue today (or yesterday) and Bryan said he might be aware of a fix that can be applied to cmd/go to make the situation better. I'll let him provide more information on that.

@broady

This comment has been minimized.

Copy link
Member Author

broady commented Mar 14, 2019

Something along the lines of this, maybe? (see patch below)
Tests don't pass, though, so this is probably not sane.

The idea is to not try to upgrade something when we already use a newer version. Theory: upgrading from 0.2 to 0.3 is less work than 0.1 to 0.3, and there's no need to check the same package if you already checked a newer version.

edit: ah, yes, theory and tests fail because vN+1 may have downgraded one of its deps.

@@ -85,6 +89,17 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo
        )
        work.Do(10, func(item interface{}) {
                m := item.(module.Version)
+
+               var skip bool
+               mu.Lock()
+               if v, ok := min[m.Path]; ok && v != "" && reqs.Max(v, m.Version) == v {
+                       skip = true
+               }
+               mu.Unlock()
+               if skip {
+                       return
+               }
+
                required, err := reqs.Required(m)
 
                mu.Lock()

Simplified problem...

M has a requirement on A@0.1 and B@0.1
B@0.1 has a requirement on C@0.1
B@0.2 dropped the requirement on C entirely
A@0.1 has a requirement on B@0.2

"go get -u" still tries to upgrade C, because of M's dependency on B@0.1, even though M won't use B@0.1 or C@0.1 in the build at all, only A@0.1 and B@0.2.


Another ecosystem problem is "indirect" entries in go.mod. As evidenced by zipkin including an indirect entry in github.com/golang/lint:

A@0.1 requires C@0.0
B@0.1 requires A@0.1 and adds an indirect entry to C@0.0
A@0.2 drops the C requirement.

M requires A@0.2 and B@0.1, indirectly depending on C@0.0 even though it isn't used to build, because MVS upgrades B's A@0.1 requirement to A@0.2.

In this case, A = grpc or genproto, B = zipkin, C = github.com/golang/lint

@broady broady changed the title cmd/go: unexpected module path but no entry in graph cmd/go: "get -u" too many required modules are attempted for upgrade Mar 14, 2019

@bcmills bcmills self-assigned this Mar 14, 2019

@bcmills bcmills added this to the Go1.13 milestone Mar 14, 2019

@bcmills bcmills added the NeedsFix label Mar 14, 2019

@thepudds

This comment has been minimized.

Copy link

thepudds commented Mar 14, 2019

FWIW, I also suspect (a) something is wrong here, or (b) that the behavior here can be improved.

Potentially related is #30455 (comment), which asks if golang.org/x/lint vs. github.com/golang/lint could be viewed as a non-fatal error:

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.

Also potentially related:

  • the comment starting at #30455 (comment) and the next 10 or so comments.
  • #26902 "cmd/go: clarify which dependencies are updated by 'get -u' without '-m'"
  • #29702 "cmd/go: 'mod tidy' downgrades an unused indirect dependency upgraded by 'get -u'"
  • #29410 "cmd/go: building requires otherwise-unused .mod files to be present"

@rsc rsc changed the title cmd/go: "get -u" too many required modules are attempted for upgrade cmd/go: "get -u" stumbles over github.com/golang/lint Mar 14, 2019

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 14, 2019

Leaving the "too much upgrading" problem for #26902.
Retitled this to be about lint since that is the specific complaint in "what did you expect to see?".

It seems like we need a clear signal from the upgraded module that "I used to be this old name". And then if you are resolving the old name and land in the new module claiming "I used to be this old name", the build would automatically interpret the old name as the new name. Maybe that's a per-module signal; maybe it's a per-package signal. It needs to be an explicit signal, though. It's not enough to just "find code at X that seems to be module Y so pretend X means Y" because that breaks people who github fork X into their own account as Y. In that case we don't want go get of the fork to silently redirect back to the original.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Mar 14, 2019

It seems like we need a clear signal from the upgraded module that "I used to be this old name".

This requires the upgraded module to know its old name. In this specific case, it might be possible, because the canonical import path of lint used to be github.com/golang/lint a long time ago (see here.

However, it didn't start enforcing import paths until much later (via import comment for GOPATH mode, and via go.mod file for module mode), so some projects may have started importing it via any other alias, not just github.com/golang/lint.

Should it be responsibility of an upgraded module to know what were all the non-canonical import paths that some projects in the wild may have imported it as?

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Mar 14, 2019

Should it be responsibility of an upgraded module to know what were all the non-canonical import paths that some projects in the wild may have imported it as?

In general that set is well-defined: we must have arrived at the repository somehow (via redirects), so the only possible names for it are things that redirect to it. I would guess that it's pretty rare for folks to set up redirectors for arbitrary repositories just to use them via different import paths.

@thepudds

This comment has been minimized.

Copy link

thepudds commented Mar 14, 2019

One short description of the build list (from the cmd/go documentation) is:

The build list initially contains only the main module. Then the go command adds to the list the exact module versions required by modules already on the list, recursively, until there is nothing left to add to the list. If multiple versions of a particular module are added to the list, then at the end only the latest version (according to semantic version ordering) is kept for use in the build.

It is unlikely that I will describe this exactly correctly, but from the outside it seems that if the inconsistency between golang.org/x/lint vs. github.com/golang/lint could be viewed as a non-fatal error, it could mean that the particular problematic version of the consumer does not get added to the candidate build list being iteratively built up. In other words, don't add the the version of the consumer with the import of github.com/golang/lint because that version of the consumer has an error, but that error is not fatal to the overall build.

Or maybe that is not the right way to think about it -- maybe instead the "bad" consumer version gets added to the build list but is marked that it is bad, but that is not a fatal problem for the overall build as long as the bad version is not the version that gets selected in the end.

It seems some type of approach along those line would have solved the original x/lint issue reported in golang/lint#436 (given google.golang.org/grpc@v1.16.0 was the "bad" version that imported the wrong flavor of lint, but there were other require statements for v1.17.0, v1.18.0 for grpc in the overall build and hence the v1.16.0 of grpc was not going to be selected anyway). Perhaps that would also help the more recent incarnation being reported here by @broady.

Edit: part of the problem with the current behavior is the degree to which old problems that were fixed a while ago are still a "hangover" for right now. E.g., grpc I think fixed the lint problem in its own go.mod over 4 months ago and released multiple releases since, and it seems a healthy portion of the ecosystem has started using those newer releases of grpc... but one laggard grpc consumer seems to be fatal right now (which seems to be in contrast to how usually in a modules world "laggards" are gently pulled forward by the rest of the ecosystem over time).

@broady

This comment has been minimized.

Copy link
Member Author

broady commented Mar 14, 2019

It seems like we need a clear signal from the upgraded module that "I used to be this old name".

I'm not sure this solves the problem. Suppose there's some requirement on some version of a module that points to the bad pseudoversion of lint that doesn't include the old name. cmd/go still tries to validate that bad version and fails.

I think it needs to be treated as a non-fatal error (non-fatal because it doesn't end up in the final build graph).

exclude works fairly well here as a workaround, could this bad version of grpc/lint get treated in the same way as excluded module versions?

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Mar 14, 2019

Suppose there's some requirement on some version of a module that points to the bad pseudoversion of lint that doesn't include the old name.

The problem is “does include the new name”, not “doesn't include the old name”. The problem today is that folks depend on revisions of github.com/golang/lint from before the go.mod file was added: those are allowed, but cannot be upgraded because the latest revision has an explicitly different path.

If the latest revision instead has an explicit alias declaration, then everything would work fine.

@broady

This comment has been minimized.

Copy link
Member Author

broady commented Mar 14, 2019

The problem is “does include the new name”, not “doesn't include the old name”.

Understood. I was illustrating a situation* where proposed fix of "module can specify acceptable old names and/or aliases" might not work. I suppose that's an edge case, since it would mean an invalid go.mod file in the first place (e.g., if someone had manually changed golang.org/x/lint to github.com/golang/lint).

'* the case where something depends on a version of github.com/golang/lint that doesn't specify its aliases.

@thepudds

This comment has been minimized.

Copy link

thepudds commented Mar 14, 2019

exclude works fairly well here as a workaround, could this bad version of grpc/lint get treated in the same way as excluded module versions?

I don't know if exclude is the right analogy, but following that thought for a moment -- rather than lint (in any form) being treated as if it was excluded... would it be possible to treat grpc@v1.16.0 as more or less excluded (by virtue of the fact that grpc@v1.16.0 references a old spelling of lint that fails to upgrade because of the mismatch with latest)?

The problem today is that folks depend on revisions of github.com/golang/lint from before the go.mod file was added: those are allowed, but cannot be upgraded because the latest revision has an explicitly different path.

Perhaps a completely different approach would be -- rather than saying the old spelling of lint cannot be upgraded, could the old spelling of lint be allowed to upgrade to the version of lint just before the go.mod file was added to lint? In other words, latest version of lint has a disallowed inconsistency with the old import path, but a prior commit does not, so emit a warning but do the upgrade to the valid prior commit of lint (which might be a no-op). If that was the behavior, in the scenarios here I think that would translate to the build completing successfully (and given grpc gets pulled up to more recent versions of grpc due to other require directives, the old spelling of lint would be dropped from the final build list here, I think?).

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Mar 14, 2019

could the old spelling of lint be allowed to upgrade to the version of lint just before the go.mod file was added to lint?

Absent explicit release tags, we would have to scan every commit to the repository to find such a version.

@DukeAnn

This comment has been minimized.

Copy link

DukeAnn commented Mar 15, 2019

What is the solution?

@thepudds

This comment has been minimized.

Copy link

thepudds commented Mar 15, 2019

@DukeAnn

What is the solution?

As far as I am following, I think this issue here is more about a medium or long term fix by changing the go tool, but that is not happening immediately. However, there is a healthy chance there is a more immediate solution to whatever you are seeing.

It probably would make sense for you to review the symptoms at googleapis/google-cloud-go#1359 and golang/lint#436 and see if your symptoms match one of those reports, and either add comments there or perhaps open a new issue.

dmitshur referenced this issue in golang/lint Mar 16, 2019

all: re-introduce go.mod and go.sum files
Now that tools' dependencies have been cleaned up in golang.org/cl/160837, lint
doesn't have any transitive dependencies on modules that import it using the
wrong path. It is now safe to reintroduce a go.mod file to this repo. I've
checked using dmitshur's instructions in
#436 (comment) that lint only
appears with its canonical module path in the build list.

Updates #436

Change-Id: I6343aa103408b20562e17ea019602b159b899fc6
Reviewed-on: https://go-review.googlesource.com/c/lint/+/166278
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.