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

Rename .gen -> gen for go 1.16 compatibility #1057

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Rename .gen -> gen for go 1.16 compatibility #1057

wants to merge 2 commits into from

Conversation

Groxx
Copy link
Contributor

@Groxx Groxx commented Feb 17, 2021

Client-side counterpart to uber/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:

go.uber.org/cadence/workflow imports
go.uber.org/cadence/.gen/go/shared: malformed import path "go.uber.org/cadence/.gen/go/shared": leading dot in path element

So this PR:

  • Pins tool versions via tools.go + go.mod, as builds did not appear stable before (likely transitive thrift dependencies updating, breaking generated code)
  • Changes the generated source dirs from ".gen" to "gen"
  • Changes licensegen to look at the new path
  • Find-and-replaces all go.uber.org/cadence/{.gen,gen}
  • To test:
    • In this repo: clean and rebuild / thriftc / 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.)
    • In a test repo that imports this repo: do the same. Build, test, 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 to NewTimeoutError, 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 to gen, we can at least let people upgrade their code before upgrading Go.

@CLAassistant
Copy link

CLAassistant commented Feb 17, 2021

CLA assistant check
All committers have signed the CLA.

@vytautas-karpavicius
Copy link
Contributor

Since we are changing .gen already, a couple of considerations:

  • maybe choose api name instead
  • maybe rename go to thrift, that way we would get gen/thrift (or api/thrift). Which would be more consistent when we introduce proto api later (e.g. gen/proto or api/proto). I imagine there will be a transition period when we will support both protocols.

@Groxx
Copy link
Contributor Author

Groxx commented Feb 19, 2021

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:

  • 1.15 and earlier Go users can stay on the older versions. no changes means no breaking changes.
  • 1.16 and newer Go users, or anyone upgrading, must adjust to the new imports.

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.

@Groxx Groxx force-pushed the sixteen branch 2 times, most recently from c725d0d to 638c36d Compare February 19, 2021 03:11
@Groxx Groxx marked this pull request as ready for review February 19, 2021 03:13
@coveralls
Copy link

coveralls commented Feb 19, 2021

Pull Request Test Coverage Report for Build c5395e85-c3b0-45a6-afef-65e97d78c071

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 74.855%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mocks/Client.go 0 1 0.0%
Totals Coverage Status
Change from base Build 480e6fe7-0185-4b1d-8ebe-95c7403fa5b7: 0.05%
Covered Lines: 9693
Relevant Lines: 12949

💛 - Coveralls

@vytautas-karpavicius
Copy link
Contributor

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:

  • 1.15 and earlier Go users can stay on the older versions. no changes means no breaking changes.
  • 1.16 and newer Go users, or anyone upgrading, must adjust to the new imports.

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.

rpc is quite the opposite though - very specific. I would expect it contain source code for making rpc calls or maybe some helpers for that. If you don't feel comfortable with api, we can just leave gen.

@Groxx
Copy link
Contributor Author

Groxx commented Feb 19, 2021

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.

@sagikazarmark
Copy link
Contributor

One way to keep things backward compatible for Go 1.15 users is to keep the .gen directory and optionally add build tags in front of each file to disable building on Go 1.16.

Users who value backward compatibility more than upgrading Go will have more time to upgrade their code this way.

@Groxx
Copy link
Contributor Author

Groxx commented Feb 25, 2021

One way to keep things backward compatible for Go 1.15 users is to keep the .gen directory and optionally add build tags in front of each file to disable building on Go 1.16.

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:

  • If we do the build tags, we need to split ALL code that exposes .gen publicly, which includes transitively (via type x = internal.x references, as fields in other types, etc). And any code using those types internally. That's a fair amount of code, it may actually be easier to make a complete fork of the repo.
  • Even if we do that, 1.16 people can't upgrade in a non-breaking way because of things like workflow.NewTimeoutError. So you have to upgrade Go + make code changes + possibly cadence-using libraries all at the same time. That's quite a lot for a normally "safe" upgrade like upgrading Go.
  • So we need to put .gen in 1.15 and add gen in 1.15 or users can't upgrade gradually no matter what. Which means copying APIs to new names (e.g. NewTimeoutErrorV2) that use only gen so they can upgrade gradually, plus build tags to lock out .gen for 1.16.

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:

  • if you stay on 1.15: stay on the older client, and nothing changes. This is always an option.
  • if you upgrade to 1.16, you get a compile-time error and have to stop upgrading.
  • in 1.15, you can upgrade to the new client version, fix the build, and it'll work. You can try it separately from 1.16. Upgrading to 1.16 takes no work after that.
  • ^ libraries using 1.15 can do the same, so users of the client+lib will have to wait for them and upgrade together. This is also unavoidable for 1.16 users, this only forces it to happen to 1.15 users who want to upgrade their client. No matter what this all has to happen at some point.

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. workflow.NewTimeoutError, that would mean making similar types in that package, which would also be more ergonomic to use: workflow.NewTimeoutError(workflow.TimeoutType, ...) instead of workflow.NewTimeoutError(.gen/shared.TimeoutType, ...).

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.

@vytautas-karpavicius
Copy link
Contributor

vytautas-karpavicius commented Mar 4, 2021

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 go.uber.org/cadence/.gen/go/... will need to be updated AGAIN on the user side. Meaning yet another migration to go.uber.org/cadence/gen/proto/...(or something else).

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.

@sagikazarmark
Copy link
Contributor

I think we should go in the direction of not exposing generated code at all.

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.

@vytautas-karpavicius
Copy link
Contributor

So basically you'll have to expose some sort of API package, most probably from the cadence project itself.

Yeah, that is the idea.

@sagikazarmark
Copy link
Contributor

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.

@Groxx
Copy link
Contributor Author

Groxx commented Mar 5, 2021

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, gofmt -w -r ..." rewrite step to the build, and then we can do literally anything we like.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants