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

add go mod #236

Merged
merged 7 commits into from
Feb 12, 2019
Merged

add go mod #236

merged 7 commits into from
Feb 12, 2019

Conversation

lanzafame
Copy link
Contributor

Note: branch v4 contains pre-go modules master for any changes that need to be made.

This PR creates valid go modules v5. It also updates the CI to use go modules.

Once merged, master needs to be tagged v5.0.0.

@ghost ghost assigned lanzafame Jan 29, 2019
@ghost ghost added the status/in-progress In progress label Jan 29, 2019
@anacrolix
Copy link
Contributor

@anacrolix
Copy link
Contributor

@lanzafame Can you fix Travis? Are you sure you want to remove the gx stuff completely? Either way I'm happy for you to merge this whenever.

@lanzafame
Copy link
Contributor Author

@anacrolix I just opened ipfs/ci-helpers#11. This is the cause of the travis issues. I haven't determined what the solution is yet though.

@lanzafame lanzafame force-pushed the feat/go-mod branch 3 times, most recently from 4f636cb to 4b2a8d3 Compare January 29, 2019 02:12
@anacrolix
Copy link
Contributor

Let's merge.

@lanzafame
Copy link
Contributor Author

@anacrolix One moment, I may have a fix for jenkins as well.

@lanzafame lanzafame force-pushed the feat/go-mod branch 4 times, most recently from e073a80 to 0cbaf16 Compare January 29, 2019 02:33
@lanzafame
Copy link
Contributor Author

@anacrolix @Stebalien I am happy for this to be merged now. Jenkins won't pass as it requires the pipeline to be updated but the PR has been created ipfs-inactive/jenkins-libs#50.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the inadvertent dependency upgrade, it looks sane. I'd like us to decide as a group (if we haven't yet, in which case I apologise) if we are taking this chance to upgrade upstream dependencies to their master HEAD.

If we are, this has implications for the security audit, and also we need to leave some kind of easy breadcrumb. Otherwise, things might start breaking from all different directions and we might end up investing too much time in tracing it down to a particular dependency upgrade.

go.mod Outdated
github.com/libp2p/go-stream-muxer v3.0.1+incompatible // indirect
github.com/libp2p/go-tcp-transport v2.0.16+incompatible // indirect
github.com/libp2p/go-testutil v1.2.10
github.com/minio/sha256-simd v0.0.0-20190117184323-cc1980cb0338 // indirect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed all dependencies, just random ones. But this one points to the current tip of master at the time of writing (minio/sha256-simd@cc1980c) instead of pointing to the commit that the gx version had snapshotted.

I'm not sure if this is intentional or inadvertent. If it's intentional, I suggest we change the title of this PR to add go mod + upgrade dependencies to make it explicit for traceability. Otherwise things might start failing beneath our feet if there are new bugs upstream.

We might want to enumerate which dependencies we're upgrading too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this matters.

@raulk
Copy link
Member

raulk commented Jan 30, 2019

Also, can use the method described here to test gomod support in Jenkins with this PR? ipfs-inactive/jenkins-libs#50 (comment)

@anacrolix
Copy link
Contributor

Has this stalled because we want to keep gx?

@anacrolix anacrolix mentioned this pull request Feb 7, 2019
@lanzafame
Copy link
Contributor Author

@anacrolix No, I have just been swamped with ipfs-cluster. And realistically, I am not willing to make calls about the security concerns of libp2p dependencies upgrades. Once you and @raulk have come to a conclusion about that, I will be happy to move forward.

@raulk
Copy link
Member

raulk commented Feb 7, 2019

Has this stalled because we want to keep gx?

As we've discussed elsewhere, we cannot suddenly drop support for gx for several reasons, out of which at least the following spring to mind:

  1. Downstreams like IPFS, Textile and others use it and depend on it.
  2. There's no solid integration layer between gomod and IPFS (we did spike a proxy, but not sure what's the status), so dropping gx is a step backwards functionally. At the very least, we'd lose IPFS publishing/fetching and release archival (Github tags do not guarantee anything) with no functional equivalent.
  3. Decisions like this need to be taken widely as a community and not in individual PRs.

From reading the discussion we all participated in, we agreed to experiment with go mod, because we could: (a) adopt it gradually, (b) non-invasively and (c) make it live alongside gx.

So to clarify: yes, we need to keep gx until we can shim the lacking functionality in gomod.

I'm actually wondering if CircleCI/Travis stop building with gx when go.mod is introduced 🤔

@lanzafame
Copy link
Contributor Author

@anacrolix re: Jenkins it seems the linux machines can't pull down some gx deps and it times out. But mac and windows pass just fine, so I don't know what is going on there.

@anacrolix
Copy link
Contributor

Ready to merge if you're not worried about caching GOCACHE and installing binaries in the repo.

@lanzafame
Copy link
Contributor Author

I am happy to merge and improve caching later. @anacrolix shall I leave it to you to merge?

@anacrolix anacrolix merged commit 7e68ac3 into master Feb 12, 2019
@ghost ghost removed the status/in-progress In progress label Feb 12, 2019
@anacrolix
Copy link
Contributor

Okay let's see how it goes. Next steps are to see if any downstream gx or go mod users barf on the major versioning.

@anacrolix anacrolix deleted the feat/go-mod branch February 12, 2019 06:40
@prestonvanloon
Copy link

