From 1567296888895bf349e23dfb15e87eb714201301 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 14 Oct 2022 12:35:30 -0400 Subject: [PATCH] Move codegen.py invoke logic into a gni template (#23154) * Move codegen logic into a standalone gni template * Remove extra hadcoded path in codegen.gni * Remove extra hadcoded path in codegen.gni * Use common codegen instruction for bridge as well * Make sure that the codegen include is a public target, so that includes work for generated code * Restyle * Enforce that outputs in the codegen are known rather than dynamic * Apply some code review comments * rephrased a comment based on code review * Restyle * Stop using exec_script in gn (faster gn) * Fix path rebasing to get valid paths * Restyle * make sure that generated expected outputs is one of the expected input files for the codegen script * Add documentation on how to get the list of expected files * Add missing init call in the test storage impl --- build/chip/chip_codegen.gni | 95 ++++++++++++ examples/dynamic-bridge-app/linux/BUILD.gn | 30 ++-- scripts/codegen.py | 35 ++++- scripts/idl/generators/__init__.py | 18 ++- scripts/idl/test_generators.py | 1 + src/controller/data_model/BUILD.gn | 167 +++++++++++++++++---- 6 files changed, 294 insertions(+), 52 deletions(-) create mode 100644 build/chip/chip_codegen.gni diff --git a/build/chip/chip_codegen.gni b/build/chip/chip_codegen.gni new file mode 100644 index 00000000000000..bab79934bd37f5 --- /dev/null +++ b/build/chip/chip_codegen.gni @@ -0,0 +1,95 @@ +# Copyright (c) 2022 Project CHIP Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import("//build_overrides/build.gni") +import("//build_overrides/chip.gni") +import("//build_overrides/pigweed.gni") + +import("$dir_pw_build/python.gni") + +# Defines a target that runs code generation based on +# scripts/codegen.py +# +# Arguments: +# input +# The ".matter" file to use to start the code generation +# +# generator +# Name of the generator to use (e.g. java, cpp-app) +# +# outputs +# Explicit names of the expected outputs. Enforced to validate that +# expected outputs are generated when processing input files. +# +# NOTE: content of "outputs" is verified to match the output of codegen.py +# exactly. It is not inferred on purpose, to make build-rules explicit +# and verifiable (even though codege.py can at runtime report its outputs) +# +# To find the list of generated files, you can run codegen.py with the +# "--name-only" argument +# +# Example usage: +# +# chip_codegen("java-jni-generate") { +# input = "controller-clusters.matter" +# generator = "java" +# +# outputs = [ +# "jni/IdentifyClient-ReadImpl.cpp", +# "jni/IdentifyClient-InvokeSubscribeImpl.cpp", +# # ... more to follow +# ] +# } +# +template("chip_codegen") { + _name = target_name + _generator = invoker.generator + + config("${_name}_config") { + include_dirs = [ target_gen_dir ] + } + + pw_python_action(_name) { + script = "${chip_root}/scripts/codegen.py" + + _idl_file = invoker.input + _expected_outputs = "${target_gen_dir}/${_name}.expected.outputs" + + write_file(_expected_outputs, invoker.outputs, "list lines") + + args = [ + "--generator", + _generator, + "--output-dir", + rebase_path(target_gen_dir, root_build_dir), + "--expected-outputs", + rebase_path(_expected_outputs, root_build_dir), + rebase_path(_idl_file, root_build_dir), + ] + + deps = [ "${chip_root}/scripts/idl" ] + public_configs = [ ":${_name}_config" ] + + inputs = [ + _idl_file, + _expected_outputs, + ] + sources = [ _idl_file ] + + outputs = [] + foreach(name, invoker.outputs) { + outputs += [ "${target_gen_dir}/${name}" ] + } + } +} diff --git a/examples/dynamic-bridge-app/linux/BUILD.gn b/examples/dynamic-bridge-app/linux/BUILD.gn index 03eee3e08a3f33..1f4bda053ce30f 100644 --- a/examples/dynamic-bridge-app/linux/BUILD.gn +++ b/examples/dynamic-bridge-app/linux/BUILD.gn @@ -15,6 +15,7 @@ import("//build_overrides/build.gni") import("//build_overrides/chip.gni") +import("${chip_root}/build/chip/chip_codegen.gni") import("${chip_root}/build/chip/tools.gni") import("${chip_root}/src/app/common_flags.gni") @@ -27,24 +28,19 @@ if (chip_enable_pw_rpc) { assert(chip_build_tools) -action("chip-bridge-codegen") { - script = "${chip_root}/scripts/codegen.py" - sources = - [ "${chip_root}/examples/bridge-app/bridge-common/bridge-app.matter" ] - - # Also several other files, but this is sufficient for dependency purposes. - outputs = [ "$target_gen_dir/bridge/BridgeClustersImpl.h" ] - - args = [ - "--generator", - "bridge", - "--output-dir", - rebase_path(target_gen_dir, root_build_dir), - rebase_path( - "${chip_root}/examples/dynamic-bridge-app/bridge-common/bridge-app.matter", - root_build_dir), +chip_codegen("chip-bridge-codegen") { + input = "${chip_root}/examples/bridge-app/bridge-common/bridge-app.matter" + generator = "bridge" + + outputs = [ + "bridge/OnOff.h", + "bridge/LevelControl.h", + "bridge/Descriptor.h", + "bridge/Switch.h", + "bridge/TemperatureMeasurement.h", + "bridge/BridgeClustersImpl.h", + "bridge/BridgeGlobalStructs.h", ] - deps = [ "${chip_root}/scripts/idl" ] } executable("dynamic-chip-bridge-app") { diff --git a/scripts/codegen.py b/scripts/codegen.py index 3eb6acd047f760..bcbba6833a49b4 100755 --- a/scripts/codegen.py +++ b/scripts/codegen.py @@ -17,12 +17,12 @@ import logging import coloredlogs import enum +import sys try: from idl.matter_idl_parser import CreateParser except: import os - import sys sys.path.append(os.path.abspath(os.path.dirname(__file__))) from idl.matter_idl_parser import CreateParser @@ -54,6 +54,9 @@ class ListGeneratedFilesStorage(GeneratorStorage): A storage that prints out file names that would have content in them. """ + def __init__(self): + super().__init__() + def get_existing_data(self, relative_path: str): return None # stdout has no pre-existing data @@ -102,10 +105,15 @@ def write_new_data(self, relative_path: str, content: str): default=False, is_flag=True, help='Output just a list of file names that would be generated') +@click.option( + '--expected-outputs', + type=click.Path(exists=True), + default=None, + help='A file containing all expected outputs. Script will fail if outputs do not match') @click.argument( 'idl_path', type=click.Path(exists=True)) -def main(log_level, generator, output_dir, dry_run, name_only, idl_path): +def main(log_level, generator, output_dir, dry_run, name_only, expected_outputs, idl_path): """ Parses MATTER IDL files (.matter) and performs SDK code generation as set up by the program arguments. @@ -124,6 +132,29 @@ def main(log_level, generator, output_dir, dry_run, name_only, idl_path): generator = __GENERATORS__[ generator].CreateGenerator(storage, idl=idl_tree) generator.render(dry_run) + + if expected_outputs: + with open(expected_outputs, 'rt') as fin: + expected = set() + for l in fin.readlines(): + l = l.strip() + if l: + expected.add(l) + + if expected != storage.generated_paths: + logging.fatal("expected and generated files do not match.") + + extra = storage.generated_paths - expected + missing = expected - storage.generated_paths + + for name in extra: + logging.fatal(" '%s' was generated but not expected" % name) + + for name in missing: + logging.fatal(" '%s' was expected but not generated" % name) + + sys.exit(1) + logging.info("Done") diff --git a/scripts/idl/generators/__init__.py b/scripts/idl/generators/__init__.py index 308aaf603f005b..46b7b6b675bde7 100644 --- a/scripts/idl/generators/__init__.py +++ b/scripts/idl/generators/__init__.py @@ -25,12 +25,22 @@ class GeneratorStorage: """ - Handles file operations for generator output. Specificall can create + Handles file operations for generator output. Specifically can create required files for output. Is overriden for unit tests. """ + def __init__(self): + self._generated_paths = set() + + @property + def generated_paths(self): + return self._generated_paths + + def report_output_file(self, relative_path: str): + self._generated_paths.add(relative_path) + def get_existing_data(self, relative_path: str): """Gets the existing data at the given path. If such data does not exist, will return None. @@ -49,6 +59,7 @@ class FileSystemGeneratorStorage(GeneratorStorage): """ def __init__(self, output_dir: str): + super().__init__() self.output_dir = output_dir def get_existing_data(self, relative_path: str): @@ -148,6 +159,11 @@ def internal_render_one_output(self, template_path: str, output_file_name: str, rendered = self.jinja_env.get_template(template_path).render(vars) + # Report regardless if it has changed or not. This is because even if + # files are unchanged, validation of what the correct output is should + # still be done. + self.storage.report_output_file(output_file_name) + if rendered == self.storage.get_existing_data(output_file_name): logging.info("File content not changed") else: diff --git a/scripts/idl/test_generators.py b/scripts/idl/test_generators.py index 0282dd8e9c3afe..ed65bc242fa34e 100755 --- a/scripts/idl/test_generators.py +++ b/scripts/idl/test_generators.py @@ -58,6 +58,7 @@ def add_outputs(self, yaml_outputs_dict): class TestCaseStorage(GeneratorStorage): def __init__(self, test_case: GeneratorTestCase, checker: unittest.TestCase): + super().__init__() self.test_case = test_case self.checker = checker self.checked_files = set() diff --git a/src/controller/data_model/BUILD.gn b/src/controller/data_model/BUILD.gn index d9665b4aaecafe..9403f04d34dd6e 100644 --- a/src/controller/data_model/BUILD.gn +++ b/src/controller/data_model/BUILD.gn @@ -17,6 +17,7 @@ import("//build_overrides/chip.gni") import("//build_overrides/pigweed.gni") import("$dir_pw_build/python.gni") +import("${chip_root}/build/chip/chip_codegen.gni") import("${chip_root}/src/app/chip_data_model.gni") chip_data_model("data_model") { @@ -29,40 +30,142 @@ chip_data_model("data_model") { } if (current_os == "android") { - pw_python_action("java-jni-generate") { - script = "${chip_root}/scripts/codegen.py" + chip_codegen("java-jni-generate") { + input = "controller-clusters.matter" + generator = "java" - _idl_file = "controller-clusters.matter" - _output_files = - exec_script("${chip_root}/scripts/codegen.py", - [ - "--generator", - "java", - "--log-level", - "fatal", - "--name-only", - rebase_path("controller-clusters.matter", root_build_dir), - ], - "list lines", - [ "controller-clusters.matter" ]) - - args = [ - "--generator", - "java", - "--output-dir", - rebase_path(target_gen_dir, root_build_dir), - rebase_path(_idl_file, root_build_dir), + outputs = [ + "jni/IdentifyClient-ReadImpl.cpp", + "jni/IdentifyClient-InvokeSubscribeImpl.cpp", + "jni/GroupsClient-ReadImpl.cpp", + "jni/GroupsClient-InvokeSubscribeImpl.cpp", + "jni/ScenesClient-ReadImpl.cpp", + "jni/ScenesClient-InvokeSubscribeImpl.cpp", + "jni/OnOffClient-ReadImpl.cpp", + "jni/OnOffClient-InvokeSubscribeImpl.cpp", + "jni/OnOffSwitchConfigurationClient-ReadImpl.cpp", + "jni/OnOffSwitchConfigurationClient-InvokeSubscribeImpl.cpp", + "jni/LevelControlClient-ReadImpl.cpp", + "jni/LevelControlClient-InvokeSubscribeImpl.cpp", + "jni/BinaryInputBasicClient-ReadImpl.cpp", + "jni/BinaryInputBasicClient-InvokeSubscribeImpl.cpp", + "jni/DescriptorClient-ReadImpl.cpp", + "jni/DescriptorClient-InvokeSubscribeImpl.cpp", + "jni/BindingClient-ReadImpl.cpp", + "jni/BindingClient-InvokeSubscribeImpl.cpp", + "jni/AccessControlClient-ReadImpl.cpp", + "jni/AccessControlClient-InvokeSubscribeImpl.cpp", + "jni/ActionsClient-ReadImpl.cpp", + "jni/ActionsClient-InvokeSubscribeImpl.cpp", + "jni/BasicClient-ReadImpl.cpp", + "jni/BasicClient-InvokeSubscribeImpl.cpp", + "jni/OtaSoftwareUpdateProviderClient-ReadImpl.cpp", + "jni/OtaSoftwareUpdateProviderClient-InvokeSubscribeImpl.cpp", + "jni/OtaSoftwareUpdateRequestorClient-ReadImpl.cpp", + "jni/OtaSoftwareUpdateRequestorClient-InvokeSubscribeImpl.cpp", + "jni/LocalizationConfigurationClient-ReadImpl.cpp", + "jni/LocalizationConfigurationClient-InvokeSubscribeImpl.cpp", + "jni/TimeFormatLocalizationClient-ReadImpl.cpp", + "jni/TimeFormatLocalizationClient-InvokeSubscribeImpl.cpp", + "jni/UnitLocalizationClient-ReadImpl.cpp", + "jni/UnitLocalizationClient-InvokeSubscribeImpl.cpp", + "jni/PowerSourceConfigurationClient-ReadImpl.cpp", + "jni/PowerSourceConfigurationClient-InvokeSubscribeImpl.cpp", + "jni/PowerSourceClient-ReadImpl.cpp", + "jni/PowerSourceClient-InvokeSubscribeImpl.cpp", + "jni/GeneralCommissioningClient-ReadImpl.cpp", + "jni/GeneralCommissioningClient-InvokeSubscribeImpl.cpp", + "jni/NetworkCommissioningClient-ReadImpl.cpp", + "jni/NetworkCommissioningClient-InvokeSubscribeImpl.cpp", + "jni/DiagnosticLogsClient-ReadImpl.cpp", + "jni/DiagnosticLogsClient-InvokeSubscribeImpl.cpp", + "jni/GeneralDiagnosticsClient-ReadImpl.cpp", + "jni/GeneralDiagnosticsClient-InvokeSubscribeImpl.cpp", + "jni/SoftwareDiagnosticsClient-ReadImpl.cpp", + "jni/SoftwareDiagnosticsClient-InvokeSubscribeImpl.cpp", + "jni/ThreadNetworkDiagnosticsClient-ReadImpl.cpp", + "jni/ThreadNetworkDiagnosticsClient-InvokeSubscribeImpl.cpp", + "jni/WiFiNetworkDiagnosticsClient-ReadImpl.cpp", + "jni/WiFiNetworkDiagnosticsClient-InvokeSubscribeImpl.cpp", + "jni/EthernetNetworkDiagnosticsClient-ReadImpl.cpp", + "jni/EthernetNetworkDiagnosticsClient-InvokeSubscribeImpl.cpp", + "jni/BridgedDeviceBasicClient-ReadImpl.cpp", + "jni/BridgedDeviceBasicClient-InvokeSubscribeImpl.cpp", + "jni/SwitchClient-ReadImpl.cpp", + "jni/SwitchClient-InvokeSubscribeImpl.cpp", + "jni/AdministratorCommissioningClient-ReadImpl.cpp", + "jni/AdministratorCommissioningClient-InvokeSubscribeImpl.cpp", + "jni/OperationalCredentialsClient-ReadImpl.cpp", + "jni/OperationalCredentialsClient-InvokeSubscribeImpl.cpp", + "jni/GroupKeyManagementClient-ReadImpl.cpp", + "jni/GroupKeyManagementClient-InvokeSubscribeImpl.cpp", + "jni/FixedLabelClient-ReadImpl.cpp", + "jni/FixedLabelClient-InvokeSubscribeImpl.cpp", + "jni/UserLabelClient-ReadImpl.cpp", + "jni/UserLabelClient-InvokeSubscribeImpl.cpp", + "jni/BooleanStateClient-ReadImpl.cpp", + "jni/BooleanStateClient-InvokeSubscribeImpl.cpp", + "jni/ModeSelectClient-ReadImpl.cpp", + "jni/ModeSelectClient-InvokeSubscribeImpl.cpp", + "jni/DoorLockClient-ReadImpl.cpp", + "jni/DoorLockClient-InvokeSubscribeImpl.cpp", + "jni/WindowCoveringClient-ReadImpl.cpp", + "jni/WindowCoveringClient-InvokeSubscribeImpl.cpp", + "jni/BarrierControlClient-ReadImpl.cpp", + "jni/BarrierControlClient-InvokeSubscribeImpl.cpp", + "jni/PumpConfigurationAndControlClient-ReadImpl.cpp", + "jni/PumpConfigurationAndControlClient-InvokeSubscribeImpl.cpp", + "jni/ThermostatClient-ReadImpl.cpp", + "jni/ThermostatClient-InvokeSubscribeImpl.cpp", + "jni/FanControlClient-ReadImpl.cpp", + "jni/FanControlClient-InvokeSubscribeImpl.cpp", + "jni/ThermostatUserInterfaceConfigurationClient-ReadImpl.cpp", + "jni/ThermostatUserInterfaceConfigurationClient-InvokeSubscribeImpl.cpp", + "jni/ColorControlClient-ReadImpl.cpp", + "jni/ColorControlClient-InvokeSubscribeImpl.cpp", + "jni/BallastConfigurationClient-ReadImpl.cpp", + "jni/BallastConfigurationClient-InvokeSubscribeImpl.cpp", + "jni/IlluminanceMeasurementClient-ReadImpl.cpp", + "jni/IlluminanceMeasurementClient-InvokeSubscribeImpl.cpp", + "jni/TemperatureMeasurementClient-ReadImpl.cpp", + "jni/TemperatureMeasurementClient-InvokeSubscribeImpl.cpp", + "jni/PressureMeasurementClient-ReadImpl.cpp", + "jni/PressureMeasurementClient-InvokeSubscribeImpl.cpp", + "jni/FlowMeasurementClient-ReadImpl.cpp", + "jni/FlowMeasurementClient-InvokeSubscribeImpl.cpp", + "jni/RelativeHumidityMeasurementClient-ReadImpl.cpp", + "jni/RelativeHumidityMeasurementClient-InvokeSubscribeImpl.cpp", + "jni/OccupancySensingClient-ReadImpl.cpp", + "jni/OccupancySensingClient-InvokeSubscribeImpl.cpp", + "jni/WakeOnLanClient-ReadImpl.cpp", + "jni/WakeOnLanClient-InvokeSubscribeImpl.cpp", + "jni/ChannelClient-ReadImpl.cpp", + "jni/ChannelClient-InvokeSubscribeImpl.cpp", + "jni/TargetNavigatorClient-ReadImpl.cpp", + "jni/TargetNavigatorClient-InvokeSubscribeImpl.cpp", + "jni/MediaPlaybackClient-ReadImpl.cpp", + "jni/MediaPlaybackClient-InvokeSubscribeImpl.cpp", + "jni/MediaInputClient-ReadImpl.cpp", + "jni/MediaInputClient-InvokeSubscribeImpl.cpp", + "jni/LowPowerClient-ReadImpl.cpp", + "jni/LowPowerClient-InvokeSubscribeImpl.cpp", + "jni/KeypadInputClient-ReadImpl.cpp", + "jni/KeypadInputClient-InvokeSubscribeImpl.cpp", + "jni/ContentLauncherClient-ReadImpl.cpp", + "jni/ContentLauncherClient-InvokeSubscribeImpl.cpp", + "jni/AudioOutputClient-ReadImpl.cpp", + "jni/AudioOutputClient-InvokeSubscribeImpl.cpp", + "jni/ApplicationLauncherClient-ReadImpl.cpp", + "jni/ApplicationLauncherClient-InvokeSubscribeImpl.cpp", + "jni/ApplicationBasicClient-ReadImpl.cpp", + "jni/ApplicationBasicClient-InvokeSubscribeImpl.cpp", + "jni/AccountLoginClient-ReadImpl.cpp", + "jni/AccountLoginClient-InvokeSubscribeImpl.cpp", + "jni/ElectricalMeasurementClient-ReadImpl.cpp", + "jni/ElectricalMeasurementClient-InvokeSubscribeImpl.cpp", + "jni/TestClusterClient-ReadImpl.cpp", + "jni/TestClusterClient-InvokeSubscribeImpl.cpp", ] - - deps = [ "${chip_root}/scripts/idl" ] - - inputs = [ _idl_file ] - sources = [ _idl_file ] - outputs = [] - - foreach(name, _output_files) { - outputs += [ "$target_gen_dir/${name}" ] - } } source_set("java-jni-sources") {