gapic-generator-java icon indicating copy to clipboard operation
gapic-generator-java copied to clipboard

feat: gapic-generator-java to perform a no-op when no services are detected

Open diegomarquezp opened this issue 1 year ago • 10 comments

Fixes https://github.com/googleapis/sdk-platform-java/issues/2050

Adds behavior to gracefully perform a NOOP if no services are contained in the generation request.

Confimation in hermetic build scripts

From generate_library.sh against google/cloud/alloydb/connectors/v1

+ /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk3/sdk-platform-java/library_generation/output/protobuf-25.2/bin/protoc --experimental_allow_proto3_optional --plugin=protoc-gen-java_gapic=/usr/local/google/home/diegomarquezp/.pyenv/versions/3.11.0/lib/python3.11/site-packages/library_generation/gapic-generator-java-wrapper --java_gapic_out=metadata:/usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk3/sdk-platform-java/library_generation/output/temp_preprocessed/java_gapic_srcjar_raw.srcjar.zip --java_gapic_opt=transport=grpc,rest-numeric-enums,grpc-service-config=,gapic-config=,api-service-config=google/cloud/alloydb/connectors/v1/connectors_v1.yaml google/cloud/alloydb/connectors/v1/resources.proto google/cloud/common_resources.proto
Apr 05, 2024 9:33:22 PM com.google.api.generator.gapic.protoparser.Parser parse
WARNING: No services found to generate. This will produce an empty zip file
Apr 05, 2024 9:33:22 PM com.google.api.generator.gapic.composer.ClientLibraryPackageInfoComposer generatePackageInfo
WARNING: Generating empty package info since no services were found
+ did_generate_gapic=true
+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk3/sdk-platform-java/library_generation/output/temp_preprocessed/java_gapic_srcjar_raw.srcjar.zip
Empty zipfile. 
+ did_generate_gapic=false
+ [[ false == \t\r\u\e ]]

