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: when initializing a go.mod from other formats, verify dependency module paths before saving? #30161

Closed
daixiang0 opened this issue Feb 11, 2019 · 13 comments
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@daixiang0
Copy link

daixiang0 commented Feb 11, 2019

The repo is loki, after use dep ensure at master branch:

[root@dx-app2 loki]# GO111MODULE=on go mod vendor
go: creating new go.mod: module github.com/grafana/loki
go: copying requirements from Gopkg.lock
go: converting Gopkg.lock: stat github.com/weaveworks/common@6ebd07f752e2a8d51d3ac919014f972350d25b39: unknown revision 6ebd07f752e2a8d51d3ac919014f972350d25b39
go: github.com/cespare/xxhash@v0.0.0-20190104012619-3b82fb7d1867: go.mod has post-v0 module path "github.com/cespare/xxhash/v2" at revision 3b82fb7d1867
go: error loading module requirements
[root@dx-app2 loki]# go version
go version go1.11 linux/amd64
[root@dx-app2 loki]# gometalinter --version
gometalinter version 3.0.0 built from df395bfa67c5d0630d936c0044cf07ff05086655 on 2019-01-29T22:44:16Z
[root@dx-app2 loki]# go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/deploy/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/golang"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build947627841=/tmp/go-build -gno-record-gcc-switches"

I do not know how to fix this.

@katiehockman katiehockman changed the title go mod vendor error cmd/go: go mod vendor error Feb 11, 2019
@katiehockman
Copy link
Contributor

Just to be sure that this isn't an issue that has already been fixed, try updating to the newest version of Go and trying again.

/cc @bcmills

@katiehockman katiehockman added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. modules labels Feb 11, 2019
@jayconrod
Copy link
Contributor

I was able to reproduce this with a newer version of Go.

This project doesn't have a go.mod file, so when it's built with GO111MODULE=on, we generate one from Gopkg.lock. This isn't specific to go mod vendor; any build will do this.

Gopkg.lock has a dependency on github.com/cespare/xxhash at 3b82fb7d186719faeedd0c2864f868c74fbf79a1. We import this as the pseudoversion v0.0.0-20190104012619-3b82fb7d1867. However, xxhash is tagged as v2.0.0 at that commit, and it has a go.mod file, so it should be required as github.com/cespare/xxhash/v2 v2.0.0. After manually changing the requirement to this, go mod vendor completes successfully.

@bcmills I think this could be fixed in internal/modconv/convert.go, but I want to make sure I'm not going off into the weeds before starting. WDYT?

Related: this Gopkg.lock file has some case-sensitivity problems. For example, we end up with github.com/sirupsen/logrus v1.3.0 // indirect instead of github.com/Sirupsen/logrus.

@jayconrod jayconrod changed the title cmd/go: go mod vendor error cmd/go: incorrect module requirements imported from Gopkg.lock Feb 11, 2019
@bcmills
Copy link
Contributor

bcmills commented Feb 12, 2019

Assuming that you don't actually need the v2 features, you should be able to fix your build by running go get github.com/cespare/xxhash@v1.1.0.

@bcmills
Copy link
Contributor

bcmills commented Feb 12, 2019

Since github.com/cespare/xxhash at commit 3b82fb7d1867 has a go.mod file specifying a /v2 path, go mod vendor really is correct to reject it: the go.mod file defines the expected import path, which doesn't match the import path under which your build is attempting to use it.

Probably the best we can do in terms of a general fix would be to download the go.mod file for each module in the converted go.mod file after conversion, check its module declaration, and remove any entries that don't match. We would presumably re-resolve those modules to latest whenever we need to access a package from them, and hopefully that would pick up something compatible.

@bcmills bcmills changed the title cmd/go: incorrect module requirements imported from Gopkg.lock cmd/go: when initializing a go.mod from other formats, verify dependency module paths before saving? Feb 12, 2019
@bcmills bcmills added this to the Unplanned milestone Feb 12, 2019
@jayconrod
Copy link
Contributor

Following that strategy, we'd lose some fidelity when importing from dep and other tools, but it seems preferable to writing out a broken go.mod file.

Russ pointed out that go get in GOPATH-mode will try fetching from the go1 tag before master. Projects should add a go1 tag when adding a go.mod with v2 or later. go-release should probably give advice on this.

I'm not sure if vendoring tools handle this tag. It's documented in go help get, but I hadn't heard of it before today. This also still relies on downstream projects doing the right thing when they aren't warned about doing the wrong thing.

@daixiang0
Copy link
Author

Is there any doc that describe difference between v1 and v2, alse the different way go mod would use?
I am confused about it, control vendor is very hard to me, mod file is a good choice, but now, migrate from vendor cost me too much time.

@jayconrod
Copy link
Contributor

@daixiang0 go help modules gives an introduction and reference on modules. You can also find this on the web at https://golang.org/cmd/go/#hdr-Modules__module_versions__and_more. The Modules page on the wiki has more detail and a lot of FAQs. Specficially Semantic Import Versioning.

To answer this specifically though, when a module is versioned at v2.0.0 or greater, its path must have the suffix v2. That means you need to import packages github.com/cespare/xxhash/v2, not github.com/cespare/xxhash.

In this project, there isn't a direct dependency on github.com/cespare/xxhash/v2; it comes through another project (I think it was one of the prometheus projects). So loki isn't actually doing anything wrong, but something upstream is importing the project with the wrong path, and neither dep nor go get in GOPATH-mode is reporting an error.

