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

Adds dependency injection rule for protobuf-java runtime #15006

Merged
merged 10 commits into from
May 27, 2022

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Apr 5, 2022

No description provided.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
def __init__(self, resolve_name: str) -> None:
super().__init__(
f"The JVM resolve `{resolve_name}` does not contain a requirement for the protobuf-java "
"runtime. Since at least one Scala target type in this repository consumes a "
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Scala/JVM/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to make this change over in Scala land too, since Java can consume scala protobuf generated sources

@tdyas
Copy link
Contributor

tdyas commented Apr 5, 2022

Needs category label too on PR.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@@ -76,7 +76,7 @@ class MissingProtobufJavaRuntimeInResolveError(ValueError):
def __init__(self, resolve_name: str) -> None:
super().__init__(
f"The JVM resolve `{resolve_name}` does not contain a requirement for the protobuf-java "
"runtime. Since at least one Scala target type in this repository consumes a "
"runtime. Since at least one JVM target type in this repository consumes a "
"`protobuf_sources` target in this resolve, the resolve must contain a `jvm_artifact` "
"target for the protobuf-java` runtime.\n\n Please add the following `jvm_artifact` "
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing leading ` character on "protobuf-java"

`
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn
Copy link
Contributor Author

chrisjrn commented Apr 7, 2022

@tdyas I missed merging this; this would be a good candidate for the new find_jvm_artifacts_or_raise method, right?

@tdyas
Copy link
Contributor

tdyas commented Apr 7, 2022

@tdyas I missed merging this; this would be a good candidate for the new find_jvm_artifacts_or_raise method, right?

Yes. That helper is intended for this exact use case.

@chrisjrn
Copy link
Contributor Author

chrisjrn commented Apr 7, 2022 via email

jvm_artifact_targets=jvm_artifact_targets,
jvm=jvm,
)
return ProtobufJavaRuntimeForResolve(next(iter(addresses)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I ended up switching types similar to ProtobufJavaRuntimeForResolve to take a collection of Address so as to not rely on assuming find_jvm_artifacts_or_raise will return one Address per coordinate provided. See #15110.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checks out

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn enabled auto-merge (squash) April 14, 2022 16:48
@Eric-Arellano Eric-Arellano removed this from the 2.11.x milestone Apr 25, 2022
@Eric-Arellano
Copy link
Contributor

I removed this from 2.11 mile stone because we decided JVM codegen is not ready to publicize

@stuhood
Copy link
Member

stuhood commented May 26, 2022

@chrisjrn : It would be useful to freshen this one up and land it: I've added it as a TODO in an example that a user opened over here: pantsbuild/example-jvm#18

@stuhood stuhood added the backend: JVM JVM backend-related issues label May 26, 2022
# Conflicts:
#	src/python/pants/backend/codegen/protobuf/scala/dependency_inference.py
#	src/python/pants/jvm/target_types.py
@chrisjrn
Copy link
Contributor Author

@stuhood I've fixed the conflicts, hopefully will merge before too long.

Christopher Neugebauer added 3 commits May 26, 2022 13:10
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn merged commit 47b9f4f into pantsbuild:main May 27, 2022
@chrisjrn chrisjrn deleted the chrisjrn/inject-java-pb branch May 31, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: JVM JVM backend-related issues category:new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants