zephyr: zmake: Only re-run cmake when required

Prior to this commit, when `zmake configure` was run on an existing
build directory, the build directory would always clobber.  Since most
people are building by using "zmake configure -b ${PROJECT_NAME}" each
time, this means that we almost never can do incremental builds.

Most of the time we can actually save the existing cmake configuration
when running configure however, but we do have to be careful to not
save it too often: a change of configuration options such as the
toolchain could mean that the build directory really does need
clobbered.

This CL adds logic to clobber build directories only when necessary by
an actual configuration difference.  We do so by serializing the build
configuration into a JSON file at ${BUILD_DIR}/cfg-{ro,rw}.json after
a successful run of cmake.  Then, before running cmake each time, if
this JSON file exists and has the same build configuration we're about
to run cmake for, we simply skip running cmake entirely.

BUG=b:214321770
BRANCH=none
TEST="zmake configure -b volteer", then do it again.  Second run only
     recompiles version file and then links.

Change-Id: Iee7e6a15f36af9dc398a8a795884bf42b1a38833
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3384403
Reviewed-by: Jeremy Bettis <jbettis@chromium.org>
Reviewed-by: Keith Short <keithshort@chromium.org>

6 files changed, 126 insertions, 11 deletions

diff --git a/zephyr/zmake/README.md b/zephyr/zmake/README.md
index 986cc8a506..d3f99fe753 100644
--- a/zephyr/zmake/README.md
+++ b/zephyr/zmake/README.md
@@ -34,7 +34,7 @@ Chromium OS's meta-build tool for Zephyr
 
 ### zmake configure
 
-**Usage:** `zmake configure [-h] [-t TOOLCHAIN] [--bringup] [--allow-warnings] [-B BUILD_DIR] [-b] [--test] project_name_or_dir [-c]`
+**Usage:** `zmake configure [-h] [-t TOOLCHAIN] [--bringup] [--clobber] [--allow-warnings] [-B BUILD_DIR] [-b] [--test] project_name_or_dir [-c]`
 
 #### Positional Arguments
 
@@ -49,6 +49,7 @@ Chromium OS's meta-build tool for Zephyr
 | `-h`, `--help` | show this help message and exit |
 | `-t TOOLCHAIN`, `--toolchain TOOLCHAIN` | Name of toolchain to use |
 | `--bringup` | Enable bringup debugging features |
+| `--clobber` | Delete existing build directories, even if configuration is unchanged |
 | `--allow-warnings` | Do not treat warnings as errors |
 | `-B BUILD_DIR`, `--build-dir BUILD_DIR` | Build directory |
 | `-b`, `--build` | Run the build after configuration |
@@ -107,13 +108,14 @@ Chromium OS's meta-build tool for Zephyr
 
 ### zmake testall
 
-**Usage:** `zmake testall [-h]`
+**Usage:** `zmake testall [-h] [--clobber]`
 
 #### Optional Arguments
 
 |   |   |
 |---|---|
 | `-h`, `--help` | show this help message and exit |
+| `--clobber` | Delete existing build directories, even if configuration is unchanged |
 
 ### zmake coverage
 
diff --git a/zephyr/zmake/tests/test_build_config.py b/zephyr/zmake/tests/test_build_config.py
index bf69b8a1fa..ba81bd480d 100644
--- a/zephyr/zmake/tests/test_build_config.py
+++ b/zephyr/zmake/tests/test_build_config.py
@@ -200,3 +200,57 @@ def test_popen_cmake_kconfig(conf, project_dir, build_dir):
         assert kconfig_defs == conf.kconfig_defs
     finally:
         os.unlink(temp_path)
