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

[internal] refactor scrooge codegen to allow for multiple languages #14030

Merged
merged 3 commits into from
Dec 30, 2021

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Dec 30, 2021

The Scrooge Thrift generator supports multiple languages. Refactor the Scrooge backend so it is structured in a similar manner to the Apache Thrift where each language is its own backend. This will open the way for introducing the Java and Android Scrooge backends.

Tom Dyas added 2 commits December 30, 2021 05:38
[ci skip-rust]
@Eric-Arellano
Copy link
Contributor

How will this interact with Apache Thrift for Java? Users must opt-in to whichever generators they want, so it's easy for them to choose between Apache Thrift vs. Scrooge.

But does it ever make sense to generate both? I wonder if they use the same file name scheme, such that it would cause a merge conflict with MergeDigests.

@tdyas
Copy link
Contributor Author

tdyas commented Dec 30, 2021

How will this interact with Apache Thrift for Java? Users must opt-in to whichever generators they want, so it's easy for them to choose between Apache Thrift vs. Scrooge.

But does it ever make sense to generate both? I wonder if they use the same file name scheme, such that it would cause a merge conflict with MergeDigests.

The Scrooge Java code generator actually aims to replicate what the Apache Thrift Java generator does. Users should never activate them both at the same time.

@Eric-Arellano
Copy link
Contributor

Users should never activate them both at the same time.

Cool. I wonder how we could reduce the risk users do this...Right now, you'll get a confusing engine error about MergeDigests failing.

Docs should probably mention the assumption.

Maybe we introduce the concept of mutually exclusive backends? Our generic BuildConfiguration will error with a good message if you try activating both. Wdyt?

A quick-and-dirty solution would be for each backend to use ThriftSourceTarget.class_has_field() to see if plugin fields are activated from the other backend.

(This discussion isn't blocking, but probably worth at least figuring out our direction.)

Comment on lines 13 to +15
"src/python/pants/backend/experimental/codegen/thrift/apache/java",
"src/python/pants/backend/experimental/codegen/thrift/apache/python",
"src/python/pants/backend/experimental/codegen/thrift/scrooge",
"src/python/pants/backend/experimental/codegen/thrift/scrooge/scala",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how you organize these backends :)

@tdyas tdyas merged commit 9381e3e into pantsbuild:main Dec 30, 2021
@tdyas tdyas deleted the scrooge_multi_lang_refactor branch December 30, 2021 21:55
@jsirois
Copy link
Contributor

jsirois commented Dec 30, 2021

Users should never activate them both at the same time.

It's worth noting that the acid test for the engine experiment in 2015 was in fact supporting both at the same time in one codebase and being able to select which was desired per target. That test was driven by real Twitter use / needs at one point in time. It would be great to be able to support this sort of tortured codebase eventually, maybe.

@Eric-Arellano
Copy link
Contributor

It's worth noting that the acid test for the engine experiment in 2015 was in fact supporting both at the same time in one codebase and being able to select which was desired per target. That test was driven by real Twitter use / needs at one point in time. It would be great to be able to support this sort of tortured codebase eventually, maybe.

Good to know! It would be easy to support. Register a plugin field on thrift_source for each backend to toggle it per-target. If disabled, simply return EMPTY_DIGEST in the codegen rule. So, even though both backends would run, there'd be no merge conflict.

@tdyas
Copy link
Contributor Author

tdyas commented Dec 31, 2021

Wrote up the discussion at #14041.

@stuhood
Copy link
Member

stuhood commented Jan 3, 2022

Users should never activate them both at the same time.

It's worth noting that the acid test for the engine experiment in 2015 was in fact supporting both at the same time in one codebase and being able to select which was desired per target. That test was driven by real Twitter use / needs at one point in time. It would be great to be able to support this sort of tortured codebase eventually, maybe.

And that eventually evolved into "variants", which became "multiple Params in a subgraph" (see #13882 (comment)). So I do think that the path specified by labeled deps / #13882 is likely to be viable.

@stuhood stuhood mentioned this pull request Jan 8, 2022
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.

4 participants