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.

Sign up for GitHub

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: search gapic additional protos in BUILD.bazel #2004

Merged
merged 9 commits into from
Sep 20, 2023
Merged

feat: search gapic additional protos in BUILD.bazel #2004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4765f15
fix: search additional protos in `BUILD.bazel`
JoeWang1127 Sep 18, 2023
0ae9731
add unit tests
JoeWang1127 Sep 18, 2023
c6917cd
modify readme
JoeWang1127 Sep 18, 2023
e8eb12f
only search in proto_library_with_info
JoeWang1127 Sep 18, 2023
cf1cf50
fix showcase ci
JoeWang1127 Sep 18, 2023
ea7554b
add option: gapic_additional_protos
JoeWang1127 Sep 19, 2023
34b6a74
change showcase
JoeWang1127 Sep 19, 2023
cda8f83
change default value
JoeWang1127 Sep 19, 2023
319d107
remove unused function
JoeWang1127 Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions library_generation/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,22 @@ Use `--grpc_version` to specify the value.

Note that if specified, the version should be compatible with gapic-generator-java.

### gapic_additional_protos (optional)
Additional protos that pass to the generator.
The default value is `google/cloud/common_resources.proto`.

Use `--gapic_additional_protos` to specify the value.

### transport (optional)
One of GAPIC options passed to the generator. The value is either `grpc` or `grpc+rest`.
One of GAPIC options passed to the generator.
The value is either `grpc` or `grpc+rest`.
The default value is `grpc`.

Use `--transport` to specify the value.

### rest_numeric_enums (optional)
One of GAPIC options passed to the generator. The value is either `true` or `false`.
One of GAPIC options passed to the generator.
The value is either `true` or `false`.
The default value is `true`.

Use `--rest_numeric_enums` to specify the value.
Expand All @@ -121,6 +129,7 @@ library_generation/generate_library.sh \
--gapic_generator_version 2.24.0 \
--protobuf_version 23.2 \
--grpc_version 1.55.1 \
--gapic_additional_protos "google/cloud/common_resources.proto google/cloud/location/locations.proto" \
--transport grpc+rest \
--rest_numeric_enums true \
--include_samples true
Expand Down
11 changes: 9 additions & 2 deletions library_generation/generate_library.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ case $key in
grpc_version="$2"
shift
;;
--gapic_additional_protos)
gapic_additional_protos="$2"
shift
;;
--transport)
transport="$2"
shift
Expand Down Expand Up @@ -56,7 +60,6 @@ done
script_dir=$(dirname "$(readlink -f "$0")")
source "${script_dir}"/utilities.sh
output_folder="$(get_output_folder)"
# source utility functions

if [ -z "${protobuf_version}" ]; then
protobuf_version=$(get_protobuf_version "${gapic_generator_version}")
Expand All @@ -66,6 +69,10 @@ if [ -z "${grpc_version}" ]; then
grpc_version=$(get_grpc_version "${gapic_generator_version}")
fi

if [ -z "${gapic_additional_protos}" ]; then
gapic_additional_protos="google/cloud/common_resources.proto"
fi

if [ -z "${transport}" ]; then
transport="grpc"
fi
Expand Down Expand Up @@ -117,7 +124,7 @@ fi
"--plugin=protoc-gen-java_gapic=${script_dir}/gapic-generator-java-wrapper" \
"--java_gapic_out=metadata:${destination_path}/java_gapic_srcjar_raw.srcjar.zip" \
"--java_gapic_opt=$(get_gapic_opts)" \
${proto_files} $(search_additional_protos)
${proto_files} ${gapic_additional_protos}

