-
Notifications
You must be signed in to change notification settings - Fork 132
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
Rename .gen -> gen for go 1.16 compatibility #1057
base: master
Are you sure you want to change the base?
Conversation
Since we are changing
|
Yeah, we could take this opportunity to rename. I think it's unavoidable that this is going to be a breaking change in some way - at the very least, go 1.16 users have absolutely no choice, they'll need to upgrade the library + fix any imports that reference .gen, or it simply won't compile. At best we could allow 1.15 people to put off the upgrade for a while. So since it's breaking:
I kinda feel like "api" might be a bit too vague / sorta implies "programmers should look at and use this" too much. Maybe "rpc"? I'm completely agreed on "go" -> "thrift" though, I'll do that now. |
c725d0d
to
638c36d
Compare
Pull Request Test Coverage Report for Build c5395e85-c3b0-45a6-afef-65e97d78c071
💛 - Coveralls |
|
RPC is the one and only purpose of that generated code, yeah - it's very specific but I don't see it as inaccurate. Though it doesn't contain e.g. ready-for-use RPC clients, so maybe it's not "all of RPC" in the same way it's not "all of API". But I think gen is mostly fine. For a few reasons we treat that folder specially because it's all generated code. |
One way to keep things backward compatible for Go 1.15 users is to keep the Users who value backward compatibility more than upgrading Go will have more time to upgrade their code this way. |
I started figuring out what exactly this would entail, earlier, and... there are no good options. Much of it is in the root comment -> will become the commit message, but to elaborate a bit:
All of which is a lot of work and still leads to a breaking change for people who just jump to go 1.16, which must be dealt with prior to 1.16. That 1.16 breaking change is completely unavoidable AFAICT. Since "you must migrate before 1.16" is necessary no matter what, I think we should just embrace the breakage. That way:
A much more complete "fix" for this is to not expose generated or internal code at all, which is pretty much always a good idea, and would've prevented this from happening. Presumably, for e.g. To get to that point though we do still need to do the .gen -> gen change, which ^ is unavoidably breaking for 1.16 users. Whether the "hide generated code" changes are done as a breaking change or via new APIs to allow gradual migration is unrelated, we can do either. |
I have been thinking about this for a bit in a context of migration to GRPC that we will need to do eventually. It basically boils down to the similar problem, as all generated code exposed as I think we should go in the direction of not exposing generated code at all. Even though it might require bigger initial investment up front - future changes like this will not that painful. |
That's going to be harder with protobuf, especially since they made a decision to make unambiguous import paths mandatory. So basically you'll have to expose some sort of API package, most probably from the cadence project itself. I often see Go submodules being used for this purpose. BTW Go 1.16.1 will (at least partially) revert the leading dot change. |
Yeah, that is the idea. |
I meant an API package with generated proto code in it, but I guess a high level API would work as well (although it's more work and closes down the possibility of lower level integrations. |
Personally I'd much prefer all of these to be internal. Generated code is a thing we control, it doesn't control us - it's not hard to add a "rewrite import X in proto Y to Z" or "after proto-gen, It doesn't matter what proto requires, or how we get .proto files copied around, or what source files we use from where. It does not have to be exposed. Absolutely nothing requires that. So we shouldn't expose it. |
Client-side counterpart to cadence-workflow/cadence/issues/3987 . This essentially has to be merged and released before the server can be upgraded.
In a nutshell:
go
hasn't liked.gen
since some time in 2018, but 1.16 was just released and now it's enforced.Doing anything with 1.16 with modules enabled produces errors like:
So this PR:
go.uber.org/cadence/{.gen,gen}
go test
with both go 1.15 and go 1.16, and make sure it's stable (it is, minus a very weird shifting import in generated code. goimports would fix that.)go mod
stuff. This failed earlier, works now.Unfortunately this is a breaking change.
.gen
was exposed directly, and e.g. github.com/uber/cadence imports it. Even if we ignore the server, e.g.workflow/error.go
exposes.gen/go/shared.TimeoutType
as an argument toNewTimeoutError
, so anyone using that function would necessarily be importing.gen
as well.We could make a 1.16+ version of the file with
gen
and use build tags to select which one... but it's still a breaking change for any 1.16 user, and they're inevitably coming. If we change all versions togen
, we can at least let people upgrade their code before upgrading Go.