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

feat: Use gapic-generator-java jar in the client library generation process #918

Merged
merged 87 commits into from Jan 6, 2023

Conversation

chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Feb 2, 2022

Note(Updated on 12/29/2022): The description is now almost re-written since a lot of the implementation details changed. Thanks to @chanseokoh who created this PR originally and enlightened me the new approach.

The main goal of this PR is to use the gapic-generator-java jar in the client library generation process, with both released jars and local SNAPSHOT jars. Hence, Bazel will not be used for loading dependencies and building the generator from source going forward. Instead, Maven is now used for building a gapic-generator-jar first, and the jar is passed into the Bazel plugin to generate the client libraries. This PR also removed as much Bazel files/configs as possible.

Nothing changes from the "UI" or "interface" side where how currently outsiders (including googleapis/googleapis) generate GAPIC libraries. The way gapic-generator-java is built is an implementation detail (that changed from Bazel to Maven) and is completely hidden from outsiders.

Local development is almost the same like before, two notable changes:

  1. Unit tests can only be done in Maven now.
  2. Before running integration tests, you would need to build a local gapic-generator-java jar.

Next Steps[Must haves]:

  1. After this PR is merged and a new version of gapic-generator-java is released, we need to update the googleapis repo to make changes accordingly.
  2. The gradle files generated by the self-service client libraries are still using the dependencies.properties file from gax-java repo, which may include different versions from the pom.xml that is used during generation runtime. It is to be determined to find the best way to manage the versions for the generated Gradle files.

Next Steps[Nice to haves, not an exhaustive list]:

  1. Integration Tests still rely on Bazel to run, this is expected as the Bazel interface didn't change, and integration tests are triggering the actual library generation process which is still based on Bazel. However, we could wrap it as a Maven plugin so that the whole development flow will not dependent on Bazel at all.
  2. This repo still exposes Java Bazel rules in rules_java_gapic folder, which is referenced by googleapis in the generation process, hence googleapis will need to dependent on both the jar and the source of gapic-generator-java. We could move these Bazel rules to googleapis so that gapic-generator-java is completely self-contained.

@chanseokoh
Copy link
Contributor Author

chanseokoh commented Feb 2, 2022

More files removed that are noteworthy: PROPERTIES.bzl and repositories.bzl.

Note:

  • These don't affect "self-service" library generation.
  • Currently, googleapis/googleapis does fetch repositories.bzl from this repo. However, with this PoC, googleapis doesn't need to do that, so we can simply make googleapis stop fetching it.

@chanseokoh
Copy link
Contributor Author

chanseokoh commented Feb 3, 2022

Unit tests are not wokring. mvn test is basically functioning, but tests are currently failing in this PoC. (mvn package without -DskipTests will thus fail too.)

mvn test is now working. Updating unit golden files still needs some adjustments to work. Updating unit golden files are working too.

@blakeli0 blakeli0 requested a review from a team as a code owner December 29, 2022 05:19
@burkedavison
Copy link
Contributor

Sorry for the unintentional pushes to this branch.

@blakeli0 blakeli0 requested review from dangazineu and removed request for vam-google January 5, 2023 00:56
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

LGTM. Deferring to @ddixit14 on the CI config changes.

@sonarcloud
Copy link

sonarcloud bot commented Jan 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@blakeli0 blakeli0 merged commit 0051f49 into main Jan 6, 2023
@blakeli0 blakeli0 deleted the maven-poc branch January 6, 2023 07:25
lqiu96 added a commit that referenced this pull request Jan 10, 2023
lqiu96 added a commit that referenced this pull request Jan 10, 2023
…ary generation process (#918)""

This reverts commit 16d552c8a0225eb6c561eadc67921518c5664544.
suztomo pushed a commit that referenced this pull request Mar 21, 2023
suztomo pushed a commit that referenced this pull request Mar 21, 2023
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
bdhess pushed a commit to GoogleCloudPlatform/kms-integrations that referenced this pull request May 18, 2023
The repository has been renamed to sdk-platform-java, which caused
issues with the strip_prefix and the sha256 checksum.

In the long term we probably want to look into
googleapis/sdk-platform-java#918 and update our
dependency accordingly if needed, but for now this should be good
enough.

Change-Id: I0388c103d7dda729c30d28995517880336ba9d42
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.

None yet

5 participants