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

#236 makes this library unusable for go get projects #258

Closed
prestonvanloon opened this issue Feb 12, 2019 · 16 comments
Closed

#236 makes this library unusable for go get projects #258

prestonvanloon opened this issue Feb 12, 2019 · 16 comments

Comments

@prestonvanloon
Copy link

prestonvanloon commented Feb 12, 2019

#236

Failing build for github.com/prysmaticlabs/prysm: https://api.travis-ci.com/v3/job/177080918/log.txt

How are users supposed to use this library with go get?

Looks like this is the one of the offending code:

go-libp2p-kad-dht/dht.go

Lines 11 to 13 in f05ddbf

opts "github.com/libp2p/go-libp2p-kad-dht/v5/opts"
pb "github.com/libp2p/go-libp2p-kad-dht/v5/pb"
providers "github.com/libp2p/go-libp2p-kad-dht/v5/providers"

Where github.com/libp2p/go-libp2p-kad-dht/v5/providers does not exist. Looking in this repository and godocs, this directory doesn't exist.

@raulk
Copy link
Member

raulk commented Feb 12, 2019

@prestonvanloon sorry for the breakage. I am looking into it right now.

@raulk
Copy link
Member

raulk commented Feb 12, 2019

@prestonvanloon I've reverted the change, please let us know if it still breaks. For us to diagnose what happened here: what version of go are you using? And are you building with gomod support?

@raulk
Copy link
Member

raulk commented Feb 12, 2019

Nevermind, I saw in your CI log that you're building with Go 1.8.x (!). I believe that's where the issue is: the change optimistically assumed that downstreams were using modern versions. Let me get to the root cause of this to find a solution.

@prestonvanloon
Copy link
Author

We are using go 1.11 for linting.

See this travis file:
https://github.com/prysmaticlabs/prysm/blob/cfaa938ca8b17d2ac993eba95a69da8493f33cb3/.travis.yml#L1-L14

This runner does go get -t ./... and then runs gometalinter. We don't use go.mod or gx or anything for linting.

We use bazel for the dependency management of our binaries, but all linting runs at HEAD and we try to update our dependencies as soon as new pushes happen to master (assuming our tests still pass and the updates are trivial updates).

@raulk
Copy link
Member

raulk commented Feb 12, 2019

You're right. I got thrown off by the docker log at the beginning of the CI log. It is the docker binary that's built with go1.8.x.

Unfortunately the log has changed: https://api.travis-ci.com/v3/job/177080918/log.txt. Can you give me access to your Travis instance, please? Or send me a gist with the log dump where the build broke? I checked https://travis-ci.com/prysmaticlabs/prysm but don't find the run there.

@raulk
Copy link
Member

raulk commented Feb 12, 2019

@prestonvanloon For linting? Was the linting the thing that failed (I have no access to the log anymore).

Note that using versions in import paths that correspond to a v2+ branches or a tag is correct, and is used in conjunction with gomod. Go versions 1.9.7+ and 1.10.3+ also support it.

It seems that gometalinter doesn't support gomod v2+ import paths natively yet and therefore tried to map the import path to the filesystem wrongly.

The recommendation in this case is to use go mod vendor and have the linter run on the resulting tree with GO111MODULE=off. This is how another project did it: filebrowser/filebrowser#586 (comment).

Anyway, I'm gonna circle back with the team to see if there's a way we can avoid the version number in the import path.

Regardless, I'd appreciate the log from the failed build.

/cc @lanzafame @anacrolix

@raulk raulk mentioned this issue Feb 12, 2019
@prestonvanloon
Copy link
Author

prestonvanloon commented Feb 12, 2019

@raulk Here's the travis: https://travis-ci.com/prysmaticlabs/prysm. It should be public.
This was the build that I linked: https://travis-ci.com/prysmaticlabs/prysm/builds/100592716

Edit: looks like that branch was deleted and/or the build restarted so the log is gone.
Edit2: Log in comment below (keep scrolling)

Yes. We run gometalinter which doesn't work with our build tooling... yet.

@anacrolix
Copy link
Contributor

The Go tooling does handle mixed use of major versioning and "old GOPATH" versioning which is nice (after a certain version I expect, probably 1.9 or 1.10, as they backported compatibility for go mod). Some tooling does not work (my current IDE included, and apparently gometalinter).

It's a nice to have if go get works for libp2p, and it will for Go >= 1.9, most of the time. I propose to continue supporting go mod in "incompatible" mode (no major versions in the import path) until the tooling catches up, and these kinks are worked out.

@prestonvanloon any reason you're not pinning your dependencies in some way?

@prestonvanloon
Copy link
Author

prestonvanloon commented Feb 12, 2019

@anacrolix because we don't care about the dependencies code for purposes of linting our code.
If we use go.mod, then we have to maintain two sets of pinned dependencies.

To be clear, the problem wasn't with gometalinter. We were just trying to go get -t ./... to fetch all of our dependencies from HEAD.
If you tried to go get github.com/libp2p/go-libp2p-kad-dht with the changes in #236 then you would have had the same issue.
Edit: I am very wrong. (keep scrolling)

Are you planning on no longer supporting install via go get github.com/libp2p/go-libp2p-kad-dht? This is how it is indicated in your README but would have been broken by #236 go mod support.

@anacrolix
Copy link
Contributor

If you tried to go get github.com/libp2p/go-libp2p-kad-dht with the changes in #236 then you would have had the same issue.

I don't believe this is correct if you use Go versions 1.9.7+, 1.10.3+, and 1.11+.

Are you planning on no longer supporting install via go get github.com/libp2p/go-libp2p-kad-dht? This is how it is indicated in your README but would have been broken by #236 go mod support.

I don't believe this is correct, with the same caveats as above. However I don't want to put a mention of go modules support until it's certain.

@Stebalien
Copy link
Member

Stebalien commented Feb 14, 2019

Are you planning on no longer supporting install via ...

We plan on supporting go get until go mod is stable.


@prestonvanloon you are correct. As far as I can tell, go get isn't working. I tested this with a fork but didn't update the path to the repo in the go.mod file.

@anacrolix you are also correct, it should work: https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher. For example, this does work: go get github.com/Stebalien/foobar.

@prestonvanloon
Copy link
Author

prestonvanloon commented Feb 14, 2019

I don't believe this is correct if you use Go versions 1.9.7+, 1.10.3+, and 1.11+.

I'm telling you that we were using go 1.11.x and we could not get this repository via go get with the changes from #236.

If we enabled the experimental env feature flag GO111MODULE=on then it might have worked, but this isn't going to be the default until 1.12.

Edit: I am very wrong.

@raulk
Copy link
Member

raulk commented Feb 14, 2019

@prestonvanloon I thought it was the linter that failed, not go get?

@prestonvanloon
Copy link
Author

@raulk we could not go get the project. We need to do this to run the linter.

@prestonvanloon
Copy link
Author

Ok wait, I am terribly mistaken. I was the linter. I managed to recover a log.

log.txt

In any case, this type of virtual renaming breaks our Bazel build tooling as well and seems to have poor support across IDEs/tooling.

Sorry for the confusion. It only breaks our linter, build tool, and probably more tooling, but we could fetch it via go get.

@raulk
Copy link
Member

raulk commented Feb 19, 2019

We reverted the change, and have tentatively settled on an approach that allows us to use pre-v2 versions to remove the version number from the path: namespacing gx versions under the gx- Git tag namespace, and starting a new version lineage for gomod. This will give more time for tooling to upgrade and for gomod to graduate.

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

No branches or pull requests

4 participants