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

Support Go Modules #169

Open
marwan-at-work opened this issue Apr 3, 2019 · 25 comments
Open

Support Go Modules #169

marwan-at-work opened this issue Apr 3, 2019 · 25 comments
Labels
pending Keeps issues from going stale

Comments

@marwan-at-work
Copy link
Contributor

marwan-at-work commented Apr 3, 2019

Twirp should be able to support Go Modules for a few reasons:

  1. Pinning dependencies correctly when GO111MODULE will be set to on by default in the next release of Go 1.13

  2. It will make the onboarding experience to twirp a little bit easier because you'll be able to get binaries such as protoc-gen-twirp at an exact version without having to use retool. For example: go get github.com/twitchtv/twirp/[email protected]

  3. There will be no need to vendor dependencies on both the twirp side as well as the user's side.

I noticed that there was already an issue about adding go.mod/go.sum and the reason for closing it was this comment: #140 (comment)

In particular:

the transition will involve quite a bit more than just adding go.mod and go.sum files as import paths will need to change

For that reason, I created a tool to automatically upgrade import paths.

I have a draft PR to demonstrate that all import paths are upgraded and that the tests past (at least locally for me).

The PR is in draft because A. I'm not familiar with Twirp's roadmap and B. I imagine a lot of documentation needs to be updated to show the new flow of getting started.

Please feel free to keep this open until Twirp officially supports module, and also feel free to add all the TODOs that need to be accomplished before rendering this issue resolved.

@spenczar
Copy link
Contributor

spenczar commented Apr 3, 2019

I agree that we clearly need to eventually move to Go modules. My biggest concern is the migration path. I think we need to go very carefully here. I'd like to make three design constraints explicit:

First, we need to support the last two minor versions of Go. Today, that's 1.10, 1.11, and 1.12. Go 1.10 doesn't have module support directly, so until Go 1.13, we would need to "fake" modules by putting code in an explicit github.com/twitchtv/twirp/v6 folder. Perhaps that means it's best to just wait for Go 1.13 to release module support - that's OK with me.

Second, we cannot mandate users (even those in Go 1.12) adopt modules to use Twirp v6 generated code. There are two prongs to this argument. First, adopting modules is not easy: it requires changes to users' build systems, and involves a change in development practices and workflows. It's not a simple switch. Second, if we mandated module usage for generated code to work, that mandate is viral. It affects not just users who run Twirp services, but all of their clients. Those two arguments combine to mean that mandatory module adoption would be an onerous burden on too many applications - many of whom (the clients) might not even be using Twirp themselves directly. It's too rude to force them to change their builds to be clients of some other service.

Third, old generated v5 code must continue to work, even if a project has adopted modules and has other new generated v6 code. This is the dual of the above argument. If I run a Twirp service, and I'm also a client of some other older Twirp service, I can't force that service to adopt modules, and I can't force them to regenerate the client code. So if I choose to upgrade to modules, I still have to be able to import generated code from non-module-based projects and it needs to work.

Your current PR, #170, is the naive straightforward approach, but it breaks these constraints in several ways:

  • It breaks Constraint 1, since it uses github.com/twitchtv/twirp/v6 import paths, but doesn't move any code into a github.com/twitchtv/twirp/v6 folder, so go1.10 builds will fail.
  • It breaks Constraint 2, since clients of generated code would need to have modules enabled for github.com/twitchtv/twirp/v6 to be resolved correctly as an import, and that's used in generated code.

I am not sure whether it breaks Constraint 3, but it might. If we merged #170, could a program have old generated code (that didn't reference a github.com/twitchtv/twirp/v6 - instead, the generated code used github.com/twitchtv/twirp) alongside new generated code? Can both be linked in, for a program using modules? I am unclear on this, and would like clarity.

I don't know the way forward, but I'd like to talk it out. Do we need to move code into a ./v6 folder to support Go1.10? Do we need to do that, and also leave another copy of exactly the same code in the root of the project? I don't know - those seem gross! - but they seem plausible.

@spenczar
Copy link
Contributor

spenczar commented Apr 3, 2019

