Work around issue#1244 #1253
Work around issue#1244 #1253
Conversation
- go.mod module name changed `github.com/labstack/echo` -> `github.com/labstack/echo/v3` - replace imports with `.../echo/v3...`
Codecov Report
@@ Coverage Diff @@
## master #1253 +/- ##
=======================================
Coverage 81.51% 81.51%
=======================================
Files 26 26
Lines 1942 1942
=======================================
Hits 1583 1583
Misses 250 250
Partials 109 109
Continue to review full report at Codecov.
|
|
@vishr @im-kulikov I don't think this is the correct solution to solve this problem. It adds unnecessary "noise" in the source code, makes it harder to work on subsequent versions (V4, etc.). Still accommodating to Go modules myself, but I much rather prefer to use go.mod to control version, then use the source code to hardcode that. With go.mod, all the versions are in one place, easy to review/change. That being said, the problem here is NOT that Echo cannot find it's own (sub)packages. I didn't test, but I doubt that's what the problem is. Rather, when people are pulling in Echo via Go modules, they pull the latest version, which is taken from the TAGs being defined. We actually have a problem with the way we tag things, and we should address that, rather than finding workarounds. Yes, we do have the v3.3.5 to v3.3.8 tags BUT they are FORKS from the mainline... no idea how that happened but it does not look right. That needs fixing, and then the issue should go away, with no other code changes. |
|
See how v3.3.4 is the last one in mainline and the rest are all forks? That's the problem. |
|
It's even worse: https://github.com/labstack/echo/blob/master/echo.go#L224 The latest code in master still reads 3.3.dev not even 3.3.4, nevermind 3.3.8 |
|
@vishr I think I see what you were trying to do, just keep it 3.3.dev in the mainline, and then fork off and tag those. It makes some sense, after you do work on say 3.3.4, it's no longer 3.3.4, it's "3.3.4 plus something" but not yet "3.3.5" either. But this solution you came up with is not playing nice with Go modules. We need the tags on the master branch, not as forks. |
|
not sure... that problem with that..
there is no v3.3.2... mb problem with that? |
|
my PR based on this video https://www.youtube.com/watch?v=H_4eRD8aegk |
|
No problem with missing v3.3.2 If the end users want to, they CAN in fact force pulling these tags:
But clearly, go modules does not like tags on forks, even sets version as v0.0.0 ... Thanks for the video, haven't seen it yet, will do with the 1st chance. I've used https://github.com/golang/go/wiki/Modules and found that very helpful. |
|
@im-kulikov - after reading some more from the link I posted, I see that you are correct. Indeed the way you started seems to be a recommended way, as per https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher However, per the exact same recommendations, they suggest making a major version bump when adopting go modules. The key is here: "Update the go.mod file to include a /v3 at the end of the module path in the module directive (e.g., module github.com/my/module/v3). Update import statements within the module to also use /v3 (e.g., import "github.com/my/module/v3/mypkg"). Tag the release with v3.0.0." Without that, things will break this way: https://stackoverflow.com/questions/52050146/go-build-keeps-complaining-that-go-mod-has-post-v0-module-path That is the error I got when I tried to import your fork directly: 1 - created this file (in a new/empty folder): package main
import "github.com/im-kulikov/echo"
func main() {
e := echo.New()
e.Start("")
}2 - ran 3 - edited go.mod and changed it from 4 - running Did you try importing your branch from another module yet? Did it work on your end? |
|
You receive error because |
|
OK, looking forward to the demo. Though, a /v2 demo would be impossible if I understand Go modules correctly. It would mean that the v2.0.0 tag should have a go.mod file defining it as a v2, which is impossible without rewriting the history from v2.0.0 onwards. Which of course makes little sense for a demo. Pretty much the same for /v4, cannot demo that without introducing a v4.0.0 tag... Also, this PR would introduce breaking changes, for users that use Go < 1.11.x since FWIW, the solution I proposed (properly tagging commits on the master branch, not on forks) would work both for Go module users, as well as for older Go versions. Unfortunately it's not a recommended practice, so that's it. |
|
@im-kulikov @alexaandru This is a good discussion. With this change will users still import |
|
@vishr with this PR import must be changed to github.com/labstack/echo/v3 and Go 1.11+ required Not sure if fixing releases will help, but it's worth a try. |
That will be too much for everyone. Somehow earlier I though this import change is just for internal code. Now I am leaning towards what @alexaandru suggested. |
|
No problem, let's try |
|
If we simply tag the current master with v3.3.9, that would work both for non Go modules users (they would not be affected in any way) and the module users will pull the v3.3.9 by default (when they don't specify a given version, which is what 99% of the will do anyway, at least for now). Sounds like a win-win. But we should change the Echo version accordingly (to say 3.3.9 not 3.3.dev). |
Let's change version reference in the code to |
|
Found quite a topic on this: semver/semver#124 :) Tl;dr: semver thinks this it's an implementation detail :) I'm good with the proposal with one change: should use a dash to separate the additional label, i.e.: |
@alexaandru Cool, can you help me with the release? |
|
Sure, I can do that. |
|
I did change the version, tag and push, however that did not fix it :-( v3.3.5 is still being pulled. I did not cut a release just yet, until we clear this out, as I want to indicate the solution in the release message (what end users need to do to get the latest). |
|
@alexaandru I take it there's no point in the demo (go mod /vX) anymore? |
|
@im-kulikov The work you did is needed, as that is the way to go, but we will need to cut a 4.0.0 release for that, since it break backward compatibility. For now, I'd just want to get it to work for both Go module users and for those that don't use them yet (well, they would just pull the latest master with |
|
Agreed, only need to make changes, if we are talking about 4.x.x |
|
I reapplied the tags on master, however that did not fix the problem :-( I'm sorry, I really thought this could fix it, oh well, looks like we need to follow https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher to the letter, which includes cutting a 4.0.0 release. Things are not that bad though, seems they introduced a "compatibility mode" for older releases as well: golang/go#27009 (comment) The key phrase there is: " The short version is the go command "does the right thing" when using Go 1.9.7+, 1.10.3+ or 1.11, including old code that has not opted into modules does not need to be updated to include the major version in import paths when consuming v2+ modules." so we will not require with V4 branch that people use 1.11 right away, they can still use 1.9 or 1.10 provided they use a recent version. Or they can remain on the latest release before V4. Anyway, gonna play with this a little on my own, need to understand exactly how this work. Seems that when and how exactly you introduce go.mod file makes all the difference. Simply adding a V3 now or releasing a V4 instead may not necessarily fix the issue... :-( |
|
@vishr @im-kulikov I was able to reproduce the issue! :-) https://github.com/alexaandru/modules Now playing with it further to see what the fix can be. Will introducing the next major version fix it?! We'll see... :-) |
|
YES!! Introducing the /v3 module with the next major version, fixed it, PROVIDED that the end users import it as /v3 (without using the suffix they still pull the latest version before go.mod - 2.1.0). So we have a clear path for fixing this. @im-kulikov do you want to amend your PR? Also, could you please introduce a new section in the readme that details the constraints? I.e. something like: RequirementsGo 1.9.7+, 1.10.3+ or 1.11+ are required in order to use V4. If using Go modules, then you must import with /v4 suffix. For older versions, please use the latest v3.x.y tag. Something like that, to make it clear up front. Thanks! |
|
No problem. If we add /v4, the minimum requirements would be go 1.11+, it’s ok? |
|
Thank you :-) And no, please read my earlier comment: #1253 (comment) it is not true that we require 1.11+ They introduced changes in Go to support some compatibility, i.e. older versions will not have full module support BUT, they will understand that /vN is a logical thing and not error out that the branch does not exist. So it won't break for 1.9.7+, 1.10.3+ either, which is awesome :) Of course, below that it will break. Also, just for clarity, previous version of Go (other than the ones mentioned above), will not be able to use V4. They will still be able to use Echo, but the latest would be v3.3.9. If they want a newer version they must upgrade to a newer Go 1.9.x, 1.10.x or 1.11.x. |
|
cc @vishr we'll need your blessing on this one :-) : adding a V4 as per https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher to properly support Go modules. This will mean that going further (so from V4 on) we will only support Go 1.9.7+, 1.10.3+ or 1.11+ |
|
There is one other option, mentioning it for the sake of completeness: if we do add the /v4, the v4.0.0 tag but NOT the internal /v4 imports, then it should work for both Go module users AND older Go (older than the ones mentioned) users. The only thing breaking the compatibility is those internal imports. However that would be against the official recommendations, so I don't think is worth it. I don't personally agree with those recommendations, I mean the "internal" imports, should use the current version of the package anyway, regardless if that version is v4, v5 or whatever, so why suffix them? Makes no sense, just seems like unnecessary work to me. But if those are the recommendations, what can we do? |
|
Closing this @im-kulikov please open a new one for V4 (if you want and have time to tackle it, if not I can look into it). Thanks! |
|
@alexaandru kk, no problem |
github.com/labstack/echo->github.com/labstack/echo/v3.../echo/v3...PS: if I correctly understand #1244
cc @vishr @alexaandru