From c52d0f4c0ba314200009d31a3f493be9838a1593 Mon Sep 17 00:00:00 2001 From: Armando Montanez Date: Tue, 12 Nov 2024 13:50:19 -0800 Subject: [PATCH] Extend build system checks * 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. --- .github/workflows/bazel_build.yml | 61 +++++++------- MODULE.bazel | 32 ++++--- tools/compare_build_systems.py | 134 +++++++++++++++++++++++++----- 3 files changed, 156 insertions(+), 71 deletions(-) diff --git a/.github/workflows/bazel_build.yml b/.github/workflows/bazel_build.yml index 183543298..81b7ae583 100644 --- 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 index ca27a1448..01bad255c 100644 --- 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 index 4a568aec4..01b808313 100755 --- 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[^=]+)=(?P[^,]+)") +BAZEL_MODULE_REGEX = re.compile(r'\s*commit\s*=\s*\"(?P[0-9a-fA-F]+)\"\s*,\s*#\s*keep-in-sync-with-submodule:\s*(?P\S*)') + +BAZEL_VERSION_REGEX = re.compile(r'module\(\s*name\s*=\s*"pico-sdk",\s*version\s*=\s*"(?P[^"]+)",?\s*\)') + +CMAKE_VERSION_REGEX = re.compile(r'^[^#]*set\(PICO_SDK_VERSION_(?P\S+)\s+(?P\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