Thinking about this more, I think the minimal module compatibility design might protect us in Go 1.9.7+, 1.10.3+, and 1.11. I'm not sure whether it works in 1.12. I think this might address Constraints 1 and 2, which would be terrific, but I'd like help thinking it through carefully (or, even better experimenting with it).

@spenczar
Copy link
Contributor

spenczar commented Apr 3, 2019

Agh, no - minimal module compatibility really only kicks in if a project has adopted Go modules already, so it only addresses Constraint 1, but not 2.

@marwan-at-work
Copy link
Contributor Author

@spenczar thanks for the detailed explanation :) here goes:

Perhaps that means it's best to just wait for Go 1.13 to release module support - that's OK with me.

I believe that's the best approach. This way Twirp has plenty of time to figure out the migration path and you still get to support up to 2 versions behind the stable release.

Second, we cannot mandate users (even those in Go 1.12) adopt modules to use Twirp v6 generated code.

I believe this is why a new major version has to be introduced. A new Twirp major release, could dictate that you have to use modules, otherwise it wouldn't have to be a major release in the first place.

Let's take a few scenarios and how that might (or might not) break users.

Scenario 1: I am a Twirp server who is using Twirp V5. If I wanted to update to v6, that means I am aware of upgrading a major version and therefore I know this is a breaking change. A breaking change in this scenario means: switching my build system to Go Modules.

However, if I couldn't or didn't want to switch to Go Modules, then there's no harm in just continuing to use v5 indefinitely.

Scenario 2: I am a Twirp V5 server who wants to import a Twirp V6 client. I have two options:

A: if I'm using Go Modules, I can still be a Twirp V5 server (through the <version>+incompatible tag), and I can still import a Twirp V6 client and use it. This is because varying major versions of the same Module will not conflict due to varying import paths.

B: if I'm not using Go Modules, go 1.10.x and above should still be able to import the v6 client without having to have a v6 folder through the backporting issue you linked above by just adding a go.mod file and not completely relying on it as a dependency manager (this should definitely be tested as I'm not 100% sure)

Point B. of Scenario 2 has an underlying issue: when you import a new library, you have to be aware of its dependency manager in the first place. This is because dependency managers can't read each other's manifest files, so that's something a user has to be aware of to begin with.

As a last thought: is there any harm to saying that a new major version of Twirp is only modules-compatible, and that's it?. People can always stick to v5 until they're ready to upgrade. And if they want to import a library that transitively depends on v6 and must have modules enabled, then you must upgrade your build system first.

The PR I opened was definitely more of a demonstration than a solution. I'm glad Twirp maintainers are thinking carefully about this :) I will close the PR and can easily use the tool I wrote to re-open the PR with modifications if need be.

@spenczar
Copy link
Contributor

spenczar commented Apr 4, 2019

This is good progress, thanks @marwan-at-work.

I'm not thrilled with making Twirp v6 modules-only. I think it could impede adoption. If we can find a way to work in v6 for non-module-users, I'd be happier.

One option, which is pretty gross, is to make an explicit v6 subdirectory and move all the code into it. Then things work under Scenario 2B easily. To support the old v5 generated code being built alongside new v6 code without modules, we'd also have a copy of the Twirp runtime in the old github.com/twitchtv/twirp path. This would be done with aliases. Something like this:

package twirp

import v6 "github.com/twitchtv/twirp/v6"

// This file gives references to everything in v6 of Twirp.

var (
	HTTPRequestHeaders     = v6.HTTPRequestHeaders
	WithHTTPRequestHeaders = v6.WithHTTPRequestHeaders

	StatusCode                    = v6.StatusCode
	ServerHTTPStatusFromErrorCode = v6.ServerHTTPStatusFromErrorCode

	SetHTTPResponseHeader = v6.SetHTTPResponseHeader

	IsValidErrorCode = v6.IsValidErrorCode

	MethodName  = v6.MethodName
	PackageName = v6.PackageName
	ServiceName = v6.ServiceName
)

type Error = v6.Error
type ErrorCode = v6.ErrorCode
type ServerHooks = v6.ServerHooks

If this file exists, then github.com/twitchtv/twirp and github.com/twitchtv/twirp/v6 are both resolvable imports, even without modules. In the future, when modules have passed some threshold for adoption, we can remove the ugly alias.

I'm less clear on whether this works if modules are enabled. Experimentation would help. Is there a way we can work in a branch to try these things out, and make a few clients/services that reference each other and try out the scenarios?

@marwan-at-work
Copy link
Contributor Author

marwan-at-work commented Apr 7, 2019

One option, which is pretty gross, is to make an explicit v6 subdirectory and move all the code into it.

I think this option is totally worth it under two conditions:

  1. Twirp intends to support non-modules for v6 as well as go1.13
  2. Twirp V7 will end up moving the code out of the v6 subdirectory and back into the module root once enough time has passed (maybe go1.14/1.15) and that Twirp maintainers are confident that they can drop support for non-modules.

To support the old v5 generated code being built alongside new v6 code without modules, we'd also have a copy of the Twirp runtime in the old github.com/twitchtv/twirp path

This is the part I don't understand: why does Twirp need to make v6 backwards compatible with v5? The whole reason v6 is introduced is so that it's incompatible with v5. It still works without Go Modules, but it's still a breaking change, hence v6.

In other words, clients/servers of Twirp needing to upgrade to v6, will have to update all of their import paths to point to the /v6 subdirectory.

Is there a way we can work in a branch to try these things out, and make a few clients/services that reference each other and try out the scenarios?

I don't believe we can work on a branch because we need to make new tags/releases. So it would have to be on a fork, which is pretty alright.

Speaking of, I updated the branch in my fork to use a subdirectory. You can check it out here: https://github.com/marwan-at-work/twirp/tree/mods -- notice that I have a new release under v6.0.2 and you can use it with or without go modules to build Twirp services. You can see an example here: https://github.com/marwan-at-work/helloworld

You can git clone it inside $GOPATH/src/helloworld or outside of GOPATH to see it working with/without Go Modules

@rynop
Copy link

rynop commented Apr 11, 2019

My vote would be to make v6 modules only. My hunch is v6 GA is still a ways out. Basing this opinion on the fact that v6_prerelease was created about a year ago (#112) and v6 work started earlier.

The long runway will allow prep for devs, and gets us farther away from a time when golang did NOT have module/dependency management built in.

This opinion obv breaks early adopters (like myself) of v6_prerelease, but we knew the risks going in. IMO the benefit of adding modules outweighs the downside of breaking early adopters.

@marwan-at-work
Copy link
Contributor Author

Food for thought: if we want users to enjoy the v6 features when it's released without having them upgrade to modules we have two options:

  1. Release v6 without modules, then almost immediately release v7 with modules. This way, people can upgrade their v5 to v6 without having to change their dependency management tools. Others, can upgrade v5 to v7 if they were already on modules, or just use v7 with directly if starting from scratch.
  2. Release v6 with modules, but backport v6 features into v5.

I'd rather do option 1 than option 2.

However, those two options are completely different from the original option where we make v6 both module and non-module compatible by creating a v6 folder, which is still a very valid consideration.

I am not familiar with what the difference between v5 and v6, as my modules PR is just built on top of the current master branch, so I'll leave that discussion up to those who are more familiar 👍

@spenczar
Copy link
Contributor

I think what I'd like to do is release a version which uses a folder to do modules. Later, when the ecosystem has stabilized around modules, we can release a version that moves things back out of the folder, if we want.

https://github.com/marwan-at-work/helloworld looks great, thanks @marwan-at-work! I think we should also try to make a test case that consumes both v5 and v6 twirp in the same built binary. Maybe that should be an integration test case in the twirp repo, I'm not sure exactly.

@marwan-at-work
Copy link
Contributor Author

marwan-at-work commented Apr 17, 2019

I think what I'd like to do is release a version which uses a folder to do modules

@spenczar that sounds like a good plan. Let me know if you'd like me to re-open the PR at some point.

I think we should also try to make a test case that consumes both v5 and v6 twirp in the same built binary.

I imagine the built binary will have to be using Go Modules since Go Modules is capable of compiling two major versions of the same library in one binary.

However, Dep and its predecessors don't let you use two versions of the same library in one module. I think I see why you want to do type aliases in the root directory now.

Users can consume v5 and v6 at the same time, but the binary will have to lock twirp to V6 in the Dep manifest file. However, I don't think Dep would be happy forcing a library that uses v5 to just use v6 instead? I'd have to go back and check.

Definitely worth testing all of this, but those are my initial guesses.

@rynop
Copy link

rynop commented Jun 17, 2019

Doesn't Twrip v5 work today with go modules once #126 was merged?

I followed this dep->gomod migration guide, then updated my protoc to include paths=source_relative. Ex:

retool do build/bin/protoc -Ibuild/protoc/include -I. rpc/platform/platform.proto --go_out=paths=source_relative:. --twirp_out=.

Everything seems to be working for me, however given the discussion above I'm concerned that I'm missing something (it should not be working). Am I?

@spenczar
Copy link
Contributor

spenczar commented Nov 7, 2019

We're well into Go module support with all modern versions of Go at this point. Let's revive this. I think it's fair to expect that all clients and servers should understand go module semantics.

@Cyberax
Copy link

Cyberax commented Jan 8, 2020

Is anyone working on this? If not, I can take a stab.

@spenczar
Copy link
Contributor

@Cyberax I am not aware of anyone working actively on this. Please, take a shot!

alexplevako added a commit to alexplevako/twirp that referenced this issue May 2, 2020
alexplevako added a commit to alexplevako/twirp that referenced this issue May 2, 2020
alexplevako added a commit to alexplevako/twirp that referenced this issue May 2, 2020
@alexplevako alexplevako mentioned this issue May 2, 2020
alexplevako added a commit to alexplevako/twirp that referenced this issue May 2, 2020
alexplevako added a commit to alexplevako/twirp that referenced this issue May 2, 2020
alexplevako added a commit to alexplevako/twirp that referenced this issue May 2, 2020
@tv42
Copy link

tv42 commented Oct 2, 2020

For what it's worth, in a module-using project, just go run github.com/twitchtv/twirp/protoc-gen-twirp works fine, the library seems to work, etc. go.mod now has github.com/twitchtv/twirp v7.1.0+incompatible, so it's in backwards-compat mode, but it just worked.

The new protobuf API works fine, because they made the old one with a forwards-compat wrapper for it. staticcheck gives two warnings for every generated *.twirp.go, to nudge that upgrade:

foo.twirp.go:21:8: package github.com/golang/protobuf/jsonpb is deprecated: Use the "google.golang.org/protobuf/encoding/protojson" package instead.  (SA1019)
foo.twirp.go:22:8: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (SA1019)

Of course twirp having proper go.mod and a non-+incompatible release would be better.

@cristaloleg
Copy link

kindly ping :)

@3ventic
Copy link
Contributor

3ventic commented Dec 3, 2020

The main issue slowing this down is separate libraries providing Twirp hooks or common client options. With a direct move to go modules the import path changing means Go would consider the types to be different too (v8.ClientHooks vs. twirp.ClientHooks). Not all of these can easily be aliased.

The solution with least amount of work for Twirp's dependents would seem to be to break it once; separate the generator and the shared components/models used by the generated code into their own separate modules so new major releases of the generator could use the same types as the previous generator, unless breaking changes were introduced to those in conjunction.

@github-actions
Copy link

github-actions bot commented Feb 2, 2021

This issue is stale because it has been open 60 days with no activity. Remove stale label / comment or this will be closed in 5 days

@tv42
Copy link

tv42 commented Feb 2, 2021

Bad stalebot, features don't appear just because time passes.

@3ventic 3ventic added the pending Keeps issues from going stale label Mar 16, 2021
@tuananh
Copy link

tuananh commented Aug 5, 2022

pinging :) it's 2022. go is at v1.18 now

@marwan-at-work
Copy link
Contributor Author

