jvm: fix ambiguous classpath provider for protobuf targets consumed by both Java and Scala codegen - #23619
Conversation
…y both Java and Scala codegen
When both the Java and Scala protobuf codegen backends (`pants.backend.codegen.protobuf.java`
and `pants.backend.codegen.protobuf.scala`) are active, and a `java_sources` or `scala_sources`
target directly depends on a `protobuf_sources`/`protobuf_source` target, JVM classpath
resolution (`ClasspathEntryRequestFactory`) found both `CompileJavaSourceRequest` and
`CompileScalaSourceRequest` compatible with the same `protobuf_sources` component and raised
`ClasspathSourceAmbiguity`, since both backends declare `GenerateSourcesRequest.input =
ProtobufSourceField` while producing different JVM-compilable outputs.
Add `GenerateSourcesRequest.skip_field`, an optional `BoolField` a codegen implementation can
declare to let a target opt out of that implementation. `ClasspathEntryRequestFactory` now
tracks each `ClasspathEntryRequest`'s associated `GenerateSourcesRequest` subclasses (rather
than just their input `SourcesField`) so `classify_impl` can consult `skip_field` per-target
before treating a codegen implementation as compatible.
Wire this up in the protobuf Java and Scala backends via new `skip_java`/`skip_scala` fields on
`protobuf_source`/`protobuf_sources`, so a target whose generated protobuf code should compile
as only one JVM language can declare e.g. `protobuf_sources(skip_scala=True)`.
Verified with `PY=python3.14 ./pants test` on:
- src/python/pants/jvm/compile_test.py
- src/python/pants/backend/codegen/protobuf/java/rules_integration_test.py
- src/python/pants/backend/codegen/protobuf/scala/rules_integration_test.py
- src/python/pants/engine/target_test.py
- src/python/pants/backend/openapi/codegen/{java,python}/*_test.py (regression check)
and `PY=python3.14 ./pants fmt lint` on all changed files.
`FrozenDict` is invariant in its value type parameter, so the `generators`/ `ambiguous_generators` dict literals (built from tuples of concrete `GenerateSourcesRequest` subclasses) didn't match `classify`'s declared parameter type `FrozenDict[type[ClasspathEntryRequest], tuple[type[GenerateSourcesRequest], ...]]`. Annotate the two local variables explicitly so mypy widens the tuple element type accordingly.
jgranstrom
left a comment
There was a problem hiding this comment.
I'm wondering if the producer side skip is the right tool here. Since it would not allow to have both scala and java protobuf codegen for a protobuf_source, while this is something otherwise supported cross-language.
I think the core problem is that on the consumer side we're not able to select a source where there's multiple options. It surfaces on JVM where we can have multiple generators that are valid for compile, for example with scala both java and scala generators are valid. And it bails early on ambiguity where it would be able to actually select one.
There could be a path here to fix this on the consumer side which would also allow having both of these be used together, and without having to skip on producer side.
In jvm/compile.py we could pass a requester ClasspathEntryRequest to for_targets in classpath_dependency_requests like requester=type(request.request), telling us who is compiling. ClasspathEntryRequest could have a preferred_codegen ClassVar so that we would have a known ordered tuple of preferred codegen source field types per language. For example, preferred_codegen = (ScalaSourceField, JavaSourceField) in CompileScalaSourceRequest. This could be used to select the first compatible one instead of failing on ambiguity there. We already build the impls_by_source which could be carried over so that it can select the first preferred one that is compatible. That resolves the ambiguity error for this case there. With an empty default on preferred_codegen, anything that doesn't declare one falls through to the same ambiguity error as today, so it's additive.
The other place it would need to be handled is in graph.py hydrate_sources where we do have the for_sources_types already, but don't honor any ordering of it when there's multiple options. Essentially when relevant_generate_request_types is > 1 but they resolve unambiguously to each for_sources_types, we could select the first one instead of failing on ambiguity. This could be gated on the request with a prefer_first_matching_source_type which JVM could set here, so that it's opt-in.
I haven't tested this, but I think this would resolve this on the more appropriate layer, and I also think the second part would be needed here anyways since skip doesn't filter out the codegen hydration. A test that compiles scala using protobuf with both backends enabled would be needed to verify that. But the first part would also enable selecting a preferred codegen for the requesting compiler when there's multiple options, which in JVM there legitimately can be.
@tdyas do you have any input here?
|
This is a good point, and I agree the ambiguity really lives on the consumer side — nothing stops a I want to be upfront that if the Given that, I'm fine with this PR being superseded rather than merged if you'd rather go straight to the consumer-side fix — happy to close it in favor of that, or leave it as a stopgap until that lands, whichever you'd prefer. |
|
I agree with @jgranstrom that a solution adding "skip" fields is probably not the right approach for ambiguity issue. Anther reason why it is not the right approach: the "skip" fields only exclude codegen backends such that what codegen is enabled depends on what backends the user has configured for the project. If the backends change, the set of potential codegen then changes without any changes being made to the We should probably prefer an explicit declarative approach. But instead of only setting a
I recommend writing a design document first before we dive into specific solutions We need to think through the developer experience and how it impacts the Pants codebase. |
Problem
When both the Java and Scala protobuf codegen backends (
pants.backend.codegen.protobuf.javaandpants.backend.codegen.protobuf.scala) are active, and ajava_sourcesorscala_sourcestarget directly depends on aprotobuf_sources/protobuf_sourcetarget, JVM classpath resolution (ClasspathEntryRequestFactory) finds bothCompileJavaSourceRequestandCompileScalaSourceRequestcompatible with the sameprotobuf_sourcescomponent and raisesClasspathSourceAmbiguity. This happens because both backends declareGenerateSourcesRequest.input = ProtobufSourceFieldwhile producing different JVM-compilable outputs, and the classpath resolution logic had no way to disambiguate which language the generated code should be compiled as.Fix
GenerateSourcesRequest.skip_field, an optionalBoolFielda codegen implementation can declare to let a target opt out of that implementation.ClasspathEntryRequestFactorynow tracks eachClasspathEntryRequest's associatedGenerateSourcesRequestsubclasses (rather than just their inputSourcesField), soclassify_implcan consultskip_fieldper-target before treating a codegen implementation as compatible.skip_java/skip_scalafields onprotobuf_source/protobuf_sources, so a target whose generated protobuf code should compile as only one JVM language can declare e.g.protobuf_sources(skip_scala=True).Test plan
src/python/pants/jvm/compile_test.pysrc/python/pants/backend/codegen/protobuf/java/rules_integration_test.pysrc/python/pants/backend/codegen/protobuf/scala/rules_integration_test.pysrc/python/pants/engine/target_test.pysrc/python/pants/backend/openapi/codegen/{java,python}/*_test.py./pants fmt linton all changed files