zmake: Trim short version on full hashes

The ec_version.h contains both a long build string and also a 31 char
string. Change the short string to omit the commit count, and only
include full hashes, i.e. hayato_v2.6-ec:5bd1aa,os:ade7b4 not
hayato_v2.6.73670-ec:5bd1aa,os:

Change test to be a real version string with hashes of the right length.

BUG=None
BRANCH=None
TEST=./zephyr/zmake/run_tests.sh

Change-Id: I18d73345036933594b8c74187796c3727f868629
Signed-off-by: Jeremy Bettis <jbettis@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3160464
Tested-by: Jeremy Bettis <jbettis@chromium.org>
Auto-Submit: Jeremy Bettis <jbettis@chromium.org>
Commit-Queue: Jack Rosenthal <jrosenth@chromium.org>
Reviewed-by: Jack Rosenthal <jrosenth@chromium.org>

4 files changed, 104 insertions, 41 deletions

diff --git a/zephyr/zmake/tests/test_version.py b/zephyr/zmake/tests/test_version.py
index a238a8ac02..b9d2ec0d4b 100644
--- a/zephyr/zmake/tests/test_version.py
+++ b/zephyr/zmake/tests/test_version.py
@@ -88,17 +88,17 @@ def _setup_example_repos(tmp_path):
 
 def test_version_string(tmp_path):
     project, zephyr_base, modules = _setup_example_repos(tmp_path)
