chore: remove protoc grpc from generation config (#3390)

JoeWang1127 · web-flow · commit 7ee0089bf84a · 2024-11-25T14:59:46.000-05:00
In this PR:
- Remove `protoc_version` and `grpc_version` from generation config.
diff --git a/.cloudbuild/library_generation/library_generation.Dockerfile b/.cloudbuild/library_generation/library_generation.Dockerfile
@@ -88,24 +88,23 @@ WORKDIR /protoc
 RUN source /src/library_generation/utils/utilities.sh \
 	&& download_protoc "${PROTOC_VERSION}" "${OS_ARCHITECTURE}"
 # we indicate protoc is available in the container via env vars
-ENV DOCKER_PROTOC_LOCATION=/protoc
+ENV DOCKER_PROTOC_LOCATION=/protoc/bin
 ENV DOCKER_PROTOC_VERSION="${PROTOC_VERSION}"
 
 # install grpc
 WORKDIR /grpc
 RUN source /src/library_generation/utils/utilities.sh \
 	&& download_grpc_plugin "${GRPC_VERSION}" "${OS_ARCHITECTURE}"
 # similar to protoc, we indicate grpc is available in the container via env vars
-ENV DOCKER_GRPC_LOCATION="/grpc/protoc-gen-grpc-java-${GRPC_VERSION}-${OS_ARCHITECTURE}.exe"
-ENV DOCKER_GRPC_VERSION="${GRPC_VERSION}"
-
+ENV DOCKER_GRPC_LOCATION="/grpc/protoc-gen-grpc-java.exe"
 
 # Here we transfer gapic-generator-java from the previous stage.
 # Note that the destination is a well-known location that will be assumed at runtime
 # We hard-code the location string to avoid making it configurable (via ARG) as
 # well as to avoid it making it overridable at runtime (via ENV).
 COPY --from=ggj-build "/sdk-platform-java/gapic-generator-java.jar" "${HOME}/.library_generation/gapic-generator-java.jar"
 RUN chmod 755 "${HOME}/.library_generation/gapic-generator-java.jar"
+ENV GAPIC_GENERATOR_LOCATION="${HOME}/.library_generation/gapic-generator-java.jar"
 
 RUN python -m pip install --upgrade pip
 
@@ -130,6 +129,7 @@ RUN apk del -r npm && apk cache clean
 ADD https://maven-central.storage-download.googleapis.com/maven2/com/google/googlejavaformat/google-java-format/${JAVA_FORMAT_VERSION}/google-java-format-${JAVA_FORMAT_VERSION}-all-deps.jar \
   "${HOME}"/.library_generation/google-java-format.jar
 RUN chmod 755 "${HOME}"/.library_generation/google-java-format.jar
+ENV JAVA_FORMATTER_LOCATION="${HOME}/.library_generation/google-java-format.jar"
 
 # allow users to access the script folders
 RUN chmod -R o+rx /src
diff --git a/hermetic_build/DEVELOPMENT.md b/hermetic_build/DEVELOPMENT.md
@@ -82,6 +82,21 @@ as per [POSIX env var definition](https://pubs.opengroup.org/onlinepubs/96999197
    mv /path/to/jar "${HOME}/.library_generation/gapic-generator-java.jar"
    ```
 
+#### Put the protobuf compiler in its well-known location
+1. Download protobuf compiler from [GitHub releases](https://github.com/protocolbuffers/protobuf/releases).
+2. Move the folder into its well-know location.
+
+   ```shell
+   unzip /path/to/zipfile -d "${HOME}/.library_generation/"
+   ```
+#### Put the GRPC plugin in its well-known location
+1. Download GRPC plugin from [Maven Central](https://central.sonatype.com/artifact/io.grpc/protoc-gen-grpc-java/versions).
+2. Move the folder into its well-know location.
+
+   ```shell
+   mv /path/to/protoc-gen-grpc-java.exe "${HOME}/.library_generation/protoc-gen-grpc-java.exe"
+   ```
+
 #### Put the java formatter jar in its well-known location
 
 1. Download google-java-format-{version}-all-deps.jar from [Maven Central](https://central.sonatype.com/artifact/com.google.googlejavaformat/google-java-format)
@@ -197,11 +212,6 @@ python hermetic_build/library_generation/cli/entry_point.py generate \
         --repository-path=/workspace \
         --api-definitions-path=/workspace/apis
    ```
-   Note that if you specify the generator version using environment variable,
-   `-e GENERATOR_VERSION="${LOCAL_GENERATOR_VERSION}"` in the above example,
-   you should not set `gapic_generator_version` and `protoc_version` in the
-   generation configuration because values in the generation configuration will
-   take precedence.
 
 # Debug the library generation container
 If you are working on changing the way the containers are created, you may want
diff --git a/hermetic_build/README.md b/hermetic_build/README.md
@@ -7,10 +7,6 @@ Running the docker image built from `hermetic_build/library_generation`
 directory, you can generate a repository containing GAPIC client libraries (a
 monorepo, for example, google-cloud-java) from a configuration file.
 
-Instead of running the docker image, if you prefer running the underlying python
-scripts directly, please refer to the [development guide](DEVELOPMENT.md#run-the-script)
-for additional instructions.
-
 ## Environment
 
 - OS: Linux
@@ -124,13 +120,11 @@ The repository level parameters define the version of API definition (proto)
 and tools.
 They are shared by library level parameters.
 
-| Name                    | Required | Notes                                        |
-|:------------------------|:--------:|:---------------------------------------------|
-| gapic_generator_version |    No    | set through env variable if not specified    |
-| protoc_version          |    No    | inferred from the generator if not specified |
-| grpc_version            |    No    | inferred from the generator if not specified |
-| googleapis_commitish    |   Yes    |                                              |
-| libraries_bom_version   |    No    | empty string if not specified                |
+| Name                    | Required | Notes                                     |
+|:------------------------|:--------:|:------------------------------------------|
+| gapic_generator_version |    No    | set through env variable if not specified |
+| googleapis_commitish    |   Yes    |                                           |
+| libraries_bom_version   |    No    | empty string if not specified             |
 
 ### Library level parameters
 
@@ -178,7 +172,6 @@ The GAPIC level parameters define how to generate a GAPIC library.
 
 ```yaml
 gapic_generator_version: 2.34.0
-protoc_version: 25.2
 googleapis_commitish: 1a45bf7393b52407188c82e63101db7dc9c72026
 libraries_bom_version: 26.37.0
 libraries:
diff --git a/hermetic_build/common/model/generation_config.py b/hermetic_build/common/model/generation_config.py
@@ -39,8 +39,6 @@ def __init__(
         libraries: list[LibraryConfig],
         gapic_generator_version: Optional[str] = None,
         libraries_bom_version: Optional[str] = None,
-        grpc_version: Optional[str] = None,
-        protoc_version: Optional[str] = None,
     ):
         self.googleapis_commitish = googleapis_commitish
         self.libraries_bom_version = (
@@ -50,8 +48,6 @@ def __init__(
             gapic_generator_version
         )
         self.libraries = libraries
-        self.grpc_version = grpc_version
-        self.protoc_version = protoc_version
         # explicit set to None so that we can compute the
         # value in getter.
         self.__contains_common_protos = None
@@ -172,8 +168,6 @@ def from_yaml(path_to_yaml: str) -> GenerationConfig:
             config, "googleapis_commitish", REPO_LEVEL_PARAMETER
         ),
         gapic_generator_version=__optional(config, GAPIC_GENERATOR_VERSION, None),
-        grpc_version=__optional(config, "grpc_version", None),
-        protoc_version=__optional(config, "protoc_version", None),
         libraries_bom_version=__optional(config, LIBRARIES_BOM_VERSION, None),
         libraries=parsed_libraries,
     )
diff --git a/hermetic_build/common/tests/model/config_change_unit_tests.py b/hermetic_build/common/tests/model/config_change_unit_tests.py
@@ -309,8 +309,6 @@ def __get_a_gen_config(
         return GenerationConfig(
             gapic_generator_version="",
             googleapis_commitish=googleapis_commitish,
-            grpc_version="",
-            protoc_version="",
             libraries=libraries,
         )
 
diff --git a/hermetic_build/common/tests/model/generation_config_unit_tests.py b/hermetic_build/common/tests/model/generation_config_unit_tests.py
@@ -74,7 +74,6 @@ def test_generation_config_set_generator_version_from_env(self):
     def test_from_yaml_succeeds(self):
         config = from_yaml(f"{test_config_dir}/generation_config.yaml")
         self.assertEqual("2.34.0", config.gapic_generator_version)
-        self.assertEqual(25.2, config.protoc_version)
         self.assertEqual(
             "1a45bf7393b52407188c82e63101db7dc9c72026", config.googleapis_commitish
         )
diff --git a/hermetic_build/common/tests/resources/test-config/generation_config.yaml b/hermetic_build/common/tests/resources/test-config/generation_config.yaml
@@ -1,5 +1,4 @@
 gapic_generator_version: 2.34.0
-protoc_version: 25.2
 googleapis_commitish: 1a45bf7393b52407188c82e63101db7dc9c72026
 libraries_bom_version: 26.37.0
 libraries:
diff --git a/hermetic_build/common/tests/utils/generation_config_comparator_unit_tests.py b/hermetic_build/common/tests/utils/generation_config_comparator_unit_tests.py
@@ -39,15 +39,11 @@ def setUp(self) -> None:
         self.baseline_config = GenerationConfig(
             gapic_generator_version="",
             googleapis_commitish="",
-            grpc_version="",
-            protoc_version="",
             libraries=[self.baseline_library],
         )
         self.current_config = GenerationConfig(
             gapic_generator_version="",
             googleapis_commitish="",
-            grpc_version="",
-            protoc_version="",
             libraries=[self.current_library],
         )
 
diff --git a/hermetic_build/library_generation/generate_composed_library.py b/hermetic_build/library_generation/generate_composed_library.py
@@ -59,7 +59,6 @@ def generate_composed_library(
     :return None
     """
     output_folder = repo_config.output_folder
-    base_arguments = __construct_tooling_arg(config=config)
     owlbot_cli_source_folder = util.sh_util("mktemp -d")
     os.makedirs(f"{library_path}", exist_ok=True)
     for gapic in library.get_sorted_gapic_configs():
@@ -81,7 +80,7 @@ def generate_composed_library(
         )
         temp_destination_path = f"java-{gapic.proto_path.replace('/','-')}"
         effective_arguments = __construct_effective_arg(
-            base_arguments=base_arguments,
+            base_arguments=[],
             gapic=gapic,
             gapic_inputs=gapic_inputs,
             temp_destination_path=temp_destination_path,
@@ -120,19 +119,6 @@ def generate_composed_library(
     )
 
 
-def __construct_tooling_arg(config: GenerationConfig) -> List[str]:
-    """
-    Construct arguments of tooling versions used in generate_library.sh
-    :param config: the generation config
-    :return: arguments containing tooling versions
-    """
-    arguments = []
-    arguments += util.create_argument("grpc_version", config)
-    arguments += util.create_argument("protoc_version", config)
-
-    return arguments
-
-
 def __construct_effective_arg(
     base_arguments: List[str],
     gapic: GapicConfig,
diff --git a/hermetic_build/library_generation/generate_library.sh b/hermetic_build/library_generation/generate_library.sh
@@ -14,14 +14,6 @@ case $key in
     destination_path="$2"
     shift
     ;;
-  --protoc_version)
-    protoc_version="$2"
-    shift
-    ;;
-  --grpc_version)
-    grpc_version="$2"
-    shift
-    ;;
   --proto_only)
     proto_only="$2"
     shift
@@ -71,14 +63,6 @@ script_dir=$(dirname "$(readlink -f "$0")")
 source "${script_dir}"/utils/utilities.sh
 output_folder="$(get_output_folder)"
 
-if [ -z "${protoc_version}" ]; then
-  protoc_version=$(get_protoc_version)
-fi
-
-if [ -z "${grpc_version}" ]; then
-  grpc_version=$(get_grpc_version)
-fi
-
 if [ -z "${proto_only}" ]; then
   proto_only="false"
 fi
@@ -171,16 +155,14 @@ case "${proto_path}" in
     proto_files="${proto_files//${removed_proto}/}"
     ;;
 esac
-# download gapic-generator-java, protobuf and grpc plugin.
-# the download_tools function will create the environment variables "protoc_path"
-# and "grpc_path", to be used in the protoc calls below.
-download_tools "${protoc_version}" "${grpc_version}" "${os_architecture}"
+
+protoc_path=$(get_protoc_location)
 ##################### Section 1 #####################
 # generate grpc-*/
 #####################################################
 if [[ ! "${transport}" == "rest" ]]; then
   # do not need to generate grpc-* if the transport is `rest`.
-  "${protoc_path}"/protoc "--plugin=protoc-gen-rpc-plugin=${grpc_path}" \
+  "${protoc_path}"/protoc "--plugin=protoc-gen-rpc-plugin=$(get_grpc_plugin_location)" \
   "--rpc-plugin_out=:${temp_destination_path}/java_grpc.jar" \
   ${proto_files} # Do not quote because this variable should not be treated as one long string.
   # unzip java_grpc.jar to grpc-*/src/main/java
diff --git a/hermetic_build/library_generation/tests/generate_library_unit_tests.py b/hermetic_build/library_generation/tests/generate_library_unit_tests.py
@@ -39,10 +39,6 @@ def setUp(self):
         # in its well-known location
         self.simulated_home = get_bash_call_output("mktemp -d")
         bash_call(f"mkdir {self.simulated_home}/.library_generation")
-        bash_call(
-            f"touch {self.simulated_home}/.library_generation/gapic-generator-java.jar"
-        )
-
         # We create a per-test directory where all output files will be created into.
         # Each folder will be deleted after its corresponding test finishes.
         test_dir = get_bash_call_output("mktemp -d")
@@ -76,34 +72,84 @@ def _run_command_and_get_sdout(self, command, **kwargs):
             command, stderr=subprocess.PIPE, **kwargs
         ).stdout.decode()[:-1]
 
-    def test_get_grpc_version_with_no_env_var_fails(self):
-        # the absence of DOCKER_GRPC_VERSION will make this function to fail
-        result = self._run_command("get_grpc_version")
+    def test_get_generator_location_with_env_returns_env(self):
+        os.environ["GAPIC_GENERATOR_LOCATION"] = "/gapic-generator-java"
+        result = self._run_command_and_get_sdout("get_gapic_generator_location")
+        self.assertEqual("/gapic-generator-java", result)
+        os.environ.pop("GAPIC_GENERATOR_LOCATION")
+
+    def test_get_generator_location_without_env_with_local_returns_local(self):
+        bash_call(
+            f"touch {self.simulated_home}/.library_generation/gapic-generator-java.jar"
+        )
+        result = self._run_command_and_get_sdout("get_gapic_generator_location")
+        self.assertEqual(
+            f"{self.simulated_home}/.library_generation/gapic-generator-java.jar",
+            result,
+        )
+
+    def test_get_generator_location_with_no_env_no_local_file_failed(self):
+        result = self._run_command("get_gapic_generator_location")
+        self.assertEqual(1, result.returncode)
+        self.assertRegex(result.stdout.decode(), "Can't find GAPIC generator in")
+
+    def test_get_protoc_location_with_env_returns_env(self):
+        os.environ["DOCKER_PROTOC_LOCATION"] = "/protoc"
+        result = self._run_command_and_get_sdout("get_protoc_location")
+        self.assertEqual("/protoc", result)
+        os.environ.pop("DOCKER_PROTOC_LOCATION")
+
+    def test_get_protoc_location_without_env_with_local_returns_local(self):
+        bash_call(f"mkdir -p {self.simulated_home}/.library_generation/bin")
+        result = self._run_command_and_get_sdout("get_protoc_location")
+        self.assertEqual(
+            f"{self.simulated_home}/.library_generation/bin",
+            result,
+        )
+
+    def test_get_protoc_location_with_no_env_no_local_file_failed(self):
+        result = self._run_command("get_protoc_location")
         self.assertEqual(1, result.returncode)
-        self.assertRegex(result.stdout.decode(), "DOCKER_GRPC_VERSION is not set")
+        self.assertRegex(result.stdout.decode(), "Can't find protoc in")
+
+    def test_get_grpc_plugin_location_with_env_returns_env(self):
+        os.environ["DOCKER_GRPC_LOCATION"] = "/grpc"
+        result = self._run_command_and_get_sdout("get_grpc_plugin_location")
+        self.assertEqual("/grpc", result)
+        os.environ.pop("DOCKER_GRPC_LOCATION")
 
-    def test_get_protoc_version_with_no_env_var_fails(self):
-        # the absence of DOCKER_PROTOC_VERSION will make this function to fail
-        result = self._run_command("get_protoc_version")
+    def test_get_grpc_plugin_location_without_env_with_local_returns_local(self):
+        bash_call(
+            f"touch {self.simulated_home}/.library_generation/protoc-gen-grpc-java.exe"
+        )
+        result = self._run_command_and_get_sdout("get_grpc_plugin_location")
+        self.assertEqual(
+            f"{self.simulated_home}/.library_generation/protoc-gen-grpc-java.exe",
+            result,
+        )
+
+    def test_get_grpc_plugin_location_with_no_env_no_local_file_failed(self):
+        result = self._run_command("get_grpc_plugin_location")
         self.assertEqual(1, result.returncode)
-        self.assertRegex(result.stdout.decode(), "DOCKER_PROTOC_VERSION is not set")
+        self.assertRegex(result.stdout.decode(), "Can't find grpc plugin in")
 
-    def test_download_tools_without_baked_generator_fails(self):
-        # This test has the same structure as
-        # download_tools_succeed_with_baked_protoc, but meant for
-        # gapic-generator-java.
+    def test_get_formatter_location_with_env_returns_env(self):
+        os.environ["JAVA_FORMATTER_LOCATION"] = "/java-formatter"
+        result = self._run_command_and_get_sdout("get_java_formatter_location")
+        self.assertEqual("/java-formatter", result)
+        os.environ.pop("JAVA_FORMATTER_LOCATION")
 
-        test_protoc_version = "1.64.0"
-        test_grpc_version = "1.64.0"
-        jar_location = (
-            f"{self.simulated_home}/.library_generation/gapic-generator-java.jar"
+    def test_get_formatter_location_without_env_with_local_returns_local(self):
+        bash_call(
+            f"touch {self.simulated_home}/.library_generation/google-java-format.jar"
         )
-        # we expect the function to fail because the generator jar is not found in
-        # its well-known location. To achieve this, we temporarily remove the fake
-        # generator jar
-        bash_call(f"rm {jar_location}")
-        result = self._run_command(
-            f"download_tools {test_protoc_version} {test_grpc_version} {self.TEST_ARCHITECTURE}"
+        result = self._run_command_and_get_sdout("get_java_formatter_location")
+        self.assertEqual(
+            f"{self.simulated_home}/.library_generation/google-java-format.jar",
+            result,
         )
+
+    def test_get_formatter_location_with_no_env_no_local_file_failed(self):
+        result = self._run_command("get_java_formatter_location")
         self.assertEqual(1, result.returncode)
-        self.assertRegex(result.stdout.decode(), "Please configure your environment")
+        self.assertRegex(result.stdout.decode(), "Can't find Java formatter in")
diff --git a/hermetic_build/library_generation/tests/generate_library_unit_tests.sh b/hermetic_build/library_generation/tests/generate_library_unit_tests.sh
diff --git a/hermetic_build/library_generation/tests/utilities_unit_tests.py b/hermetic_build/library_generation/tests/utilities_unit_tests.py
diff --git a/hermetic_build/library_generation/utils/utilities.sh b/hermetic_build/library_generation/utils/utilities.sh
diff --git a/showcase/scripts/generate_showcase.sh b/showcase/scripts/generate_showcase.sh

Original file line number	Diff line number	Diff line change
`@@ -309,8 +309,6 @@ def __get_a_gen_config(`
`309`	`309`	`return GenerationConfig(`
`310`	`310`	`gapic_generator_version="",`
`311`	`311`	`googleapis_commitish=googleapis_commitish,`
`312`		`- grpc_version="",`
`313`		`- protoc_version="",`
`314`	`312`	`libraries=libraries,`
`315`	`313`	`)`
`316`	`314`
Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,6 @@ def test_generation_config_set_generator_version_from_env(self):`
`74`	`74`	`def test_from_yaml_succeeds(self):`
`75`	`75`	`config = from_yaml(f"{test_config_dir}/generation_config.yaml")`
`76`	`76`	`self.assertEqual("2.34.0", config.gapic_generator_version)`
`77`		`- self.assertEqual(25.2, config.protoc_version)`
`78`	`77`	`self.assertEqual(`
`79`	`78`	`"1a45bf7393b52407188c82e63101db7dc9c72026", config.googleapis_commitish`
`80`	`79`	`)`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`gapic_generator_version: 2.34.0`
`2`		`-protoc_version: 25.2`
`3`	`2`	`googleapis_commitish: 1a45bf7393b52407188c82e63101db7dc9c72026`
`4`	`3`	`libraries_bom_version: 26.37.0`
`5`	`4`	`libraries:`