-
Notifications
You must be signed in to change notification settings - Fork 295
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
Fix module issues once and for all #299
Comments
@ugorji i am pretty sure that option 3 is a valid solution is probably preferred to support backward compatibility with those folks that depend on the library from the main module. It is just unfortunate that you will have to maintain two sets of tags requiring making sure that they are always on the same commits. |
https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories
This is why I'm not sure if 3. is a valid option, as this suggest that .../go movie will NOT contain .../go/codec |
I think option 1 is a more practical choice. In gin-gonic, just use go/codec decode msgpack. |
Option 3 will not work easily. If the codec directory contains a go.mod file, its contents are "carved out" from the outer module and won't be included in it. As far as I know, you can only make it work by maintaining two parallel git histories: one for inner module (with go.mod in subdirectory) and another for outer module (without go.mod in subdirectory). I don't think that's practical. |
I suspect you could make any of those three options work... But the simplest in the long-term might be option 2, which is what the remainder of my comment focuses on:
Having a single That also happens to be the current state of the latest release here, as far as I've followed -- it seems v1.1.4 has a single go.mod in the repo root: https://github.com/ugorji/go/blob/v1.1.4/go.mod However, it seems the main issue with the current state is multiple consumers have the old In general, moving The modules wiki covers this in a fair amount of detail if you are adding a new module: https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository However, removing a module from a multi-module repo (which I think is what happened here most recently) is covered much more briefly -- it says:
On the other hand, there is a more precise but older description of removing a module that lives within the code itself. The full section is here, and worth reading: where this snippet here might be the piece that is most applicable in terms of next steps:
That might be an approach here -- prepare a new But, I've never done that myself, so take that as a theoretical possibility that might or might not be correct. Also, there might be a simpler solution. CC @jadekler (the main author of the multi-module repo documentation) |
@thepudds Thanks much for this detailed explanation. Much much appreciated. I messed things up by not respecting how tricky this would be, and trying to fix it quickly by moving go.mod around, which made it worse. Now there are frustrated users on both the .../go module and .../go/codec module. It will be nice if I could get option 3. to work somehow. When I read https://github.com/golang/go/blob/release-branch.go1.12/src/cmd/go/internal/modfetch/repo.go#L106-L125 , it suggests to me that I can support both, by having 2 modules that are kept in sync.
This way, folks can use either of the module versions and work. Do you (or does anyone else with more module experience) see a reason why this wouldn't work? ^^ @jadekler |
I did a light review of the issues involved. It seems like you had added a module below the root, creating a multi-module repository, and then removed it? I agree with @thepudds that the easiest solution is for all your dependors to update to latest version of
I'm not sure this works: users still would need to upgrade to the later version of the module, no? Sorry I never got around to fleshing out Is it possible to remove a module from a multi-module repository?. I should go back and add more detail to that. |
This commit was created by manually removing github.com/ugorji/go/codec from the go.mod (which now does not exist - see ugorji/go#299) and running `go mod tidy`. This removes the borked github.com/ugorji/go/codec dependency, fixing dependors of this library. This commit should be included in a release ASAP.
I've created spf13/viper#705 to fix the viper dependency. Any other dependor needs to do the same AFAICT. |
This commit was created by manually removing github.com/ugorji/go/codec from the go.mod (which now does not exist - see ugorji/go#299) and running `go mod tidy`. Closes #658
@jadekler thanks for sending that PR to viper! At least so far, that seems to have resolved the problem for viper. |
Hi @jadekler, I think each of the options 1-3 outlined here will involve upgrading. In the approach I had tried to outline in #299 (comment), the requirement cycle means though that it ends up being a consistent upgrade: you don't end up with a build that has a mix of the old and new (which is what can trigger the ambiguous import error if the same package appears in two modules due to having moved). |
I think the option I had outlined in #299 (comment) ends up working both for consumers who are currently using the .../go module as well as for consumers currently using the .../go/codec module. For example, if a consumer is using the .../go/codec module currently, if you were to do the approach in that comment, then when they upgrade, the .../go/codec module ends up not having any code, but it has a Separately @bcmills looked over that comment and he mentioned he didn't see anything wrong there... So... I think there's a healthy chance it would work. A concrete next step could be to try on a test repo.
I think I am not 100% following the suggestion there, or in particular what you mean by "kept in sync". If you were to go with that approach, you might need to set up a requirement cycle between the two modules so that they depend on each other, and then also make sure the tagging uses In any event, I think there are likely multiple workable approaches here, and likely debatable which one is "best". That said, it seems to me the approach I had outlined in #299 (comment) should work for existing consumers today, and might end up being the simplest over the medium and long term, including you could encourage people via your README to standardize on the Finally, this is all nuanced enough that whatever approach you are going to try, it probably makes sense to try on a test repo first... |
@jadekler wrote
Right. They will always need to upgrade to the later version of the module. I just thought that having the module itself be updated might help resolve some of the "ambiguous version" issues. Either way, as @thepudds wrote, an upgrade is needed. @thepudds wrote
I think it would require more churn in the git history if I do it this way, i.e. have a github.com/ugorji/go/codec on a different branch which is empty, and have a requirement cycle between both. Also, that might affect non-module uses (Go 1.9-1.12).
My concern here is still the location for it. It feels like a requirements cycle is the main important thing here, that we can enable.
You are right. I was talking about having 2 tags which point to the same revision (v1.1.5 and codec/v1.1.5) and having go.mod in both folders (.../go and .../go/codec) where they both require one another in a cycle. That should satisfy your core idea that the 2 "modules" require one another, allowing us to continue to work in master, while supporting module and non-module execution without much churn in github. Fundamentally, I just want to find out if this will solve some of the issues folks are having so that a |
came here searching for a solution to this, didn't see a message with people running into it so I'll post in the hopes the responses help others. go get seems to be completely broken with anything depending on msgpack through modules atm. is there any workaround (short of working around go modules in general) that I can implement while ya'll sort this out? should have clarified originally: "completely broken" in this case means that I depend on gin-gonic, which depends on msgpack, which depends on go/codec.
More detail because I wasn't thinking and didn't supply
anyway HTH. I got it working, but had to downgrade gin to 1.3, for anyone looking. |
@erikh would you be able to put together a quick playground link that shows your original problem? You could use this as a starting point: |
got a lot on my plate today but I will try to put together a repro
tomorrow; sorry, I just deleted about 40k LoC and need to get that into
shape. :)
I hate seagulls too.
…On Sun, May 26, 2019 at 5:11 PM thepudds ***@***.***> wrote:
@erikh <https://github.com/erikh> would you be able to put together a
quick playground link that shows your original problem? You could use this
as a starting point:
https://play.golang.org/p/QvLwENa4pTp
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#299?email_source=notifications&email_token=AAAET25XEQQRRQXHUN5OJJDPXMRLLA5CNFSM4HPLD7CKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIQETQ#issuecomment-496042574>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAET24MMR5U5QTDTOGNQ33PXMRLLANCNFSM4HPLD7CA>
.
|
Suppose I create the 2 tags: I don't know how to generate the error cases, so it's hard for me to test this out. I seemingly have to depend on expert advice and then see what things break. I keep seeing error reports like: swaggo/gin-swagger#58 |
That should work. For testing, one thing you could do is create a branch, then introduce the two modules and tag them with a pair of prerelease tags that do not have the requirement cycle (to see then the error trigger). Then introduce the requirement cycle with a new pair of prelease tags on that branch, which then should avoid the error. A real consumers would not get any of the prerelease tags by default (only if explicitly asked for, which a real consumer would not normally do). (On mobile, so brief) |
Also, if you do end up putting a branch together like that, I probably can generate a quick example of showing the error triggering, and then showing the error not triggering (as hopefully expected). |
https://github.com/ugorji/go/tree/v1.1.5-pre I created a branch Hope this is sufficient for you. Thanks. |
@ugorji Here is a quick test for you. It uses If interested, here is an overview of the commands you can place in a testscript here: Here is an overview of the testscript utility itself, which provides a convient way to run from the command line: To run the test below, copy it to a file such as
Here is the basic test I put together just now:
Running it reports time taken, along with a 'PASS' result:
|
Also, running with |
@thepudds Thank you so very much. This is awesome!!! I just ran it and saw that it passed also. I will release v1.1.5 by the end of the week with this requirements cycle with a FAQ entry. That should hopefully put these issues to bed, as it should be sufficient to do |
@ugorji Side note: the more interesting test case is probably without the
The I could be wrong about this, but in your case I think in theory that means the cyclical requirements could have the incorrect (too old) versions listed, and In any event, I suspect a test without using |
I just ran with -u and without -u, and the tests still pass.
I will test with both when I push 1.1.5 (likely over the weekend). |
Great. (If the versions are set properly in the cyclical requirements, I would expect both with and without |
FYI - I am moving the update to upcoming weekend. I wanted to get a few enhancement requests that came in over the past week in, and then give a week to soak before cutting a new release. |
I'm having the same problem as described here: #303 Please let me know how to solve it |
Add this line to your
|
@qaisjp thanks for your hint. There must be something else to fix: Even if actually modified go.mod:
The problem persists:
I found that a new directory within go/pkg/mod/cache/download/github.com is created:
|
@marcoippolito I will be pushing out a v1.2 release around the end of next week. Just FYI, in case that helps. |
I just released a go-codec production release - version 1.1.7 (finally) with the fixes to ensure the import paths Please let me know if seeing any issues. If all is well over the next few days, I will close this github issue. See |
Closing now after 3 weeks with no issues raised. Thanks for all your help, folks! |
The use of modules in go-codec introduced more headaches than necessary.
It has been a disaster, and I really wish I had waited until the dust settled before making this module aware, especially since so many people depended on this package and it has the atypical "package name differs from directory name" issue.
I regret jumping on modules that early, and having to constantly try to correct my mistakes.
It seems the dust has settled now and we have the tools to:
We have 2 options:
I am not sure if 3. above is supported by go modules. I read that a module doesn't include modules within it.
I will appreciate any thoughts on how best to resolve this. I am including references to places where I have seen folks have issues. I hope that you folks can respond and help me settle this once and for all with the best solution.
https://github.com/golang/go/wiki/Modules#publishing-a-release
https://golang.org/wiki/Modules#faqs--multi-module-repositories
gin-gonic/gin#1897
@dmitshur
@arizvisa
@bvisness
@HZ89
@bcmills
@amarox
@thepudds
The text was updated successfully, but these errors were encountered: