Skip to content

jvm: fix ambiguous classpath provider for protobuf targets consumed by both Java and Scala codegen - #23619

Open
robertpi wants to merge 2 commits into
pantsbuild:mainfrom
robertpi:fix/protobuf-jvm-classpath-ambiguity
Open

jvm: fix ambiguous classpath provider for protobuf targets consumed by both Java and Scala codegen#23619
robertpi wants to merge 2 commits into
pantsbuild:mainfrom
robertpi:fix/protobuf-jvm-classpath-ambiguity

Conversation

@robertpi

Copy link
Copy Markdown
Contributor

Problem

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) finds both CompileJavaSourceRequest and CompileScalaSourceRequest compatible with the same protobuf_sources component and raises ClasspathSourceAmbiguity. This happens because both backends declare GenerateSourcesRequest.input = ProtobufSourceField while 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

  • 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).

Test plan

  • 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
  • Regression check: src/python/pants/backend/openapi/codegen/{java,python}/*_test.py
  • ./pants fmt lint on all changed files

…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 jgranstrom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@robertpi

Copy link
Copy Markdown
Contributor Author

This is a good point, and I agree the ambiguity really lives on the consumer side — nothing stops a protobuf_sources target from legitimately producing both Java and Scala output for different consumers.

I want to be upfront that if the preferred_codegen approach lands, it makes this PR's fields unnecessary for the bug we're actually fixing here — ambiguity would just resolve automatically, no explicit opt-out needed. This PR isn't a complement to that, it's a narrower stopgap for the same problem. The only thing that would still be distinct is skipping codegen entirely for a language you know you'll never use (avoiding a wasted protoc invocation), but that's a minor secondary benefit, not the motivation here.

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.

@tdyas

tdyas commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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 BUILD file(s). Moreover, skip fields do not solve the issue of allowing both Java and Scala codegen to coexist with each other.

We should probably prefer an explicit declarative approach. But instead of only setting a preferred_codegen in Pants rule logic, we could add an explicit codegen_type field to Protobuf targets and then use the Pants parametrize mechanism to create separate targets for each kind of codegen. For example, imagine this protobuf_sources target:

protobuf_sources(
  name="pb",
  codegen_type=parametrize("scalapb", "java"),
)
  1. With this :pb target using parametrize, both kinds of codegen have addresses: the java codegen has address :pb@codegen_type=java and the ScalaPB codegen has address :pb@codegen_type=scalapb.

  2. The dependency inference logic in Java and Scala rule logic could then be modified to infer the dependency on the applicable target with the correct codegen_type. Some form of preferred_codegen in rule logic might be necessary to help facilitate that.

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.

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.

3 participants