unzip -o -q "${destination_path}/java_gapic_srcjar_raw.srcjar.zip" -d "${destination_path}"
# Sync'\''d to the output file name in Writer.java.
Expand Down
2 changes: 2 additions & 0 deletions library_generation/test/generate_library_integration_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ grep -v '^ *#' < "${proto_path_list}" | while IFS= read -r line; do
# parse GAPIC options from proto_path/BUILD.bazel
pushd "${output_folder}"
proto_build_file_path="${proto_path}/BUILD.bazel"
gapic_additional_protos=$(get_gapic_additional_protos_from_BUILD "${proto_build_file_path}")
transport=$(get_transport_from_BUILD "${proto_build_file_path}")
rest_numeric_enums=$(get_rest_numeric_enums_from_BUILD "${proto_build_file_path}")
include_samples=$(get_include_samples_from_BUILD "${proto_build_file_path}")
Expand All @@ -81,6 +82,7 @@ grep -v '^ *#' < "${proto_path_list}" | while IFS= read -r line; do
--gapic_generator_version "${gapic_generator_version}" \
--protobuf_version "${protobuf_version}" \
--grpc_version "${grpc_version}" \
--gapic_additional_protos "${gapic_additional_protos}" \
--transport "${transport}" \
--rest_numeric_enums "${rest_numeric_enums}" \
--include_samples "${include_samples}"
Expand Down
70 changes: 32 additions & 38 deletions library_generation/test/generate_library_unit_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,40 +41,6 @@ get_protobuf_version_failed_with_invalid_generator_version_test() {
assertEquals 1 $((res))
}

search_additional_protos_common_resources_test() {
local proto_path="${script_dir}/resources/search_additional_proto/common_resources"
local addition_protos
addition_protos=$(search_additional_protos)
assertEquals "google/cloud/common_resources.proto" "${addition_protos}"
}

search_additional_protos_iam_test() {
local proto_path="${script_dir}/resources/search_additional_protos/iam"
local addition_protos
addition_protos=$(search_additional_protos)
assertEquals \
"google/cloud/common_resources.proto google/iam/v1/iam_policy.proto" \
"${addition_protos}"
}

search_additional_protos_location_test() {
local proto_path="${script_dir}/resources/search_additional_protos/location"
local addition_protos
addition_protos=$(search_additional_protos)
assertEquals \
"google/cloud/common_resources.proto google/cloud/location/locations.proto" \
"${addition_protos}"
}

search_additional_protos_iam_location_test() {
local proto_path="${script_dir}/resources/search_additional_protos/iam_location"
local addition_protos
addition_protos=$(search_additional_protos)
assertEquals \
"google/cloud/common_resources.proto google/iam/v1/iam_policy.proto google/cloud/location/locations.proto" \
"${addition_protos}"
}

