Support Go Modules

[marwan-at-work]

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: https://github.com/twitchtv/twirp/issues/140#issuecomment-452401108

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]

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]

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]

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]

@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]

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]

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]

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 (https://github.com/twitchtv/twirp/pull/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]

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 :+1:

[spenczar]

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]

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]

Doesn't Twrip v5 work today with go modules once https://github.com/twitchtv/twirp/pull/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]

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]

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

[spenczar]

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

[tv42]

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]

kindly ping :)

[3ventic]

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[bot]]

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]

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

[tuananh]

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

[marwan-at-work]

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]

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]

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: https://github.com/twitchtv/twirp/blob/bc0716559cbfc3646f09a3c1e416ce0bd1ea2c95/protoc-gen-twirp/generator.go#L257

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]

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
}