Our CI is barfing.

A lot of /home/travis/gopath/src/github.com/libp2p/go-libp2p-kad-dht/dht.go:11:7: could not import github.com/libp2p/go-libp2p-kad-dht/v5/opts (cannot find package "github.com/libp2p/go-libp2p-kad-dht/v5/opts"

https://api.travis-ci.com/v3/job/177080918/log.txt

@raulk
Copy link
Member

raulk commented Feb 12, 2019

Backing this change out as it has broken downstreams. Clearly we didn't consider all cases and we need to go to the drawing board, because some things like suddenly changing import paths are not admissible. (see EDIT)

A note for the future: @anacrolix @lanzafame we never merge PRs that have "request changes" statuses.


EDIT: I see that the version token in the import path is virtual, and maps to a branch/tag in Go versions 1.9.7+, 1.10.3+ and 1.11+. What broke is their linter (gometalinter).


Reconsidering our gomod versioning approach

Can we try to avoid these funky and erratic version numbers across modules?

I suspect we were "lavish" with version numbers in the past because gx did not surface them in any way. Gx is about hashes, and version numbers are purely informative and for human-friendly archival purposes. In fact, as we all know, some gx releases in some repos are not even tagged. Now that version numbers have stronger implications, we should consider other options. I really don't want to see v2, v3, v4, v5 randomly across our codebase.

Here's an idea:

  1. Rename all tags gx tags to prefix them with gx-.
  2. Change the tool to produce gx- prefixed tags from now on.
  • I don't think gx consumes tags in any manner, only hashes.
  • And it just generates them for archival purposes, so it has no implications for dependency management.
  1. We can now use pre v2 lingo for gomod, drop versions from import paths, and start clean from 1.0.0.
  2. Unfortunately we lose the ability to reference existing gx tags (e.g. v4.1.2) in require dependency statements. Let's reference those past releases by commit, and when we need to make a new gx release for the dependency, let's commit to add a go.mod file, which should be picked up by dependents automatically thanks to minimal version selection.

For gx users: Since we'll still carry out gx releases using the old version line (as they are locked in package.json), downstreams will not notice anything, because they anyway import dependencies by hash.

raulk added a commit to raulk/go-libp2p-kad-dht that referenced this pull request Feb 12, 2019
raulk added a commit that referenced this pull request Feb 12, 2019
Revert #236: Test go mod in travis and use major versioning in import paths
@anacrolix
Copy link
Contributor

@raulk No such complicated re-versioning is necessary. The random "v5" thing is a part of the future of Go, it's just how it's going to be. Everything else I'll address in #258.

@raulk
Copy link
Member

raulk commented Feb 12, 2019

@anacrolix What I’m saying is that we should ditch “v5” because it brings no benefit in our case. We are really v1. And the above approach is a proposal to wipe the slate clean without losing history.

@raulk
Copy link
Member

raulk commented Feb 12, 2019

That v5 is a side effect of a version lineage that brings no value IMO. This is a case of XY problem.

@lanzafame
Copy link
Contributor Author

@raulk I am happy for all git tags to be prefixed with gx- and things to go back to v1 across the board for all >=v2 libp2p repos. I am not willing to make those changes though so I will leave it to you and @anacrolix.

@Stebalien
Copy link
Member

So, I (and david, actually) would prefer to reversion. The only reason I haven't already done this is to avoid:

  1. Breaking downstream users who are already using our tags.
  2. Accidentally re-introducing these tags on push (not sure how git handles this).

Note 1: Really, we could just delete all those tags as gx doesn't care. Again, the issue is that we'll break anyone else relying on those tags.

Note 2: We should go back to v0 across the board. We're not v1 yet.

Note 3: I wouldn't optimize for users using old tooling that doesn't support the /vN/ syntax. Given the go stability promise, our policy has always been to update our minimum go version as needed, dragging everyone else along kicking and screaming if necessary.


So yeah, if you think the impact won't be that bad, I'd be all for re-versioning everything (not for the /vN/ issue but because we should have sub-zero versions).

@anacrolix
Copy link
Contributor

I'm not sure we should delete the tags, or rewrite any versions. I think the past should be immutable: Existing users can find ways to continue with what they had if they don't like the future direction, we shouldn't pull the rug from under them.

As Raúl pointed out, support for major versions in import paths is backported as far as Go 1.9. Given that version 1/2+ is intended eventually anyway, and that the past should be immutable, I think we should just proceed with the existing versioning, and make use of the stability guarantees going forward. It's a lot of fanfare for a vanity number (see semver 2.0 lol).

I'm not sure what gx behaviour is with the major versioning, but if it relies enough on the Go tooling, it may be transparent to it if we're lucky. If it isn't, proceeding with "+incompatible" go modules, or leaving gx behind are options (per above, they can opt not to proceed beyond their current versions, or do the necessary wrapping superficially to the libp2p go mod repos).

@anacrolix
Copy link
Contributor

Also note that a lot of the benefit of not having the major version in the imports, by virtue of having a version 0 or 1 major release, is immediately lost when you do finally make 2+. We'd be coupling our future major release version with tooling support guarantees, which seems very undesirable. So resetting versions to gain some respite from bad tooling seems like a really hacky loophole.

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

Successfully merging this pull request may close these issues.

5 participants