fix(build_gen): support proto java_package, make gapic.yaml dep optional, add tests [rules_gapic] #26
Conversation
product-auto-label
bot
added
the
api: cloudbuild
google-cla
bot
added
the
cla: yes
There was a problem hiding this comment.
LGTM but please wait for @vam-google's review since he knows this code better than I do.
| Pattern.compile( | ||
| "(?m)^option\\s+(?<optName>\\w+)\\s+=\\s+\"(?<optValue>[\\w./;\\\\\\-]+)\"\\s*;\\s*$"); | ||
| "(?m)^option\\s+(?<optName>(java|go|csharp|ruby|php|javascript)_(namespace|package))\\s+=\\s+\"(?<optValue>[\\w./;\\\\\\-]+)\"\\s*;\\s*$"); |
There was a problem hiding this comment.
Was the less specific regexp failing on practical cases (if yes, can you please give an example?), or is this change mostly to avoid something like that in the future?
There was a problem hiding this comment.
Yes, it was failing. For Java, it grabbed the first available option, resulting in the wrong value. For example, since java_outer_classname comes first in the asset proto, we ended up with the map entry "java" : "AssetProto" instead of the expected "java" : "com.google.cloud.asset.v1". I think this worked for other languages because they generally have only one config option.
| @@ -361,9 +366,11 @@ void parseBazelBuildFile(Path file) { | |||
| // Duplicated rule of the same kind will break our logic for preserving rule name. | |||
| System.err.println("There are more than one rule of kind " + kind + "."); | |||
| System.err.println( | |||
| "Bazel build file generator does not support regenerating BUILD.bazel in this case."); | |||
| "Bazel build file generator does not support regenerating BUILD.bazel in this" | |||
There was a problem hiding this comment.
Since we are already touching this code and split it into concatenated strings, can you please move all that into single System.err.println() (instead of using subseuent calls to print essentially a single multi-line error message)?
| bp.getLangGapicNameOverrides() | ||
| .get("java") | ||
| .getOrDefault(bp.getProtoPackage() + "." + service, service); | ||
| bp.getLangGapicNameOverrides().containsKey("java") |
There was a problem hiding this comment.
I've seen recently the test targets for java test rules generated with empty test class list. Looks like this has potential of fixing that, right? If yes, then it is awesome!
| @@ -80,7 +80,7 @@ java_gapic_library( | |||
| java_gapic_test( | |||
| name = "library_java_gapic_test_suite", | |||
| test_classes = [ | |||
| "com.google.cloud.example.library.v1.LibraryServiceClientTest", | |||
| "com.google.example.library.v1.LibraryServiceClientTest", | |||
There was a problem hiding this comment.
The actual clients (regardless if they are micro or macro generated) are still distributed under "cloud" namespace. Where does the "cloud" portion in the namespace should come from now, if not from gapic yaml?
I'm afraid that we may still be generating client libraries under cloud namespace, but the tests specified in the build file may be loosing the cloud portion of the package, making it a broken test class name essentially.
There was a problem hiding this comment.
If you're referring to this Library client specifically, any other suggested value for java_package in the protos are welcome. This test means to exercise the "detect package from proto" case with a package distinct from the gapic.yaml one, so any value that achieves that is fine.
For other APIs, many have .cloud in their proto-specified java_package (e.g. Functions).
| @@ -0,0 +1,336 @@ | |||
| # This file was automatically generated by BuildFileGenerator | |||
There was a problem hiding this comment.
Do we really want to keep the legacy tests? It seems like a lot of text with sophisticated folder structure is added only to test the very small, legacy and rare case of reading from gapic yaml. I think it is ok just to not test that obscure execution path in the tests.
There was a problem hiding this comment.
Yes - we need to ensure we don't break the 10+ clients who still use this. We also need to ensure the delta between the main and legacy test cases are clear, otherwise an untested edge case could easily slip past.
We could remove sections of the library proto that don't affect imports (for both cases), and I'd be happy to do that in a separate PR.
build_genpreviously relied onlanguage_settings.javaingapic.yamlto find and generate test classes inBUILD.bazel. However, modern GAPIC clients shouldn't usegapic.yamlat all, so we should derive the Java package from the options specified in the protobuf files.The existing regex to parse language package options was actually grabbing the first <common_prefix><more_text> proto option, so I fixed that as well.
Since Java needs to continue supporting the
gapic.yamllanguage overrides for the foreseeable future (for popular legacy clients like PubSub), I added a legacy test as well.