-    assert (
-        version.get_version_string(project, zephyr_base, modules)
-        == "prj_v2.6.4-ec:b5991f,os:377d26,mod1:02fd7a"
+    assert version.get_version_string(project, zephyr_base, modules) == (
+        "prj_v2.6.4-ec:b5991f,os:377d26,mod1:02fd7a",
+        "prj_v2.6-ec:b5991f,os:377d26",
     )
 
 
 def test_version_string_static(tmp_path):
     project, zephyr_base, modules = _setup_example_repos(tmp_path)
-    assert (
-        version.get_version_string(project, zephyr_base, modules, static=True)
-        == "prj_v2.6.0-STATIC"
+    assert version.get_version_string(project, zephyr_base, modules, static=True) == (
+        "prj_v2.6.0-STATIC",
+        "prj_v2.6.0-STATIC",
     )
 
 
@@ -117,17 +117,36 @@ def fake_date():
         yield
 
 
-HEADER_VERSION_STR = "trogdor_v2.6.1004-cmsis:0dead0,hal_stm32:0beef0,os:ad00da"
+HEADER_VERSION_STR = (
+    "hayato_v2.6.73670-ec:5bd1aa,os:ade7b4,cmsis:c3bd20,hal_stm32:f8ff8d"
+)
+SHORT_HEADER_VERSION_STR = "hayato_v2.6-ec:5bd1aa,os:ade7b4"
 EXPECTED_HEADER = (
     "/* This file is automatically generated by zmake */\n"
-    '#define VERSION "trogdor_v2.6.1004-cmsis:0dead0,hal_stm32:0beef0,os:ad00da"\n'
-    '#define CROS_EC_VERSION32 "trogdor_v2.6.1004-cmsis:0dead0,"\n'
+    '#define VERSION "hayato_v2.6.73670-ec:5bd1aa,os:ade7b4,cmsis:c3bd20,'
+    'hal_stm32:f8ff8d"\n'
+    '#define CROS_EC_VERSION32 "hayato_v2.6-ec:5bd1aa,os:ade7b4"\n'
+    '#define BUILDER "toukmond@pokey"\n'
+    '#define DATE "2021-06-28 03:18:53"\n'
+    '#define CROS_FWID_MISSING_STR "CROS_FWID_MISSING"\n'
+    "#define CROS_FWID32 CROS_FWID_MISSING_STR\n"
+)
+HEADER_VERSION_STR2 = (
+    "herobrine_npcx9_v2.6.73695-ec:7b0169,os:862339,cmsis:c3bd20,hal_stm32:f8ff8d"
+)
+SHORT_HEADER_VERSION_STR2 = "herobrine_npcx9_v2.6-ec:7b0169"
+EXPECTED_HEADER2 = (
+    "/* This file is automatically generated by zmake */\n"
+    '#define VERSION "herobrine_npcx9_v2.6.73695-ec:7b0169,os:862339,'
+    'cmsis:c3bd20,hal_stm32:f8ff8d"\n'
+    '#define CROS_EC_VERSION32 "herobrine_npcx9_v2.6-ec:7b0169"\n'
     '#define BUILDER "toukmond@pokey"\n'
     '#define DATE "2021-06-28 03:18:53"\n'
     '#define CROS_FWID_MISSING_STR "CROS_FWID_MISSING"\n'
     "#define CROS_FWID32 CROS_FWID_MISSING_STR\n"
 )
 HEADER_VERSION_STR_STATIC = "trogdor_v2.6.0-STATIC"
+SHORT_HEADER_VERSION_STR_STATIC = "trogdor_v2.6.0-STATIC"
 EXPECTED_HEADER_STATIC = (
     "/* This file is automatically generated by zmake */\n"
     '#define VERSION "trogdor_v2.6.0-STATIC"\n'
@@ -139,18 +158,32 @@ EXPECTED_HEADER_STATIC = (
 )
 
 
-def test_header_gen(fake_user_hostname, fake_date, tmp_path):
+@pytest.mark.parametrize(
+    "version_str,short_version_str,expected_header,static",
+    [
+        (HEADER_VERSION_STR, SHORT_HEADER_VERSION_STR, EXPECTED_HEADER, False),
+        (HEADER_VERSION_STR2, SHORT_HEADER_VERSION_STR2, EXPECTED_HEADER2, False),
+        (
+            HEADER_VERSION_STR_STATIC,
+            SHORT_HEADER_VERSION_STR_STATIC,
+            EXPECTED_HEADER_STATIC,
+            True,
+        ),
+    ],
+)
+def test_header_gen(
+    fake_user_hostname,
+    fake_date,
+    tmp_path,
+    version_str,
+    short_version_str,
+    expected_header,
+    static,
+):
     # Test the simple case (static=False, no existing header).
     output_file = tmp_path / "ec_version.h"
-    version.write_version_header(HEADER_VERSION_STR, output_file)
-    assert output_file.read_text() == EXPECTED_HEADER
-
-
-def test_header_gen_reproducible_build(tmp_path):
-    # With static=True this time.
-    output_file = tmp_path / "ec_version.h"
-    version.write_version_header(HEADER_VERSION_STR_STATIC, output_file, static=True)
-    assert output_file.read_text() == EXPECTED_HEADER_STATIC
+    version.write_version_header(version_str, short_version_str, output_file, static)
+    assert output_file.read_text() == expected_header
 
 
 def test_header_gen_exists_not_changed(fake_user_hostname, fake_date, tmp_path):
@@ -158,11 +191,15 @@ def test_header_gen_exists_not_changed(fake_user_hostname, fake_date, tmp_path):
     output_file = tmp_path / "ec_version.h"
 
     # First time, write and record mtime.
-    version.write_version_header(HEADER_VERSION_STR, output_file)
+    version.write_version_header(
+        HEADER_VERSION_STR, SHORT_HEADER_VERSION_STR, output_file
+    )
     expected_mtime = output_file.stat().st_mtime
 
     # Do another write (contents should be unchanged).
-    version.write_version_header(HEADER_VERSION_STR, output_file)
+    version.write_version_header(
+        HEADER_VERSION_STR, SHORT_HEADER_VERSION_STR, output_file
+    )
 
     # Assert we didn't write again.
     assert output_file.stat().st_mtime == expected_mtime
@@ -173,11 +210,18 @@ def test_header_gen_exists_needs_changes(fake_user_hostname, fake_date, tmp_path
     output_file = tmp_path / "ec_version.h"
 
     # First time, write and save contents.
-    version.write_version_header(HEADER_VERSION_STR, output_file)
+    version.write_version_header(
+        HEADER_VERSION_STR, SHORT_HEADER_VERSION_STR, output_file
+    )
     original_contents = output_file.read_text()
 
     # Do another write (contents should be changed).
-    version.write_version_header(HEADER_VERSION_STR_STATIC, output_file, static=True)
+    version.write_version_header(
+        HEADER_VERSION_STR_STATIC,
+        SHORT_HEADER_VERSION_STR_STATIC,
+        output_file,
+        static=True,
+    )
 
     # Assert we overwrote.
     assert output_file.read_text() != original_contents
diff --git a/zephyr/zmake/tests/test_zmake.py b/zephyr/zmake/tests/test_zmake.py
index 641f9f3db9..e57a4f5ed3 100644
--- a/zephyr/zmake/tests/test_zmake.py
+++ b/zephyr/zmake/tests/test_zmake.py
@@ -141,7 +141,7 @@ EXTRAVERSION =
 """
                 )
             zephyr_base.resolve = mock.Mock(return_value=pathlib.Path(tmpname))
-            with patch("zmake.version.get_version_string", return_value="123"):
+            with patch("zmake.version.get_version_string", return_value=("123", "123")):
                 with patch.object(zmake.project, "Project", return_value=FakeProject()):
                     if use_configure:
                         zmk.configure(
diff --git a/zephyr/zmake/zmake/version.py b/zephyr/zmake/zmake/version.py
index 47aba6d804..c5506e9921 100644
--- a/zephyr/zmake/zmake/version.py
+++ b/zephyr/zmake/zmake/version.py
@@ -86,12 +86,14 @@ def get_version_string(project, zephyr_base, modules, static=False):
             commits.
 
     Returns:
-        A version string which can be placed in FRID, FWID, or used in
-        the build for the OS.
+        A tuple of version string which can be placed in FRID, FWID, or used in
+        the build for the OS, and a shortened string which is 31 chars or less.
     """
     major_version, minor_version, *_ = util.read_zephyr_version(zephyr_base)
     project_id = project.project_dir.parts[-1]
     num_commits = 0
+    # Omit num_commits from short version string.
+    short_version_string = "{}_v{}.{}".format(project_id, major_version, minor_version)
 
     if static:
         vcs_hashes = "STATIC"
@@ -104,23 +106,38 @@ def get_version_string(project, zephyr_base, modules, static=False):
         for repo in repos.values():
             num_commits += _get_num_commits(repo)
 
-        vcs_hashes = ",".join(
-            "{}:{}".format(name, _get_revision(repo)[:6])
-            for name, repo in sorted(
-                repos.items(),
-                # Put the EC module first, then Zephyr OS kernel, as
-                # these are probably the most important hashes to
-                # developers.
-                key=lambda p: (p[0] != "ec", p[0] != "os", p),
+        vcs_parts = []
+        for name, repo in sorted(
+            repos.items(),
+            # Put the EC module first, then Zephyr OS kernel, as
+            # these are probably the most important hashes to
+            # developers.
+            key=lambda p: (p[0] != "ec", p[0] != "os", p),
+        ):
+            # Add hashes to vcs_hashes and short_version_string if it is still
+            # short enough.
+            new_short_ver = short_version_string
+            val = "{}:{}".format(name, _get_revision(repo)[:6])
+            vcs_parts.append(val)
+            vcs_hashes = ",".join(vcs_parts)
+            new_short_ver = "{}_v{}.{}-{}".format(
+                project_id, major_version, minor_version, vcs_hashes
             )
-        )
 
-    return "{}_v{}.{}.{}-{}".format(
+            if len(new_short_ver) <= 31:
+                short_version_string = new_short_ver
+
+    long_version_string = "{}_v{}.{}.{}-{}".format(
         project_id, major_version, minor_version, num_commits, vcs_hashes
     )
+    # If the long version string is short enough use it.
+    if len(long_version_string) <= 31:
+        short_version_string = long_version_string
+    assert len(short_version_string) <= 31
+    return long_version_string, short_version_string
 
 
-def write_version_header(version_str, output_path, static=False):
+def write_version_header(version_str, short_version_str, output_path, static=False):
     """Generate a version header and write it to the specified path.
 
     Generate a version header in the format expected by the EC build
@@ -148,7 +165,7 @@ def write_version_header(version_str, output_path, static=False):
         output.write("#define {} {}\n".format(name, value))
 
     add_def("VERSION", version_str)
-    add_def("CROS_EC_VERSION32", version_str[:31])
+    add_def("CROS_EC_VERSION32", short_version_str)
 
     if static:
         add_def("BUILDER", "reproducible@build")
diff --git a/zephyr/zmake/zmake/zmake.py b/zephyr/zmake/zmake/zmake.py
index 1b60e66f66..34e25cc365 100644
--- a/zephyr/zmake/zmake/zmake.py
+++ b/zephyr/zmake/zmake/zmake.py
@@ -376,7 +376,7 @@ class Zmake:
         project = zmake.project.Project(build_dir / "project")
 
         # Compute the version string.
-        version_string = zmake.version.get_version_string(
+        version_string, short_version_string = zmake.version.get_version_string(
             project,
             build_dir / "zephyr_base",
             zmake.modules.locate_from_directory(build_dir / "modules"),
@@ -387,6 +387,7 @@ class Zmake:
         # configure was run.
         zmake.version.write_version_header(
             version_string,
+            short_version_string,
             build_dir / "include" / "ec_version.h",
         )
 
@@ -442,7 +443,7 @@ class Zmake:
         if output_files_out is None:
             output_files_out = []
         for output_file, output_name in project.packer.pack_firmware(
-            packer_work_dir, self.jobserver, version_string=version_string, **dirs
+            packer_work_dir, self.jobserver, version_string=short_version_string, **dirs
         ):
             shutil.copy2(output_file, output_dir / output_name)
             self.logger.debug("Output file '%s' created.", output_file)
@@ -591,7 +592,7 @@ class Zmake:
             return rv
 
         # Compute the version string.
-        version_string = zmake.version.get_version_string(
+        version_string, short_version_string = zmake.version.get_version_string(
             project,
             build_dir / "zephyr_base",
             zmake.modules.locate_from_directory(build_dir / "modules"),
@@ -602,6 +603,7 @@ class Zmake:
         # configure was run.
         zmake.version.write_version_header(
             version_string,
+            short_version_string,
             build_dir / "include" / "ec_version.h",
         )