Extend build system comparison checks #2039
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| other-bazel-checks: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Get Bazel | ||
| uses: bazel-contrib/[email protected] | ||
| 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 | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
| # 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", | ||
| # 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, 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 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 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, 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), | ||
| 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, | ||
|
There was a problem hiding this comment. I guess this is assuming that our submodules are always in the There was a problem hiding this comment. Sent out #2040 for this one |
||
| 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) | ||
| ] | ||
|
|
||
| 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/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 False not in results: | ||
| _LOG.info("Passed with no blocking failures") | ||
| return 0 | ||
|
|
||
| _LOG.error("One or more blocking failures detected") | ||
| return 1 | ||
|
|
||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be changed from
CYW43_PIO_CLOCK_DIV_FRACtoCYW43_PIO_CLOCK_DIV_FRAC8to match the change in #1926 ?Also, this list might be missing
CYW43_PIN_WL_DYNAMICwhich is one of the things that #2033 fixes in the CMake-world?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, I forgot to pull latest changes before adding these. This just squelches the warnings, the findings shouldn't get in the way of anything. I can put up a separate PR to add those.