I'd love to come back to this and fix it.

Go is at 1.19 and Modules has been the default for a few years now. And a lot of bug fixes have gone in to make Modules work well in terms of compatibility.

I think if we introduced Go Modules and created a github.com/github/twitchtv/v9 upgrade (Twirp hasn't been shy about major version upgrades, so going from v8 to v9 should not be a big deal), then we can finally close this issue and make Twirp a lot easier to work with.

I have a working branch here that does the following:

  1. Removes the vendor (no longer needed with go modules and the proxy.golang.org)
  2. Removes the _tools folder (now you can just run go get|run|install <tool>@<version> -- so there should be no need for extra tooling.
  3. Adds a new go.mod file with the path github.com/twitchtv/twirp/v9

Branch: https://github.com/marwan-at-work/twirp/tree/marwan/v9

I also created a repo that tests backwards compatibility which proves the following:

  1. A server that's running in v9 can be ping'd by a Client that's running in v8 without any issue
  2. The reverse of the above.
  3. v9 and v9 can talk to each other

Repo: https://github.com/marwan-at-work/twirptest

Are there any maintainers still around here? If so, I'd love to open the PR and get this shipped out barring any concerns.

Thanks!

@rhysh
Copy link
Contributor

rhysh commented Sep 19, 2022

Thanks @marwan-at-work . Yes, Twirp hasn't been shy about major version upgrades, and yes we'd do another when moving to modules. But what does the next major version upgrade after that look like?

Today, an application can run with v8 of the Twirp library, with code and instrumentation written in the v6 era and import clients generated with v7 of protoc-gen-twirp. These can all exchange twirp.Error values, ServerHooks values, and store/access information via the Context.

Presenting Twirp as a Go Module means the major version number becomes part of the package import path. That's a benefit in many ways for many libraries, but it'll take some planning to work out the implications for Twirp and how to make the most of it. I see that planning (and subsequent execution) as the bulk of the "support Go modules" work.

If Twirp v9 moves to a Go Module, can we ever have a v10? Does the move from non-module to v9 require not only a module update for app owners, but also regenerating their server stub and the stubs for all of the clients they import?

One of the neat things about modules is that different import paths for v9 and v10 mean they can coexist in a single program .. but do app owners who go that route leave themselves open to incompatible meanings of twirp.Error, because the Code method of github.com/twitchtv/twirp/v9.Error doesn't return a github.com/twitchtv/twirp/v10.ErrorCode? (Do we have the modules import each other and use type aliases for this?)

If moving a new module-based major version requires a bulk update, that's unfortunate. If it doesn't, but the penalty for getting the partial update wrong is that the app will behave in hard-to-understand ways, that's also unfortunate.

It seems similar to the work in https://go.dev/issue/53896 to make http/2 errors available whether they came from the golang.org/x/net/http2 package or from the version that net/http includes in h2_bundle.go. It's possible, but it takes specific clever design effort to solve. And in Twirp's case, we may not be able to enumerate all possible sibling packages.

@marwan-at-work
Copy link
Contributor Author

Today, an application can run with v8 of the Twirp library, with code and instrumentation written in the v6 era and import clients generated with v7 of protoc-gen-twirp. These can all exchange twirp.Error values, ServerHooks values, and store/access information via the Context.

In this scenario, the Go compiler will actually work, because twirp.Error is an interface, and as long as twirp6.Error and twirp7.Error are the same interface, then they will cast correctly. Feel free to see my code above and try it out.

As for ServerHooks, if you happen to upgrade the generated code but not the twirp library itself, then the compiler will force you to upgrade thanks to this line:

t.P(`const _ = `, t.pkgs["twirp"], `.TwirpPackageMinVersion_8_1_0`)

So we should be covered on those accounts.

Presenting Twirp as a Go Module means the major version number becomes part of the package import path. That's a benefit in many ways for many libraries, but it'll take some planning to work out the implications for Twirp and how to make the most of it. I see that planning (and subsequent execution) as the bulk of the "support Go modules" work.

To be honest, I've thought about this a lot. It should be okay. From every angle I've tackled this, chances are you don't even need to upgrade. Like I said, a v8 client can talk to a v9 server and so on.

But even if you wanted to upgrade, the only thing people need to do is upgrade their import paths, and that's okay because it's a breaking change and breaking changes require you to make changes. Furthermore, it's a small change because some tools and simple search/replace allow you to do that over an entire codebase.

If Twirp v9 moves to a Go Module, can we ever have a v10?

Yes you can. If you introduce a breaking change.

Does the move from non-module to v9 require not only a module update for app owners, but also regenerating their server stub and the stubs for all of the clients they import?

No, as you can see in my code above, the clients did not need to do anything and it just continued to work. Feel free to run the tests I wrote above.

To be honest, even if we found one edge case where something breaks that's okay, because at the end of the day it's a breaking change.

If moving a new module-based major version requires a bulk update, that's unfortunate. If it doesn't, but the penalty for getting the partial update wrong is that the app will behave in hard-to-understand ways, that's also unfortunate.

Agreed, but it's been many years and Modules is stable and we can run many tests (as I have) to prove that it should be okay.

Happy to chat some more or try and find more edge cases.

@rhysh
Copy link
Contributor

rhysh commented Oct 3, 2022

You've covered wire-protocol compatibility. Thank you. That's a critical form of compatibility, but it's not the only one that's important for the project.

Today, a project can import additional libraries that operate on twirp.Error values or which present instrumentation as twirp.ServerHooks. Some store values in Context, or operate on values stored in Context.

Today, a project can present a Twirp server interface, consume several Twirp client implementations, and use a single twirp package to interact with all of them. The clients may be generated asynchronously by the teams that own the relevant services.

To move forward with a conversion to Go Modules, we need to have a good understanding of how those use cases will change, and to make sure that the required changes aren't too disruptive to our users.

It sounds like the one option is to require an app owner to update their Twirp runtime library, regenerate their Twirp server stub, regenerate the Twirp client stubs they import, and rewrite the imports of any instrumentation libraries they use. Do I correctly understand the technical implications of your proposal?

My opinion is that's too disruptive to our users, and that we need a different solution.


Here's a straightforward incompatibility (and why I referenced https://go.dev/issue/53896), at the tip of the iceberg:

package main

import (
	t8 "github.com/twitchtv/twirp"
	t9 "github.com/twitchtv/twirp/v9"
)

var _ t9.Error = (t8.Error)(nil)

Adding that to ./compat.go at the root of https://github.com/marwan-at-work/twirptest leads to this build failure:

# marwan.io/twirptest
./compat.go:8:18: cannot use (t8.Error)(nil) (value of type "github.com/twitchtv/twirp".Error) as type "github.com/twitchtv/twirp/v9".Error in variable declaration:
	"github.com/twitchtv/twirp".Error does not implement "github.com/twitchtv/twirp/v9".Error (wrong type for Code method)
		have Code() "github.com/twitchtv/twirp".ErrorCode
		want Code() "github.com/twitchtv/twirp/v9".ErrorCode

Here's one that's more insidious: in ./main.go, this gives a Name of either "qwer" or "qwerHaberdasher" at runtime, depending on which version of twirp the file imports, with no corresponding build failure. Note that before this change, ./main.go did not reference any version of the Twirp runtime library.

// MakeHat implements twirp9.Haberdasher
func (*server) MakeHat(ctx context.Context, req *twirp9.Size) (*twirp9.Hat, error) {
	fmt.Println("GOT", req.Inches)
	name, _ := twirp.ServiceName((ctx))
	return &twirp9.Hat{
		Size:  123,
		Color: "abc",
		Name:  "qwer" + name,
	}, nil
}

@abhinav
Copy link

abhinav commented Jul 19, 2024

Hey all. It's a couple years later.
The ecosystem has fully migrated to Go modules, and has been generally happy with it.
What's your opinion on tagging a v9 with a qualified import path now?
I want to echo @marwan-at-work that risk of breaking changes from major version releases is a given, and accepted as part of semver compliance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending Keeps issues from going stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants