-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix(bazel): generate java assembly for type-only #169
Conversation
didn't have Blake in owners before, now we do
deps = [":{{name}}_java_proto"], | ||
# Open Source Packages | ||
java_gapic_assembly_gradle_pkg( | ||
name = "{{type_only_assembly_name}}-java", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is consistent with the rest of the file so I think it's good in the context of the PR.
However, do we want to keep the it consistent with the names of regular packages? I don't see anything wrong with google-cloud-alloydb-connectors-v1alpha-java
for types only packages. It may not matter for Bazel bot but the benefit I see is that
- Developers do not have to be aware of the differences between type-only and a regular package.
- It's a more predictable name when we want to test it locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type only packages don't always have a version in them, sometimes they are just google/shopping/type
. In this case it would like google-shopping-type-java
which isn't bad either.
Why don't we just use this name type_only_assembly_name
variable here, and I will change the code in a separate PR to update the format of its value. It will impact other languages as well so best to keep in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agree that it should be a different PR even if we want to change it.
java_gapic_assembly_gradle_pkg( | ||
name = "{{type_only_assembly_name}}-java", | ||
deps = [ | ||
":{{name}}_proto", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably my own lack of understanding, why do we need this for Java but not for other languages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea tbh. I just saw it in the same target for GAPIC libraries and copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps because the protos are included in the generated code under the proto-
directory?
For type-only packages (those that don't have any proto
service
definitions), stop generatejava_grpc_library
(no code would be generated anyways) and addjava_gapic_assembly_gradle_pkg
in order to enable publication of the Java classes.Note: This is implementation is different from the inspiration CL in a few ways:
bazel-bot
only looks for tarball outputs)proto_library
target in the generated output (the inspiration CL did not)cc: @blakeli0
Part of b/281092541.