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

consider ways to configure (conflicting) codegen implementations #14041

Closed
tdyas opened this issue Dec 31, 2021 · 7 comments
Closed

consider ways to configure (conflicting) codegen implementations #14041

tdyas opened this issue Dec 31, 2021 · 7 comments

Comments

@tdyas
Copy link
Contributor

tdyas commented Dec 31, 2021

In #14030, the point came up that certain codegen implementation might conflict with one another if activated at the same time. In the case of that PR, the conflicting implementations would be the Apache Thrift and Scrooge Thrift code generators for Java.

Ideas for how to configure:

  1. Have specific backends be aware of it each other and error if the other is configured. This might lead to circular dependency issues.
  2. Add skip_GEN_NAME fields to the applicable target types (e.g., thrift_sources) and have the codegen backends not run if their specific skip field is set. This is brittle to new codegen backends being enabled since it might require also updating lots of targets to have a skip field for the new backend.
  3. Add a generators field to the applicable target types (e.g., thrift_sources) that takes a list of strings that are generator names. Only those specific generators will be enabled for that target. This is probably more scalable than (2) since its semantics would not change if an additional codegen backend was enabled. Plus it will fail if a configured generator has its codegen backend disabled.
  4. Use labelled dependencies to allow the usage sites to configure which generator to use via a dependency label. For example, the dependency could be dependencies=["src/thrift/foo!generator=scrooge_java"]. This can be duplicative if every usage site had to declare the generator label versus just putting fields on the thrift_sources. Moreover, it would not work with dependency inference unless there were a way to specify which generator to use which leads back to (2) and (3). It could be useful as an override syntax though.
  5. (Re-?)introduce language-specific codegen target types. So there would still be protobuf_sources to configure Protobuf-specific options. But there would also be python_protobuf_sources which would depend on a specific protobuf_sources target via a source_target attribute (which would be inferred). This is where the python_source_root field would live. That could have a generator attribute to select which codegen backend to use where that makes sense (i.e., a java_protobuf_sources could select from Apache Thrift or Scrooge). This would allow having multiple LANG_protobuf_sources targets if there were a need to vary options. Consuming targets could then override dependency inference if there were any ambiguity in the usual way.
@tdyas
Copy link
Contributor Author

tdyas commented Dec 31, 2021

Related point: We currently inject dependencies for codegen regardless of whether the generator is used or not. That behavior will need to match whatever is chosen for codegen generally. Alternatively, consider whether GeneratedSources should carry the runtime dependencies as an additional field so there is no need to use InjectDependenciesRequest as it is currently being used.

@tdyas
Copy link
Contributor Author

tdyas commented Jan 13, 2022

Added an additional point (5) regarding language-specific codegen target types.

@tdyas
Copy link
Contributor Author

tdyas commented Jan 14, 2022

Multiple generators enabled is causing an error for me in practice:

18:44:40.46 [ERROR] 1 Exception encountered:

  AmbiguousCodegenImplementationsException: Multiple of the registered code generators can generate PythonSourceField from ProtobufSourceField. It is ambiguous which implementation to use.

Possible implementations:

  * GenerateJavaFromProtobufRequest
  * GeneratePythonFromProtobufRequest

I have both java and python protobuf codgen backends enabled in my test repo, but there is no Java code written yet. This failure occurs when just testing the Python code with ./pants_from_sources test src/python/protobuf_example::

@Eric-Arellano
Copy link
Contributor

Hmmm, that sounds like a bug. GenerateJavaFromProtobufRequest shouldn't be claiming to generate PythonSource. I'll investigate right now.

@tdyas
Copy link
Contributor Author

tdyas commented Jan 14, 2022

class GenerateJavaFromProtobufRequest(GenerateSourcesRequest):
    input = ProtobufSourceField
    output = PythonSourceField

my bug ... ugh

@tdyas
Copy link
Contributor Author

tdyas commented Jan 26, 2022

This issue impacts #14258 which describes supporting compiled languages in codegen.

chrisjrn pushed a commit that referenced this issue Mar 11, 2022
#14751)

This amends `ClasspathEntryRequest.for_targets` to accept a codegen source request target and output a `ClasspathEntryRequest` that corresponds to a relevant JVM compilation request.

With this approach, it seems that any `GenerateSourcesRequest` with `output` that is compatible with a `ClasspathEntryRequest` will Just Work™. To demonstrate this in action, there's backend registration support for the Java and Scala protobuf generators included in this PR too. 

The main current limitation is when multiple language backends are enabled for a given codegen source type. A solution for this would address #14041, and realistically needs to take multiple JVM languages into account.
@stuhood
Copy link
Sponsor Member

stuhood commented Mar 30, 2022

#14694 is a dupe of this one, but was opened after consensus was reached on adding a generator(s) field. Closing in favor of that one.

@stuhood stuhood closed this as completed Mar 30, 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

No branches or pull requests

3 participants