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, assuming current master is a v3 branch #327

Closed
wants to merge 1 commit into from

Conversation

mwf
Copy link

@mwf mwf commented Jun 28, 2018

Import paths are changed according to Semantic Import Versions rationale.
Otherwise vgo test ./middleware fails bacause of import path interpretation - it assumes
that "github.com/go-chi/chi" points to v1, fails to find some public functions from v3 and dies.

As semantic import versioning seems to be the official way, we should respect it.

This is an experimental way of implementing #302 without major version bump.
Backwards compatibility is done via v3 symlink - old go is OK with it.

Import paths are changed according to Semantic Import Versions rationale.
Otherwise `vgo test ./middleware` fails bacause of import path interpretation - it assumes
that "github.com/go-chi/chi" points to v1, fails to find some public functions from v3 and dies.

As semantic import versioning seems to be the official way, we should respect it.

This is an experimental way of implementing go-chi#302 without major version bump.
Backwards compatibility is done via `v3` symlink - old go is OK with it.
Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

The direction of this PR looks good to me.

However, I'm not a big fan of having to change the import paths everywhere to the /v3 suffix. It affects the middleware subpkg too, as seen in the changeset. We need to be VERY careful not to break existing go ecosystem that is not aware of Go modules -- can they still use this project "as is" without the "/v3" suffix?

- "github.com/go-chi/chi"
+ "github.com/go-chi/chi/v3" 

The "v3" directory symlink hack may not work for everyone. Or is that a recommended approach for the v2+ projects that are adopting vgo?

v3
@@ -0,0 +1 @@
.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a symlink, right?

it's a nice hack .. but does this work outside of Unix at all?

Copy link
Author

Choose a reason for hiding this comment

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

Yeap, it's a symlink.

Unfortunately, I don't have Windows to check. But I bet it doesn't work on Windows.

Anyway, the official approach is to use go 1.9.7+ or go 1.10.3+ and drop support of older versions.

And with this symlink we give Unix users with older go versions a chance to keep using it for a while.

As you can see, CI with go 1.7.x and 1.8.x passed successfully


require (
golang.org/x/net v0.0.0-20180611182652-db08ff08e862
golang.org/x/text v0.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we use golang.org/x/text ?

Copy link
Author

Choose a reason for hiding this comment

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

It's used in golang.org/x/net undercover, for Unicode support.
golang.org/x/net haven't adopted go.mod yet, so golang.org/x/text resolving appears here.

As soon as go.mod is introduced in golang.org/x/net and you update it, transitive dependency will go away automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that sounds good to me.

@cskr
Copy link

cskr commented Jun 28, 2018

@VojtechVitek The latest point releases of go (1.10.3 and 1.9.7) can work with /v3 in middleware's import without the symlink. This change describes the details. Will it be reasonable to change the minimum go version required by bumping version to 4.0.0?

@VojtechVitek
Copy link
Contributor

VojtechVitek commented Jun 28, 2018

@cskr I don't think we want to bump to v4.0 just because of vgo :) we gotta figure this out, or let the Go authors know that vgo sucks

I tried a while ago.. see golang/go#25967

btw: We can't assume people upgraded to go 1.10.3 and 1.9.7 .. we have to be backward compatible with older versions.

@mwf
Copy link
Author

mwf commented Jun 28, 2018

However, I'm not a big fan of having to change the import paths everywhere to the /v3 suffix. It affects the middleware subpkg too, as seen in the changeset. We need to be VERY careful not to break existing go ecosystem that is not aware of Go modules -- can they still use this project "as is" without the "/v3" suffix?

Yeap, pre-go1.9.7 versions can use the project "as is", if they are on Unix, as commented above. I don't see a quick-win way for Windows users though.

The "v3" directory symlink hack may not work for everyone. Or is that a recommended approach for the v2+ projects that are adopting vgo?

No, the only recommended approach I've seen yet is to use go1.9.7+ and go1.10.3+. But this approach works :)

There is a possibility, that it can break some opensource toolset (maybe goimports for example?), but only in case if the author didn't think about symlinks resolving. It could be considered a bug in the toolset, not in the project.

@pkieltyka
Copy link
Member

I haven't been following the vgo specification -- I've been waiting for the dust to settle. I trust @VojtechVitek 's intuition/direction on the matters so will take lead from you guys, but one of the goals of chi is to have no dependencies besides stdlib, so where do the require's for golang.org/x/net and /text come from?

I do understand the need for versioning of chi and allowing other projects depend on a particular version of chi easily.

@pkieltyka
Copy link
Member

personally I was happy with https://github.com/golang/dep but, the Go devs are much smarter than me, so I must be out of the loop on the latest thinking

@mwf
Copy link
Author

mwf commented Jun 29, 2018

so where do the require's for golang.org/x/net and /text come from?

https://github.com/go-chi/chi/blob/master/middleware/middleware18_test.go#L12

We're using golang.org/x/net in tests.
go.mod doesn't make a difference between test and regular dependencies.

Actually, it's OK, because it doesn't force anyone to use your particular dependency version, it's just a minimal version.

@VojtechVitek
Copy link
Contributor

VojtechVitek commented Jun 29, 2018

Yeap, pre-go1.9.7 versions can use the project "as is", if they are on Unix, as commented above. I don't see a quick-win way for Windows users though.

I don't think so. Afaik, the Unix users would have to switch to "/v3" suffix everywhere too, since the middleware package imports the "suffixed" github.com/go-chi/chi/v3. Without this change, this PR wouldn't really work "as is" in older versions than go 1.10.3 or go 1.9.7.

Since this situation is very uncomfortable for both v2.0+ pkg authors and consumers, I suggest not to release a go.mod file, unless Go authors change their mind / figure this out .. or until we're comfortable supporting go 1.10.3+ or go 1.9.7+ only (I guess we'd have to wait at least for a stable go 1.12) or when we release v4.0.0 (which is not likely in the near future).