+
+
+def test_build_config_json_stability():
+    # as_json() should return equivalent strings for two equivalent
+    # build configs.
+    a = BuildConfig(
+        environ_defs={
+            "A": "B",
+            "B": "C",
+        },
+        cmake_defs={
+            "Z": "Y",
+            "X": "W",
+        },
+        kconfig_defs={
+            "CONFIG_A": "y",
+            "CONFIG_B": "n",
+        },
+        kconfig_files=[
+            pathlib.Path("/a/b/c.conf"),
+            pathlib.Path("d/e/f.conf"),
+        ],
+    )
+
+    # Dict ordering is intentionally reversed in b.
+    b = BuildConfig(
+        environ_defs={
+            "B": "C",
+            "A": "B",
+        },
+        cmake_defs={
+            "X": "W",
+            "Z": "Y",
+        },
+        kconfig_defs={
+            "CONFIG_B": "n",
+            "CONFIG_A": "y",
+        },
+        kconfig_files=[
+            pathlib.Path("/a/b/c.conf"),
+            pathlib.Path("d/e/f.conf"),
+        ],
+    )
+
+    assert a.as_json() == b.as_json()
+
+
+def test_build_config_json_inequality():
+    # Two differing build configs should not have the same json
+    # representation.
+    a = BuildConfig(cmake_defs={"A": "B"})
+    b = BuildConfig(environ_defs={"A": "B"})
+
+    assert a.as_json() != b.as_json()
diff --git a/zephyr/zmake/tests/test_zmake.py b/zephyr/zmake/tests/test_zmake.py
index 83862ce65d..f2759e5af3 100644
--- a/zephyr/zmake/tests/test_zmake.py
+++ b/zephyr/zmake/tests/test_zmake.py
@@ -155,7 +155,9 @@ EXTRAVERSION =
                     return_value={"fakeproject": FakeProject()},
                 ):
                     if use_configure:
-                        zmk.configure("fakeproject", build_dir=pathlib.Path("build"))
+                        zmk.configure(
+                            "fakeproject", build_dir=pathlib.Path("build"), clobber=True
+                        )
                     else:
                         with patch("zmake.version.write_version_header", autospec=True):
                             zmk.build(pathlib.Path(tmpname))
diff --git a/zephyr/zmake/zmake/__main__.py b/zephyr/zmake/zmake/__main__.py
index 28ec232148..d8f33a97d9 100644
--- a/zephyr/zmake/zmake/__main__.py
+++ b/zephyr/zmake/zmake/__main__.py
@@ -176,6 +176,12 @@ def get_argparser():
         help="Enable bringup debugging features",
     )
     configure.add_argument(
+        "--clobber",
+        action="store_true",
+        dest="clobber",
+        help="Delete existing build directories, even if configuration is unchanged",
+    )
+    configure.add_argument(
         "--allow-warnings",
         action="store_true",
         default=False,
@@ -254,10 +260,16 @@ def get_argparser():
         help="The build directory used during configuration",
     )
 
-    sub.add_parser(
+    testall = sub.add_parser(
         "testall",
         help="Execute all known builds and tests",
     )
+    testall.add_argument(
+        "--clobber",
+        action="store_true",
+        dest="clobber",
+        help="Delete existing build directories, even if configuration is unchanged",
+    )
 
     coverage = sub.add_parser(
         "coverage",
diff --git a/zephyr/zmake/zmake/build_config.py b/zephyr/zmake/zmake/build_config.py
index 9a9c7f36a2..85e06d8f11 100644
--- a/zephyr/zmake/zmake/build_config.py
+++ b/zephyr/zmake/zmake/build_config.py
@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 """Encapsulation of a build configuration."""
 
+import json
 
 import zmake.util as util
 
@@ -98,3 +99,15 @@ class BuildConfig:
                 if getattr(self, name)
             )
         )
+
+    def as_json(self):
+        """Provide a stable JSON representation of the build config."""
+        return json.dumps(
+            {
+                "environ_defs": self.environ_defs,
+                "cmake_defs": self.cmake_defs,
+                "kconfig_defs": self.kconfig_defs,
+                "kconfig_files": [str(p.resolve()) for p in self.kconfig_files],
+            },
+            sort_keys=True,
+        )
diff --git a/zephyr/zmake/zmake/zmake.py b/zephyr/zmake/zmake/zmake.py
index abc41de855..47a79f179b 100644
--- a/zephyr/zmake/zmake/zmake.py
+++ b/zephyr/zmake/zmake/zmake.py
@@ -193,6 +193,7 @@ class Zmake:
         toolchain=None,
         build_after_configure=False,
         test_after_configure=False,
+        clobber=False,
         bringup=False,
         coverage=False,
         allow_warnings=False,
@@ -217,6 +218,7 @@ class Zmake:
             toolchain=toolchain,
             build_after_configure=build_after_configure,
             test_after_configure=test_after_configure,
+            clobber=clobber,
             bringup=bringup,
             coverage=coverage,
             allow_warnings=allow_warnings,
@@ -229,6 +231,7 @@ class Zmake:
         toolchain=None,
         build_after_configure=False,
         test_after_configure=False,
+        clobber=False,
         bringup=False,
         coverage=False,
         allow_warnings=False,
@@ -242,9 +245,10 @@ class Zmake:
                 / "zephyr"
                 / project.config.project_name
             )
-        # Make sure the build directory is clean.
-        if os.path.exists(build_dir):
-            self.logger.info("Clearing old build directory %s", build_dir)
+
+        # Clobber build directory if requested.
+        if clobber and build_dir.exists():
+            self.logger.info("Clearing build directory %s due to --clobber", build_dir)
             shutil.rmtree(build_dir)
 
         generated_include_dir = (build_dir / "include").resolve()
@@ -294,11 +298,9 @@ class Zmake:
         if not generated_include_dir.exists():
             generated_include_dir.mkdir()
         processes = []
+        files_to_write = []
         self.logger.info("Building %s in %s.", project.config.project_name, build_dir)
         for build_name, build_config in project.iter_builds():
-            self.logger.info(
-                "Configuring %s:%s.", project.config.project_name, build_name
-            )
             config = (
                 base_config
                 | toolchain_config
@@ -306,7 +308,33 @@ class Zmake:
                 | dts_overlay_config
                 | build_config
             )
+
+            config_json = config.as_json()
+            config_json_file = build_dir / f"cfg-{build_name}.json"
+            if config_json_file.is_file():
+                if config_json_file.read_text() == config_json:
+                    self.logger.info(
+                        "Skip reconfiguring %s:%s due to previous cmake run of "
+                        "equivalent configuration.  Run with --clobber if this "
+                        "optimization is undesired.",
+                        project.config.project_name,
+                        build_name,
+                    )
+                    continue
+                else:
+                    config_json_file.unlink()
+
+            files_to_write.append((config_json_file, config_json))
+
             output_dir = build_dir / "build-{}".format(build_name)
+            if output_dir.exists():
+                self.logger.info("Clobber %s due to configuration changes.", output_dir)
+                shutil.rmtree(output_dir)
+
+            self.logger.info(
+                "Configuring %s:%s.", project.config.project_name, build_name
+            )
+
             kconfig_file = build_dir / "kconfig-{}.conf".format(build_name)
             proc = config.popen_cmake(
                 self.jobserver,
@@ -343,6 +371,9 @@ class Zmake:
             if proc.wait():
                 raise OSError(get_process_failure_msg(proc))
 
+        for path, contents in files_to_write:
+            path.write_text(contents)
+
         # To reconstruct a Project object later, we need to know the
         # name and project directory.
         (build_dir / "project_name.txt").write_text(project.config.project_name)
@@ -531,7 +562,7 @@ class Zmake:
                 raise OSError(get_process_failure_msg(proc))
         return 0
 
-    def testall(self):
+    def testall(self, clobber=False):
         """Test all the valid test targets"""
         tmp_dirs = []
         for project in zmake.project.find_projects(
@@ -550,6 +581,7 @@ class Zmake:
                     build_dir=pathlib.Path(temp_build_dir),
                     build_after_configure=True,
                     test_after_configure=is_test,
+                    clobber=clobber,
                 )
             )