Concretely, you may be able to get this to work by depending on github.com/cespare/xxhash at an older version. You can do that simply by deleting the requirement on the module from your go.mod file. It will be added back automatically on your next build (go mod tidy will also add it). This assumes you don't depend on a feature or API in v2.0.0 though.

@thepudds
Copy link
Contributor

thepudds commented Feb 13, 2019

@daixiang0 I just wanted to briefly expand on some of the points from @jayconrod, as well as ask a few questions.

The first is I was wondering if your intent is to convert this loki repository over to using a go.mod file?

If so, it would be more conventional I think to first run go mod init, and to do that prior to running go mod vendor.

Separately, as @jayconrod said, the root of the problem seems to be github.com/cespare/xxhash. This quick test seemed to work for me at least (where in my case, I first ran go mod init to have a clean conversion step):

git clone https://github.com/grafana/loki /tmp/scratchpad/loki
cd /tmp/scratchpad/loki
go mod init github.com/grafana/loki 
sed -i '/github.com\/cespare\/xxhash/d' go.mod
go list -m all
go mod vendor

I also then ran make, which seemed to complete successfully as well, but I am not familiar with the details of the loki project, so I don't know if make completing is meaningful or not.

In terms of where the v2.0.0 requirement for github.com/cespare/xxhash came from, there might be some nuance there. According to go mod graph | grep github.com/cespare/xxhash (run from within loki after the steps above), it seems loki's overall dependency on github.com/cespare/xxhash is coming via two of loki's dependencies:

  • github.com/prometheus/prometheus 718344434c52
  • github.com/prometheus/tsdb 6d489a1004dc

Both of those dependencies themselves seem to already have a go.mod, which both seem to require:

  • github.com/cespare/xxhash v1.1.0

However, if you run dep ensure update (or some other dep ensure variations) from inside of loki, at that point I think dep has no knowledge of the go.mod files that live in github.com/prometheus/prometheus and github.com/prometheus/tsdb. I suspect at that point, dep needs to pick a version of xxhash for the loki Gopkg.lock file, and under those circumstances I think dep might pick the latest available semver version of xxhash, which happens to be v2.0.0. In other words, I think when dep is run inside of loki, dep made a guess that github.com/prometheus/prometheus and/or github.com/prometheus/tsdb want the v2.0.0 version of xxhash, even though their go.mod files actually ask for the v1.1.0 version of xxhash.

Sorry if any of that is confusing or if I missed something here, but if correct, I think it would both explain where the v2.0.0 for xxhash came from, as well as underscore the validity of the original advice from @jayconrod to remove xxhash from the the loki go.mod file (because in fact, no one actually seems to be declaring their need for that v2.0.0 version).

Finally, I should add that it is just a guess as to what dep operations might have put the loki repo into this state (given there is more than one path into this state, including a manual edit, etc.), but it does seem be the case that no one actually requires the v2.0.0 version of xxhash and hence it seems to be safe to remove it from the loki go.mod file (again assuming I haven't just confused myself on the scenario here or otherwise missed something).

@daixiang0
Copy link
Author

daixiang0 commented Feb 14, 2019

@bcmills @jayconrod @thepudds Thanks very much. Now i learn something about how go mod work.

I follow tip and find there is no xxhash in go.mod:

$ grep xxhash go.mod
$ 

Then i call go mod tidy, still not find, is it normal?

make in loki does not check go mod since now it still use vendor.

The guess about why go mod vendor fail after dep ensure may be close to fact, i have some ideas about this:

  • if there is no go.mod, refuse to call go mod vendor and tip user to call go mod init could be better?
  • if init failed, do not generate go.mod since a broken file may be a trap right?
  • do not recommend run dep before go mod xxx

I am happy to contribute to docs about above ideas if possible. Thanks again for your patient explanation。

@jayconrod
Copy link
Contributor

Then i call go mod tidy, still not find, is it normal?

go.mod will list requirements that are either direct dependencies by your module, or indirectly required by another module without a specific version (e.g., because the other module doesn't have a go.mod file). An indirect requirement will not be listed in go.mod if it is required at a specific version in another module's go.mod. This seems to be the case here.

if there is no go.mod, refuse to call go mod vendor and tip user to call go mod init could be better?

Whether you call go mod vendor depends on whether you need a vendor directory. It's supposed to work either way.

if init failed, do not generate go.mod since a broken file may be a trap right?

I don't think we should bail out entirely if there's an error importing from dep, especially if it's an error in a dependency the users can't directly address. In this case, there's a workaround (deleting problematic requirements and re-running go mod tidy or other module commands). But I think cmd/go should confirm imported requirements work before saving them to go.mod.

do not recommend run dep before go mod xxx

It can be useful to import versions from dep if specific versions are needed. Not always necessary, but frequently useful.

@bcmills
Copy link
Contributor

bcmills commented Feb 14, 2019

Note that as of #29433, you will not be able to run go mod vendor until after an explicit go mod init anyway.

@daixiang0
Copy link
Author

It can be useful to import versions from dep if specific versions are needed. Not always necessary, but frequently useful.
@jayconrod Where should i add comment?

@jayconrod
Copy link
Contributor

@daixiang0 I'm not sure more documentation is needed for this, but the Modules wiki is probably the right place if any details are missing.

Specifically, I have a problem with a complex dependency that has not opted in to modules. Can I use information from its current dependency manager? and FAQs — Possible Problems.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants