Extend build system checks (#2039)

armandomontanez · web-flow · commit 1b64cac8d697 · 2024-11-13T14:11:49.000-06:00
* Adds check that ensures the Bazel and CMake Pico SDK version strings
  stay in sync.
* Adds check that ensures the Bazel pins for external dependencies stays
  in sync with the git submodule pins.
* Updates cyw43-driver pin in Bazel.
* Makes the checks for some build configurability options non-blocking.
diff --git a/.github/workflows/bazel_build.yml b/.github/workflows/bazel_build.yml
@@ -43,34 +43,33 @@ jobs:
       # Checks that the current BCR-requested version of Picotool builds.
       - name: Bazel Picotool backwards compatibility
         run: bazel build @picotool//:picotool
-  # Add back when it begins to pass.
-  # other-bazel-checks:
-  #   runs-on: ubuntu-latest
-  #   steps:
-  #     - name: Checkout
-  #       uses: actions/checkout@v4
-  #       with:
-  #         fetch-depth: 0
-  #     - name: Get Bazel
-  #       uses: bazel-contrib/setup-bazel@0.9.0
-  #       with:
-  #         # Avoid downloading Bazel every time.
-  #         bazelisk-cache: true
-  #         # Store build cache per workflow.
-  #         disk-cache: ${{ github.workflow }}
-  #         # Share repository cache between workflows.
-  #         repository-cache: true
-  #     # Only needed to drive the presbumit scripts.
-  #     - name: Setup Python
-  #       uses: actions/setup-python@v5
-  #       with:
-  #         python-version: '3.10'
-  #     - name: Fetch latest Picotool
-  #       uses: actions/checkout@v4
-  #       with:
-  #         repository: raspberrypi/picotool
-  #         ref: develop
-  #         fetch-depth: 0
-  #         path: lib/picotool
-  #     - name: Other Bazel checks
-  #       run: python3 tools/run_all_bazel_checks.py --program=other --picotool-dir=lib/picotool
+  other-bazel-checks:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+      - name: Get Bazel
+        uses: bazel-contrib/setup-bazel@0.9.0
+        with:
+          # Avoid downloading Bazel every time.
+          bazelisk-cache: true
+          # Store build cache per workflow.
+          disk-cache: ${{ github.workflow }}
+          # Share repository cache between workflows.
+          repository-cache: true
+      # Only needed to drive the presbumit scripts.
+      - name: Setup Python
+        uses: actions/setup-python@v5
+        with:
+          python-version: '3.10'
+      - name: Fetch latest Picotool
+        uses: actions/checkout@v4
+        with:
+          repository: raspberrypi/picotool
+          ref: develop
+          fetch-depth: 0
+          path: lib/picotool
+      - name: Other Bazel checks
+        run: python3 tools/run_all_bazel_checks.py --program=other --picotool-dir=lib/picotool
diff --git a/MODULE.bazel b/MODULE.bazel
@@ -84,40 +84,38 @@ http_archive(
     url = "https://chrome-infra-packages.appspot.com/dl/fuchsia/third_party/clang/mac-arm64/+/git_revision:2b0a708f41dd6291ee744704d43febc975e3d026",
 )
 
+new_git_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:git.bzl", "new_git_repository")
+
 # TODO: Provide tinyusb as a proper Bazel module.
-http_archive(
+new_git_repository(
     name = "tinyusb",
     build_file = "//src/rp2_common/tinyusb:tinyusb.BUILD",
-    sha256 = "d64728aef58b80d5ce3747cad133f520da46e2b7ea3aadfda0e981aba6b658b3",
-    strip_prefix = "tinyusb-4232642899362fa5e9cf0dc59bad6f1f6d32c563",
-    url = "https://github.com/hathach/tinyusb/archive/4232642899362fa5e9cf0dc59bad6f1f6d32c563.tar.gz",
+    commit = "4232642899362fa5e9cf0dc59bad6f1f6d32c563",  # keep-in-sync-with-submodule: tinyusb
+    remote = "https://github.com/hathach/tinyusb.git",
 )
 
 # TODO: Provide btstack as a proper Bazel module.
-http_archive(
+new_git_repository(
     name = "btstack",
     build_file = "//src/rp2_common/pico_btstack:btstack.BUILD",
-    sha256 = "64e86d9cf82b346e743fe1d4818b9380712b17abdb3f2c3524e92464b5ef3d19",
-    strip_prefix = "btstack-2b49e57bd1fae85ac32ac1f41cdb7c794de335f6",
-    url = "https://github.com/bluekitchen/btstack/archive/2b49e57bd1fae85ac32ac1f41cdb7c794de335f6.tar.gz",
+    commit = "2b49e57bd1fae85ac32ac1f41cdb7c794de335f6",  # keep-in-sync-with-submodule: btstack
+    remote = "https://github.com/bluekitchen/btstack.git",
 )
 
-# TODO: Provide btstack as a proper Bazel module.
-http_archive(
+# TODO: Provide cyw43-driver as a proper Bazel module.
+new_git_repository(
     name = "cyw43-driver",
     build_file = "//src/rp2_common/pico_cyw43_driver:cyw43-driver.BUILD",
-    sha256 = "0fcc7707fef95dd562d5572604713266613a27caeeae2f10afeccee9592a53ce",
-    strip_prefix = "cyw43-driver-faf36381bad1f668a30172b6336c9a970966ef4c",
-    url = "https://github.com/georgerobotics/cyw43-driver/archive/faf36381bad1f668a30172b6336c9a970966ef4c.tar.gz",
+    commit = "cf924bb04c8984675ca0fc2178f082e404e048c3",  # keep-in-sync-with-submodule: cyw43-driver
+    remote = "https://github.com/georgerobotics/cyw43-driver.git",
 )
 
 # TODO: Provide lwip as a proper Bazel module.
-http_archive(
+new_git_repository(
     name = "lwip",
     build_file = "//src/rp2_common/pico_lwip:lwip.BUILD",
-    sha256 = "72856d557f72911cf6826ef745c23c54822df83a474557823241164a1d1361aa",
-    strip_prefix = "lwip-0a0452b2c39bdd91e252aef045c115f88f6ca773",
-    url = "https://github.com/lwip-tcpip/lwip/archive/0a0452b2c39bdd91e252aef045c115f88f6ca773.tar.gz",
+    commit = "0a0452b2c39bdd91e252aef045c115f88f6ca773",  # keep-in-sync-with-submodule: lwip
+    remote = "https://github.com/lwip-tcpip/lwip.git",
 )
 
 register_toolchains(
diff --git a/tools/compare_build_systems.py b/tools/compare_build_systems.py
@@ -16,7 +16,9 @@
 import glob
 import logging
 import os
+from pathlib import Path
 import re
+import subprocess
 import sys
 from typing import Dict
 
@@ -37,6 +39,12 @@
 
 ATTR_REGEX = re.compile(r",?\s*(?P<key>[^=]+)=(?P<value>[^,]+)")
 
+BAZEL_MODULE_REGEX = re.compile(r'\s*commit\s*=\s*\"(?P<commit>[0-9a-fA-F]+)\"\s*,\s*#\s*keep-in-sync-with-submodule:\s*(?P<dependency>\S*)')
+
+BAZEL_VERSION_REGEX = re.compile(r'module\(\s*name\s*=\s*"pico-sdk",\s*version\s*=\s*"(?P<sdk_version>[^"]+)",?\s*\)')
+
+CMAKE_VERSION_REGEX = re.compile(r'^[^#]*set\(PICO_SDK_VERSION_(?P<part>\S+)\s+(?P<value>\S+)\)')
+
 # Sometimes the build systems are supposed to be implemented differently. This
 # allowlist permits the descriptions to differ between CMake and Bazel.
 BUILD_SYSTEM_DESCRIPTION_DIFFERENCE_ALLOWLIST = (
@@ -60,7 +68,7 @@
     "PICO_TOOLCHAIN_PATH",
     # Bazel uses native --platforms mechanics.
     "PICO_PLATFORM",
-    # TODO: No built-in, pre-configured clang offering for Bazel yet.
+    # Named PICO_TOOLCHAIN in Bazel.
     "PICO_COMPILER",
     # Entirely irrelevant to Bazel, use Bazel platforms:
     #     https://bazel.build/extending/platforms
@@ -87,14 +95,17 @@
     "PICO_DEFAULT_PIOASM_OUTPUT_FORMAT",
     # Bazel always has picotool.
     "PICO_NO_PICOTOOL",
-    # TODO: Eventualy support.
-    "PICO_NO_COPRO_DIS",
-    "PICO_DEFAULT_RP2350_PLATFORM",
-    "PICO_GCC_TRIPLE",
-    "PICO_NO_FLASH",
-    "PICO_COPY_TO_RAM",
-    "PICO_RP2350_ARM_S_CONFIG_HEADER_FILES",
-    "PICO_RP2350_RISCV_CONFIG_HEADER_FILES",
+    # These aren't supported as build flags in Bazel. Prefer to
+    # set these in board header files like other SDK defines.
+    "CYW43_DEFAULT_PIN_WL_REG_ON",
+    "CYW43_DEFAULT_PIN_WL_DATA_OUT",
+    "CYW43_DEFAULT_PIN_WL_DATA_IN",
+    "CYW43_DEFAULT_PIN_WL_HOST_WAKE",
+    "CYW43_DEFAULT_PIN_WL_CLOCK",
+    "CYW43_DEFAULT_PIN_WL_CS",
+    "CYW43_PIO_CLOCK_DIV_INT",
+    "CYW43_PIO_CLOCK_DIV_FRAC",
+    "CYW43_PIO_CLOCK_DIV_DYNAMIC",
 )
 
 BAZEL_ONLY_ALLOWLIST = (
@@ -181,17 +192,17 @@ def FindKnownOptions(option_pattern_matcher, file_paths):
     return options
 
 
-def OptionsAreEqual(bazel_option, cmake_option):
+def OptionsAreEqual(bazel_option, cmake_option, warnings_as_errors):
     if bazel_option is None:
         if cmake_option.name in CMAKE_ONLY_ALLOWLIST:
             return True
         _LOG.warning(f"    {cmake_option.name} does not exist in Bazel")
-        return False
+        return not warnings_as_errors
     elif cmake_option is None:
         if bazel_option.name in BAZEL_ONLY_ALLOWLIST:
             return True
         _LOG.warning(f"    {bazel_option.name} does not exist in CMake")
-        return False
+        return not warnings_as_errors
     elif not bazel_option.matches(cmake_option):
         _LOG.error("    Bazel and CMAKE definitions do not match:")
         _LOG.error(f"    [CMAKE]    {bazel_option}")
@@ -201,7 +212,7 @@ def OptionsAreEqual(bazel_option, cmake_option):
     return True
 
 
-def CompareOptions(bazel_pattern, bazel_files, cmake_pattern, cmake_files):
+def CompareOptions(bazel_pattern, bazel_files, cmake_pattern, cmake_files, warnings_as_errors=True):
     bazel_options = FindKnownOptions(bazel_pattern, bazel_files)
     cmake_options = FindKnownOptions(cmake_pattern, cmake_files)
 
@@ -210,10 +221,72 @@ def CompareOptions(bazel_pattern, bazel_files, cmake_pattern, cmake_files):
     both.update(bazel_options)
     both.update(cmake_options)
     for k in both.keys():
-        if not OptionsAreEqual(bazel_options.get(k, None), cmake_options.get(k, None)):
+        if not OptionsAreEqual(
+            bazel_options.get(k, None),
+            cmake_options.get(k, None),
+            warnings_as_errors,
+        ):
             are_equal = False
     return are_equal
 
+def CompareExternalDependencyVersions():
+    pattern = re.compile(BAZEL_MODULE_REGEX)
+    all_okay = True
+    with open(Path(SDK_ROOT) / "MODULE.bazel", "r") as bazel_module_file:
+        for line in bazel_module_file:
+            maybe_match = pattern.match(line)
+            if not maybe_match:
+                continue
+
+            current_submodule_pin = subprocess.run(
+                ("git", "-C", SDK_ROOT, "rev-parse", f'HEAD:lib/{maybe_match.group("dependency")}'),
+                text=True,
+                check=True,
+                capture_output=True,
+            ).stdout.strip()
+            if current_submodule_pin != maybe_match.group("commit"):
+                _LOG.error("    External pins for %s do not match:", maybe_match.group("dependency"))
+                _LOG.error("    [CMAKE]    %s", current_submodule_pin)
+                _LOG.error("    [BAZEL]    %s", maybe_match.group("commit"))
+                all_okay = False
+            else:
+                _LOG.info("    External pins for %s match!", maybe_match.group("dependency"))
+
+    return all_okay
+
+def CompareSdkVersion():
+    # Find version string specified in Bazel.
+    bazel_module_file_path = Path(SDK_ROOT) / "MODULE.bazel"
+    bazel_module_file_contents = bazel_module_file_path.read_text()
+    bazel_sdk_version = BAZEL_VERSION_REGEX.search(bazel_module_file_contents)
+    if not bazel_sdk_version:
+        _LOG.error("    Failed to find Bazel Pico SDK version string")
+        return False
+    bazel_version_string = bazel_sdk_version.group("sdk_version")
+
+    # Find version string specified in CMake.
+    cmake_version_parts = {}
+    with open(Path(SDK_ROOT) / "pico_sdk_version.cmake", "r") as cmake_version_file:
+        for line in cmake_version_file:
+            match = CMAKE_VERSION_REGEX.match(line)
+            if match:
+                cmake_version_parts[match.group("part")] = match.group("value")
+    if len(cmake_version_parts) < 3:
+        _LOG.error("    Failed to find CMake Pico SDK version string")
+        return False
+    cmake_version_string = ".".join((
+        cmake_version_parts["MAJOR"],
+        cmake_version_parts["MINOR"],
+        cmake_version_parts["REVISION"],
+    ))
+    if "PRE_RELEASE_ID" in cmake_version_parts:
+        cmake_version_string += "-" + cmake_version_parts["PRE_RELEASE_ID"]
+
+    if cmake_version_string != bazel_version_string:
+        _LOG.error("    Declared CMake SDK version is %s and Bazel is %s", cmake_version_string, bazel_version_string)
+        return False
+
+    return True
 
 def compare_build_systems():
     cmake_files = [
@@ -227,20 +300,35 @@ def compare_build_systems():
         for f in glob.glob(os.path.join(SDK_ROOT, p), recursive=True)
     ]
 
-    _LOG.info("[1/2] Checking build system configuration flags...")
-    build_options_ok = CompareOptions(
-        "PICO_BAZEL_CONFIG", bazel_files, "PICO_CMAKE_CONFIG", cmake_files
-    )
+    results = []
+    _LOG.info("[1/3] Checking build system configuration flags...")
+    results.append(CompareOptions(
+        "PICO_BAZEL_CONFIG",
+        bazel_files,
+        "PICO_CMAKE_CONFIG",
+        cmake_files,
+        # For now, allow CMake and Bazel to go out of sync when it comes to
+        # build configurability since it's a big ask to make contributors
+        # implement the same functionality in both builds.
+        warnings_as_errors=False,
+    ))
 
-    _LOG.info("[2/2] Checking build system defines...")
-    build_defines_ok = CompareOptions(
+    _LOG.info("[2/4] Checking build system defines...")
+    results.append(CompareOptions(
         "PICO_BUILD_DEFINE", bazel_files, "PICO_BUILD_DEFINE", cmake_files
-    )
+    ))
+
+    _LOG.info("[3/4] Checking submodule pins...")
+    results.append(CompareExternalDependencyVersions())
+
+    _LOG.info("[4/4] Checking version strings...")
+    results.append(CompareSdkVersion())
 
-    if build_options_ok and build_defines_ok:
-        _LOG.info("OK")
+    if False not in results:
+        _LOG.info("Passed with no blocking failures")
         return 0
 
+    _LOG.error("One or more blocking failures detected")
     return 1
 
 