For now... vgo pioneers (including myself) will have to stick to the v0.0.0-DATE-HASH versions, since vgo is not friendly to Github packages that already released v2.0.0+ tags (see golang/go#25967) :-(

require(
    "github.com/go-chi/chi" v0.0.0-20180202194135-e223a795a06a
)

(as suggested by @nicpottier in #302)

@mwf
Copy link
Author

mwf commented Jul 5, 2018

@VojtechVitek sorry for answering so long, I was terribly busy these days :(

I don't think so. Afaik, the Unix users would have to switch to "/v3" suffix everywhere too, since the middleware package imports the "suffixed" github.com/go-chi/chi/v3. Without this change, this PR wouldn't really work "as is" in older versions than go 1.10.3 or go 1.9.7.

Actually, you're not quite right. Old go users on Unix won't have to do anything with their imports if they get this version.

Please take a look at this test project https://github.com/mwf/chi-test-go1.8
I download go1.8 there and my fork to vendor - everything works just fine. You can test the same with any version you want.

So the only real stopper for the symlink way are Windows users.

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

@mwf

Old go users on Unix won't have to do anything with their imports if they get this version.

I'm not sure. Let me give you an example:

Imagine a project in Go 1.10 (or any other version that's not aware of "Go modules") that imports

import (
	"github.com/go-chi/chi"
	"github.com/go-chi/chi/middleware"
)

now.. if we merged this PR and user updated chi to a version with this new symlink hack, the github.com/go-chi/chi/middleware pkg would suddenly start importing "github.com/go-chi/chi/v3" instead of "github.com/go-chi/chi" -- a different path => a different package.

Now, the user would technically end up with both

	"github.com/go-chi/chi"
	"github.com/go-chi/chi/v3"

in the codebase, which would cause type and context value mismatches.

I'd like to see Go authors to come up with an official solution that takes care of these v2+ upgrades.

cc @myitcv

@myitcv
Copy link

myitcv commented Jul 9, 2018

@VojtechVitek I commented in golang/go#25967 (comment)

@VojtechVitek
Copy link
Contributor

From golang/go#25967 (comment)

mvdan commented 4 hours ago
@VojtechVitek how many Go versions do you need to support? With 1.9.7 and 1.10.3 now out, any project supporting up to the two last Go versions (which I think is a majority) should be able to switch to Go modules soon.

The only potential problem I see is people still using old minor releases like 1.10.2. This is why I'm holding off for a few weeks before porting my v2+ projects to Go modules. I'll probably do the switch once 1.11 is out, at which point backported support will have been out for two full months.

If you need to support the latest three major Go versions, waiting until 1.11 is out should also do the trick. This obviously doesn't scale past three major Go versions, but hopefully the set of v2+ packages with that constraint is very small.

myitcv commented 2 hours ago
@VojtechVitek as @mvdan points out, if you are happy only supporting Go 1.9.7 (or later) and 1.10.3 (or later) from the 1.9.x and 1.10.x series respectively, and of course Go 1.11 and later, and your project is v >= 2, then you can safely convert your project to be a Go module. Otherwise you will hit issues with older Go versions not being able to handle the /vX in import paths.

VojtechVitek commented a minute ago
I'm not happy dropping support for Go <=1.10.2, <=1.9.6 and 1.8.x for a long long time. I'm talking year+, maybe until 1.12 is out.

Go was always about backwards compatibility ... and now we just break stuff because vgo can't support the existing git tags properly .. and requires new "module" notation instead :(

@mwf
Copy link
Author

mwf commented Jul 17, 2018

Adding a ref to golang/go#26238

Go authors seem to understand the pain, so maybe they will start picking latest tagged version in automatic imports.
So users won't suffer from missmatched versions after vgo mod -init && vgo mod -sync and we won't need adding go.mod until you decide to drop older go version support and release v4 with Semantic Import Versioning.

Let's watch where it leads us.

@mwf
Copy link
Author

mwf commented Jul 25, 2018

golang/go#26238 is closed, and you can try go1.11beta2 - we don't need go.mod to make the go1.11 users happy - old v2+ repos work with new go out-of-box.

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.

None yet

5 participants