I made some changes to library_generation but I moved them to a follow up PR (https://github.com/googleapis/sdk-platform-java/pull/2599) so the checks can pass here.

diegomarquezp avatar Feb 12 '24 22:02 diegomarquezp

Throwing an exception and catch it just to perform no-op and/or logging is not a good practice. It would be better if we can do the same thing gracefully. For example, can we remove this check, log the scenario, and return an empty GapicContext?

blakeli0 avatar Feb 28 '24 15:02 blakeli0

From https://github.com/googleapis/sdk-platform-java/actions/runs/8150253915/job/22276206879?pr=2460

+ /home/runner/work/sdk-platform-java/sdk-platform-java/output/protobuf-25.2/bin/protoc --experimental_allow_proto3_optional --plugin=protoc-gen-java_gapic=/home/runner/work/sdk-platform-java/sdk-platform-java/library_generation/gapic-generator-java-wrapper --java_gapic_out=metadata:/home/runner/work/sdk-platform-java/sdk-platform-java/output/temp_preprocessed/java_gapic_srcjar_raw.srcjar.zip --java_gapic_opt=transport=grpc,rest-numeric-enums,grpc-service-config=,gapic-config=,api-service-config=google/cloud/alloydb/connectors/v1/connectors_v1.yaml google/cloud/alloydb/connectors/v1/resources.proto google/cloud/common_resources.proto
Error: Exception in thread "main" java.lang.IllegalStateException: No services found to generate
	at com.google.common.base.Preconditions.checkState(Preconditions.java:512)
	at com.google.api.generator.gapic.protoparser.Parser.parse(Parser.java:178)
	at com.google.api.generator.gapic.Generator.generateGapic(Generator.java:30)
	at com.google.api.generator.Main.main(Main.java:28)

I confirmed they pass locally. Not sure how to prove it in a PR that updates both the generator and the hermetic build scripts

Reason: the hermetic build IT uses a published GGJ instead of compiling its own

EDIT: I decided to move all library_generation changes to https://github.com/googleapis/sdk-platform-java/pull/2599

diegomarquezp avatar Mar 05 '24 02:03 diegomarquezp

Thanks @diegomarquezp! I think this looks good. To refresh my memory, this is to deal with the scenario where we try to generate files for messages in java-common-protos (the proto files have no services), right?

lqiu96 avatar Mar 22 '24 22:03 lqiu96

@diegomarquezp I'm a little late to this, but is there a related issue I can review that this is trying to solve?

meltsufin avatar Mar 23 '24 01:03 meltsufin

@diegomarquezp I'm a little late to this, but is there a related issue I can review that this is trying to solve?

@meltsufin https://github.com/googleapis/sdk-platform-java/issues/2050

I'm adding it to the pr description

diegomarquezp avatar Mar 23 '24 01:03 diegomarquezp

What happens if you try to unzip temp-codegen.srcjar? Does it fail or produce an empty folder (I prefer this one because we can add logic to remove empty folders using remove_empty_files in utilities.sh, similarly to grpc plugin)?

+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk-platform-java/library_generation/output/temp_preprocessed/temp-codegen.srcjar
Empty zipfile. 
+ did_generate_gapic=false

JoeWang1127 avatar Mar 24 '24 17:03 JoeWang1127

What happens if you try to unzip temp-codegen.srcjar? Does it fail or produce an empty folder (I prefer this one because we can add logic to remove empty folders using remove_empty_files in utilities.sh, similarly to grpc plugin)?

+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk-platform-java/library_generation/output/temp_preprocessed/temp-codegen.srcjar
Empty zipfile. 
+ did_generate_gapic=false

We will now NOT generate a srcjar if no services are found. This means that the wrapper zip java_gapic_srcjar_raw.srcjar.zip will be empty. If we try to unzip it, it will fail (and the script will exit due to set -e), so we still need that zipinfo check. So to answer the question, it would not generate an empty folder, it would just fail.

diegomarquezp avatar Apr 05 '24 21:04 diegomarquezp

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Apr 05 '24 21:04 sonarqubecloud[bot]

What happens if you try to unzip temp-codegen.srcjar? Does it fail or produce an empty folder (I prefer this one because we can add logic to remove empty folders using remove_empty_files in utilities.sh, similarly to grpc plugin)?

+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk-platform-java/library_generation/output/temp_preprocessed/temp-codegen.srcjar
Empty zipfile. 
+ did_generate_gapic=false

We will now NOT generate a srcjar if no services are found. This means that the wrapper zip java_gapic_srcjar_raw.srcjar.zip will be empty. If we try to unzip it, it will fail (and the script will exit due to set -e), so we still need that zipinfo check. So to answer the question, it would not generate an empty folder, it would just fail.

What creates the java_gapic_srcjar_raw.srcjar.zip? The generator or the hermetic build scripts? Can we not generate anything at all, which would be a true no-op? In general, I think the changes in GapicContext and Parser makes sense, but the we may not need changes in Composer and Writer. For example, can we return null CodeGeneratorResponse and skip writing to disk all together?

blakeli0 avatar Apr 09 '24 22:04 blakeli0

What happens if you try to unzip temp-codegen.srcjar? Does it fail or produce an empty folder (I prefer this one because we can add logic to remove empty folders using remove_empty_files in utilities.sh, similarly to grpc plugin)?

+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk-platform-java/library_generation/output/temp_preprocessed/temp-codegen.srcjar
Empty zipfile. 
+ did_generate_gapic=false

We will now NOT generate a srcjar if no services are found. This means that the wrapper zip java_gapic_srcjar_raw.srcjar.zip will be empty. If we try to unzip it, it will fail (and the script will exit due to set -e), so we still need that zipinfo check. So to answer the question, it would not generate an empty folder, it would just fail.

What creates the java_gapic_srcjar_raw.srcjar.zip? The generator or the hermetic build scripts? Can we not generate anything at all, which would be a true no-op? In general, I think the changes in GapicContext and Parser makes sense, but the we may not need changes in Composer and Writer. For example, can we return null CodeGeneratorResponse and skip writing to disk all together?

That sounds good. I modified Writer to return null if the context is empty, rendering the changes in package info unnecessary.

diegomarquezp avatar May 28 '24 22:05 diegomarquezp

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Jun 08 '24 01:06 sonarqubecloud[bot]