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

Allow for codegen targets to be used directly by JVM compiler requests #14751

Merged
merged 12 commits into from
Mar 11, 2022

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Mar 9, 2022

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.

Christopher Neugebauer added 4 commits March 9, 2022 13:44
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@chrisjrn chrisjrn marked this pull request as draft March 9, 2022 23:02
@stuhood
Copy link
Member

stuhood commented Mar 9, 2022

  • Handling ambiguity around multiple generators (i.e. protobufs for java and scala)
  • Allowing protobuf_sources etc to include/exclude languages

Both of these should likely be handled in the same way: via the generators field.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Seems reasonable! Thanks.

src/python/pants/backend/codegen/protobuf/java/register.py Outdated Show resolved Hide resolved
src/python/pants/backend/codegen/protobuf/target_types.py Outdated Show resolved Hide resolved
src/python/pants/jvm/compile.py Outdated Show resolved Hide resolved
src/python/pants/jvm/compile.py Outdated Show resolved Hide resolved
src/python/pants/jvm/compile.py Outdated Show resolved Hide resolved
src/python/pants/jvm/compile.py Outdated Show resolved Hide resolved
Christopher Neugebauer added 4 commits March 10, 2022 12:38
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
…en input codegen type

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@chrisjrn chrisjrn changed the title WIP JVM protobuf compilation Allow for codegen targets to be used directly by JVM compiler requests Mar 11, 2022
@chrisjrn chrisjrn marked this pull request as ready for review March 11, 2022 00:20
@chrisjrn chrisjrn requested a review from stuhood March 11, 2022 00:20
if not component.representative.has_field(input):
continue
if len(request_types) > 1:
raise ClasspathSourceAmbiguity(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

at the moment, this will result in a late failure, and only if a user attempts to use a source file with conflicting implementations as a dependency. I could re-write to fail fast, but the code will eventually need to fail here once #14041 is in place.

Copy link
Member

Choose a reason for hiding this comment

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

It's still not clear to me why this matching isn't happening inside of ClasspathEntryRequest.classify_impl. If it did, both 1) only matching the representative, 2) late erroring... would be handled by the existing code below, right? Because you'd match multiple impls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that all checks out. I'll make that refactor before landing this.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Looks good. But expanding the test and trying to reuse classify_impl (which should make the testing more uniform as well?) would be good before landing.

@@ -47,6 +54,38 @@ class _ClasspathEntryRequestClassification(Enum):
INCOMPATIBLE = auto()


@dataclass(frozen=True)
class JVMRequestTypes:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: a more meaningful name would be good.

if not component.representative.has_field(input):
continue
if len(request_types) > 1:
raise ClasspathSourceAmbiguity(
Copy link
Member

Choose a reason for hiding this comment

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

It's still not clear to me why this matching isn't happening inside of ClasspathEntryRequest.classify_impl. If it did, both 1) only matching the representative, 2) late erroring... would be handled by the existing code below, right? Because you'd match multiple impls.

@@ -193,7 +194,7 @@ def classify(
members: Sequence[type[ClasspathEntryRequest]],
) -> tuple[type[ClasspathEntryRequest], type[ClasspathEntryRequest] | None]:
req = ClasspathEntryRequest.for_targets(
UnionMembership({ClasspathEntryRequest: members}),
JVMRequestTypes(tuple(members), FrozenDict()),
Copy link
Member

Choose a reason for hiding this comment

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

This test should probably be expanded to also test some codegen impls.

Christopher Neugebauer added 2 commits March 11, 2022 11:28
…. Much better.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@chrisjrn chrisjrn force-pushed the chrisjrn/14730-real-jvm-codegens branch from 7373631 to 39890bd Compare March 11, 2022 20:12
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@chrisjrn chrisjrn requested a review from stuhood March 11, 2022 20:29
@chrisjrn
Copy link
Contributor Author

@stuhood Submitting for re-review just in case

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
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