get_gapic_opts_with_rest_test() {
local proto_path="${script_dir}/resources/gapic_options"
local transport="grpc"
Expand Down Expand Up @@ -227,6 +193,34 @@ generate_library_failed_with_invalid_grpc_version() {
cleanup "${destination}"
}

get_gapic_additional_protos_from_BUILD_common_resources_test() {
local proto_path="${script_dir}/resources/search_additional_protos/BUILD_common_resources.bazel"
local addition_protos
addition_protos=$(get_gapic_additional_protos_from_BUILD "${proto_path}")
assertEquals "google/cloud/common_resources.proto" "${addition_protos}"
}

get_gapic_additional_protos_from_BUILD_iam_policy_test() {
local proto_path="${script_dir}/resources/search_additional_protos/BUILD_iam_policy.bazel"
local addition_protos
addition_protos=$(get_gapic_additional_protos_from_BUILD "${proto_path}")
assertEquals "google/cloud/common_resources.proto google/iam/v1/iam_policy.proto" "${addition_protos}"
}

get_gapic_additional_protos_from_BUILD_locations_test() {
local proto_path="${script_dir}/resources/search_additional_protos/BUILD_locations.bazel"
local addition_protos
addition_protos=$(get_gapic_additional_protos_from_BUILD "${proto_path}")
assertEquals "google/cloud/common_resources.proto google/cloud/location/locations.proto" "${addition_protos}"
}

get_gapic_additional_protos_from_BUILD_iam_locations_test() {
local proto_path="${script_dir}/resources/search_additional_protos/BUILD_iam_locations.bazel"
local addition_protos
addition_protos=$(get_gapic_additional_protos_from_BUILD "${proto_path}")
assertEquals "google/cloud/common_resources.proto google/iam/v1/iam_policy.proto google/cloud/location/locations.proto" "${addition_protos}"
}

get_transport_from_BUILD_grpc_rest_test() {
local build_file="${script_dir}/resources/misc/BUILD_grpc_rest.bazel"
local transport
Expand Down Expand Up @@ -316,10 +310,6 @@ test_list=(
get_grpc_version_failed_with_invalid_generator_version_test
get_protobuf_version_succeed_with_valid_generator_version_test
get_protobuf_version_failed_with_invalid_generator_version_test
search_additional_protos_common_resources_test
search_additional_protos_iam_test
search_additional_protos_location_test
search_additional_protos_iam_location_test
get_gapic_opts_with_rest_test
get_gapic_opts_without_rest_test
remove_grpc_version_test
Expand All @@ -336,6 +326,10 @@ test_list=(
generate_library_failed_with_invalid_generator_version
generate_library_failed_with_invalid_protobuf_version
generate_library_failed_with_invalid_grpc_version
get_gapic_additional_protos_from_BUILD_common_resources_test
get_gapic_additional_protos_from_BUILD_iam_policy_test
get_gapic_additional_protos_from_BUILD_locations_test
get_gapic_additional_protos_from_BUILD_iam_locations_test
get_transport_from_BUILD_grpc_rest_test
get_transport_from_BUILD_grpc_test
get_transport_from_BUILD_rest_test
Expand Down
6 changes: 6 additions & 0 deletions library_generation/test/resources/search_additional_protos/BUILD_common_resources.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# this file is only used in testing `get_gapic_additional_protos_from_BUILD` in utilities.sh

proto_library_with_info(
deps = [
]
)
8 changes: 8 additions & 0 deletions library_generation/test/resources/search_additional_protos/BUILD_iam_locations.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# this file is only used in testing `get_gapic_additional_protos_from_BUILD` in utilities.sh

proto_library_with_info(
deps = [
"//google/iam/v1:iam_policy_proto",
"//google/cloud/location:location_proto",
]
)
7 changes: 7 additions & 0 deletions library_generation/test/resources/search_additional_protos/BUILD_iam_policy.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# this file is only used in testing `get_gapic_additional_protos_from_BUILD` in utilities.sh

proto_library_with_info(
deps = [
"//google/iam/v1:iam_policy_proto",
]
)
7 changes: 7 additions & 0 deletions library_generation/test/resources/search_additional_protos/BUILD_locations.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# this file is only used in testing `get_gapic_additional_protos_from_BUILD` in utilities.sh

proto_library_with_info(
deps = [
"//google/cloud/location:location_proto",
]
)
10 changes: 0 additions & 10 deletions ...ch_additional_protos/common_resources/search_additional_protos_common_resources_test.yaml
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions ...ration/test/resources/search_additional_protos/iam/search_additional_protos_iam_test.yaml
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions ...ces/search_additional_protos/iam_location/search_additional_protos_iam_location_test.yaml
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions ...t/resources/search_additional_protos/location/search_additional_protos_location_test.yaml
View file
Open in desktop

This file was deleted.

72 changes: 44 additions & 28 deletions library_generation/utilities.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,38 @@ __get_config_from_BUILD() {
if_match=$5

result="${default}"
if grep -A 15 "${rule}" "${build_file}" | grep -q "${pattern}"; then
if grep -A 20 "${rule}" "${build_file}" | grep -q "${pattern}"; then
result="${if_match}"
fi
echo "${result}"
}

__get_iam_policy_from_BUILD() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need to search for these protos from BUILD now that they are being passed in as parameters?
In addition, do we need to read anything else from BUILD file other than searching for common protos?

Copy link
Collaborator Author

@JoeWang1127 JoeWang1127 Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we still need to search for these protos from BUILD now that they are being passed in as parameters?

All functions that read BUILD are used by generate_library_integration_test.sh for testing against googleapis-gen. They are not in test_utilities.sh because they are part of generate_sdk_libraries.sh later.

In addition, do we need to read anything else from BUILD file other than searching for common protos?

transport, rest_numeric_enums and include_samples are also read from BUILD for testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All functions that read BUILD are used by generate_library_integration_test.sh for testing against googleapis-gen. They are not in test_utilities.sh because they are part of generate_sdk_libraries.sh later.

I'm not sure we need it in generate_sdk_libraries.sh either. We would still need to keep the bazel interface, so for a single client library, the flow would be bazel build -> java_gapic_assembly_gradle_pkg -> generate_library.sh, where java_gapic_assembly_gradle_pkg would pass all the info from bazel file to our shell script. So generate_sdk_libraries.sh would just be calling bazel build for each service in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that's the case then we probably don't need these utility functions.

Right now it's only used in testing and can be removed without impacting generate_library.sh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now it's only used in testing and can be removed without impacting generate_library.sh.

We can still keep them and move them to the test utils.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can still keep them and move them to the test utils.

How about I create another PR to refactor test utilities into test_utilities.sh. I think there are other functions can move into test_utilities.sh as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM

local build_file=$1
local contains_iam_policy
contains_iam_policy=$(__get_config_from_BUILD \
"${build_file}" \
"proto_library_with_info(" \
"//google/iam/v1:iam_policy_proto" \
"false" \
"true"
)
echo "${contains_iam_policy}"
}

__get_locations_from_BUILD() {
local build_file=$1
local contains_locations
contains_locations=$(__get_config_from_BUILD \
"${build_file}" \
"proto_library_with_info(" \
"//google/cloud/location:location_proto" \
"false" \
"true"
)
echo "${contains_locations}"
}

# define utility functions
extract_folder_name() {
local destination_path=$1
Expand Down Expand Up @@ -69,33 +95,6 @@ unzip_src_files() {
rm -r -f "${destination_path}/${category}-${folder_name}/src/main/java/META-INF"
}

find_additional_protos_in_yaml() {
local pattern=$1
local find_result
find_result=$(grep --include=\*.yaml -rw "${proto_path}" -e "${pattern}")
if [ -n "${find_result}" ]; then
echo "${find_result}"
fi
}

# Apart from proto files in proto_path, additional protos are needed in order
# to generate GAPIC client libraries.
# In most cases, these protos should be within google/ directory, which is
# pulled from googleapis as a prerequisite.
# Search additional protos in .yaml files.
search_additional_protos() {
additional_protos="google/cloud/common_resources.proto" # used by every library
iam_policy=$(find_additional_protos_in_yaml "name: '*google.iam.v1.IAMPolicy'*")
if [ -n "$iam_policy" ]; then
additional_protos="$additional_protos google/iam/v1/iam_policy.proto"
fi
locations=$(find_additional_protos_in_yaml "name: '*google.cloud.location.Locations'*")
if [ -n "${locations}" ]; then
additional_protos="$additional_protos google/cloud/location/locations.proto"
fi
echo "${additional_protos}"
}

# get gapic options from .yaml and .json files from proto_path.
get_gapic_opts() {
local gapic_config
Expand Down Expand Up @@ -253,6 +252,23 @@ get_version_from_WORKSPACE() {
echo "${version}"
}

# Apart from proto files in proto_path, additional protos are needed in order
# to generate GAPIC client libraries.
# In most cases, these protos should be within google/ directory, which is
# pulled from googleapis as a prerequisite.
# Get additional protos in BUILD.bazel.
get_gapic_additional_protos_from_BUILD() {
local build_file=$1
local gapic_additional_protos="google/cloud/common_resources.proto"
if [[ $(__get_iam_policy_from_BUILD "${build_file}") == "true" ]]; then
gapic_additional_protos="${gapic_additional_protos} google/iam/v1/iam_policy.proto"
fi
if [[ $(__get_locations_from_BUILD "${build_file}") == "true" ]]; then
gapic_additional_protos="${gapic_additional_protos} google/cloud/location/locations.proto"
fi
echo "${gapic_additional_protos}"
}

get_transport_from_BUILD() {
local build_file=$1
local transport
Expand Down
2 changes: 2 additions & 0 deletions showcase/scripts/generate_showcase.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ fi


ggj_version=$(get_version_from_versions_txt ../../versions.txt "gapic-generator-java")
gapic_additional_protos="google/iam/v1/iam_policy.proto google/cloud/location/locations.proto"
rest_numeric_enums="false"
transport="grpc+rest"
include_samples="false"
Expand All @@ -49,6 +50,7 @@ bash "${SCRIPT_DIR}/../../library_generation/generate_library.sh" \
--proto_path "schema/google/showcase/v1beta1" \
--destination_path "showcase-output" \
--gapic_generator_version "${ggj_version}" \
--gapic_additional_protos "${gapic_additional_protos}" \
--rest_numeric_enums "${rest_numeric_enums}" \
--include_samples "${include_samples}" \
--transport "${transport}"
Expand Down
Loading