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/protoc-gen-twirp@v5.6.0

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]

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 (#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 👍

[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 #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!