From 3668d3ba106a99b0565fd65095f583fd6bff6e25 Mon Sep 17 00:00:00 2001 From: James Roy Date: Thu, 12 Jun 2025 20:31:47 +0800 Subject: [PATCH 1/4] [nrf fromtree] scripts: ci: Add CI bindings style checker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement a check in the CI pipeline to enforce that property names in device tree bindings do not contain underscores. Signed-off-by: James Roy Signed-off-by: Martí Bolívar (cherry picked from commit 4240853a06b83a0d5d49a5e80697f4a6485a3a76) (cherry picked from commit 70e7ab3812919e7d9562ff4ee243be806d1982f3) --- scripts/bindings_properties_allowlist.yaml | 11 ++ scripts/ci/check_compliance.py | 117 +++++++++++++++------ 2 files changed, 98 insertions(+), 30 deletions(-) create mode 100644 scripts/bindings_properties_allowlist.yaml diff --git a/scripts/bindings_properties_allowlist.yaml b/scripts/bindings_properties_allowlist.yaml new file mode 100644 index 00000000000..100d4d380fb --- /dev/null +++ b/scripts/bindings_properties_allowlist.yaml @@ -0,0 +1,11 @@ +# This file is a YAML allowlist of property names that are allowed to +# bypass the underscore check in bindings. These properties are exempt +# from the rule that requires using '-' instead of '_'. +# +# The file content can be as shown below: +# - propname1 +# - propname2 +# - ... + +- mmc-hs200-1_8v +- mmc-hs400-1_8v diff --git a/scripts/ci/check_compliance.py b/scripts/ci/check_compliance.py index 3ee7b62c9b5..7ad88630e17 100755 --- a/scripts/ci/check_compliance.py +++ b/scripts/ci/check_compliance.py @@ -21,6 +21,7 @@ import shutil import textwrap import unidiff +import yaml from yamllint import config, linter @@ -35,6 +36,30 @@ import list_boards import list_hardware +sys.path.insert(0, str(Path(__file__).resolve().parents[2] + / "scripts" / "dts" / "python-devicetree" / "src")) +from devicetree import edtlib + + +# Let the user run this script as ./scripts/ci/check_compliance.py without +# making them set ZEPHYR_BASE. +ZEPHYR_BASE = os.environ.get('ZEPHYR_BASE') +if ZEPHYR_BASE: + ZEPHYR_BASE = Path(ZEPHYR_BASE) +else: + ZEPHYR_BASE = Path(__file__).resolve().parents[2] + # Propagate this decision to child processes. + os.environ['ZEPHYR_BASE'] = str(ZEPHYR_BASE) + +# Initialize the property names allowlist +BINDINGS_PROPERTIES_AL = None +with open(Path(__file__).parents[1] / 'bindings_properties_allowlist.yaml') as f: + allowlist = yaml.safe_load(f.read()) + if allowlist is not None: + BINDINGS_PROPERTIES_AL = set(allowlist) + else: + BINDINGS_PROPERTIES_AL = set() + logger = None def git(*args, cwd=None, ignore_non_zero=False): @@ -380,32 +405,75 @@ class DevicetreeBindingsCheck(ComplianceTest): doc = "See https://docs.zephyrproject.org/latest/build/dts/bindings.html for more details." def run(self, full=True): - dts_bindings = self.parse_dt_bindings() - - for dts_binding in dts_bindings: - self.required_false_check(dts_binding) + bindings_diff, bindings = self.get_yaml_bindings() - def parse_dt_bindings(self): + # If no bindings are changed, skip this check. + try: + subprocess.check_call(['git', 'diff', '--quiet', COMMIT_RANGE] + + bindings_diff) + nodiff = True + except subprocess.CalledProcessError: + nodiff = False + if nodiff: + self.skip('no changes to bindings were made') + + for binding in bindings: + self.check(binding, self.check_yaml_property_name) + self.check(binding, self.required_false_check) + + @staticmethod + def check(binding, callback): + while binding is not None: + callback(binding) + binding = binding.child_binding + + def get_yaml_bindings(self): """ - Returns a list of dts/bindings/**/*.yaml files + Returns a list of 'dts/bindings/**/*.yaml' """ + from glob import glob + BINDINGS_PATH = 'dts/bindings/' + bindings_diff_dir, bindings = set(), [] - dt_bindings = [] - for file_name in get_files(filter="d"): - if 'dts/bindings/' in file_name and file_name.endswith('.yaml'): - dt_bindings.append(file_name) + for file_name in get_files(filter='d'): + if BINDINGS_PATH in file_name: + p = file_name.partition(BINDINGS_PATH) + bindings_diff_dir.add(os.path.join(p[0], p[1])) - return dt_bindings + for path in bindings_diff_dir: + yamls = glob(f'{os.fspath(path)}/**/*.yaml', recursive=True) + bindings.extend(yamls) - def required_false_check(self, dts_binding): - with open(dts_binding) as file: - for line_number, line in enumerate(file, 1): - if 'required: false' in line: - self.fmtd_failure( - 'warning', 'Devicetree Bindings', dts_binding, - line_number, col=None, - desc="'required: false' is redundant, please remove") + bindings = edtlib.bindings_from_paths(bindings, ignore_errors=True) + return list(bindings_diff_dir), bindings + def check_yaml_property_name(self, binding): + """ + Checks if the property names in the binding file contain underscores. + """ + for prop_name in binding.prop2specs: + if '_' in prop_name and prop_name not in BINDINGS_PROPERTIES_AL: + better_prop = prop_name.replace('_', '-') + print(f"Required: In '{binding.path}', " + f"the property '{prop_name}' " + f"should be renamed to '{better_prop}'.") + self.failure( + f"{binding.path}: property '{prop_name}' contains underscores.\n" + f"\tUse '{better_prop}' instead unless this property name is from Linux.\n" + "Or another authoritative upstream source of bindings for " + f"compatible '{binding.compatible}'.\n" + "\tHint: update 'bindings_properties_allowlist.yaml' if you need to " + "override this check for this property." + ) + + def required_false_check(self, binding): + raw_props = binding.raw.get('properties', {}) + for prop_name, raw_prop in raw_props.items(): + if raw_prop.get('required') is False: + self.failure( + f'{binding.path}: property "{prop_name}": ' + "'required: false' is redundant, please remove" + ) class KconfigCheck(ComplianceTest): """ @@ -1984,17 +2052,6 @@ def _main(args): # The "real" main(), which is wrapped to catch exceptions and report them # to GitHub. Returns the number of test failures. - global ZEPHYR_BASE - ZEPHYR_BASE = os.environ.get('ZEPHYR_BASE') - if not ZEPHYR_BASE: - # Let the user run this script as ./scripts/ci/check_compliance.py without - # making them set ZEPHYR_BASE. - ZEPHYR_BASE = str(Path(__file__).resolve().parents[2]) - - # Propagate this decision to child processes. - os.environ['ZEPHYR_BASE'] = ZEPHYR_BASE - ZEPHYR_BASE = Path(ZEPHYR_BASE) - # The absolute path of the top-level git directory. Initialize it here so # that issues running Git can be reported to GitHub. global GIT_TOP From 110960f8e0074ba2f794ab60f7bce73cbaaed424 Mon Sep 17 00:00:00 2001 From: Jamie McCrae Date: Tue, 4 Mar 2025 09:11:24 +0000 Subject: [PATCH 2/4] [nrf fromtree] scripts: ci: check_compliance: Add support for modules for Kconfig Adds support for checking modules for disallow Kconfig's in boards and SoCs, which have been defined in a Zephyr module file Signed-off-by: Jamie McCrae (cherry picked from commit 6d73a9c45aee4a1040bf4f94d41a27463ceca027) (cherry picked from commit 2f556e6769ccef2e66d1607ca0cf6e6dab63aa15) --- scripts/ci/check_compliance.py | 59 +++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/scripts/ci/check_compliance.py b/scripts/ci/check_compliance.py index 7ad88630e17..5a441c873f0 100755 --- a/scripts/ci/check_compliance.py +++ b/scripts/ci/check_compliance.py @@ -10,7 +10,7 @@ import json import logging import os -from pathlib import Path +from pathlib import Path, PurePath import platform import re import subprocess @@ -31,6 +31,11 @@ from west.manifest import Manifest from west.manifest import ManifestProject +try: + from yaml import CSafeLoader as SafeLoader +except ImportError: + from yaml import SafeLoader + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from get_maintainer import Maintainers, MaintainersError import list_boards @@ -781,6 +786,23 @@ def get_logging_syms(self, kconf): return set(kconf_syms) + def module_disallowed_check(self, module_path, type, folder, meta, regex): + # Returns a list with lines from git grep which includes Kconfigs from defconfig files + entry = type + '_root' + git_folder = ":" + folder + + if entry in meta['build']['settings']: + tmp_path = module_path.joinpath(meta['build']['settings'][entry]) + + if Path(tmp_path.joinpath(folder)).is_dir(): + tmp_output = git("grep", "--line-number", "-I", "--null", + "--perl-regexp", regex, "--", git_folder, + cwd=tmp_path, ignore_non_zero=True) + + if len(tmp_output) > 0: + return tmp_output.splitlines() + return [] + def check_disallowed_defconfigs(self, kconf): """ Checks that there are no disallowed Kconfigs used in board/SoC defconfig files @@ -823,14 +845,41 @@ def check_disallowed_defconfigs(self, kconf): grep_stdout_boards = git("grep", "--line-number", "-I", "--null", "--perl-regexp", regex_boards, "--", ":boards", - cwd=ZEPHYR_BASE) + cwd=ZEPHYR_BASE).splitlines() grep_stdout_socs = git("grep", "--line-number", "-I", "--null", "--perl-regexp", regex_socs, "--", ":soc", - cwd=ZEPHYR_BASE) + cwd=ZEPHYR_BASE).splitlines() + + manifest = Manifest.from_file() + for project in manifest.get_projects([]): + if not manifest.is_active(project): + continue + + if not project.is_cloned(): + continue + + module_path = PurePath(project.abspath) + module_yml = module_path.joinpath('zephyr/module.yml') + + if not Path(module_yml).is_file(): + module_yml = module_path.joinpath('zephyr/module.yaml') + + if Path(module_yml).is_file(): + with Path(module_yml).open('r', encoding='utf-8') as f: + meta = yaml.load(f.read(), Loader=SafeLoader) + + if 'build' in meta and 'settings' in meta['build']: + grep_stdout_boards.extend(self.module_disallowed_check(module_path, + 'board', + 'boards', meta, + regex_boards)) + grep_stdout_socs.extend(self.module_disallowed_check(module_path, 'soc', + 'soc', meta, + regex_socs)) # Board processing # splitlines() supports various line terminators - for grep_line in grep_stdout_boards.splitlines(): + for grep_line in grep_stdout_boards: path, lineno, line = grep_line.split("\0") # Extract symbol references (might be more than one) within the line @@ -847,7 +896,7 @@ def check_disallowed_defconfigs(self, kconf): # SoCs processing # splitlines() supports various line terminators - for grep_line in grep_stdout_socs.splitlines(): + for grep_line in grep_stdout_socs: path, lineno, line = grep_line.split("\0") # Extract symbol references (might be more than one) within the line From fea19e4852bbb107d5b99bf6f13b0816b9e638c8 Mon Sep 17 00:00:00 2001 From: Declan Snyder Date: Fri, 27 Jun 2025 13:04:13 -0500 Subject: [PATCH 3/4] [nrf fromtree] scripts: check_compliance: Fix resource leak The git diff subprocess was leaking, ie., it was still running with it's file streams open, and python was printing warnings about this. Fix by calling communicate() on the object which will do the cleanup. Signed-off-by: Declan Snyder (cherry picked from commit 3b5bfd9f4ad3e140af3b0e30f250807b8a886573) (cherry picked from commit 2d0838c2fc9c8994e19a32a6bf50fd9dbbda30e8) --- scripts/ci/check_compliance.py | 50 +++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/scripts/ci/check_compliance.py b/scripts/ci/check_compliance.py index 5a441c873f0..332f6cf08c3 100755 --- a/scripts/ci/check_compliance.py +++ b/scripts/ci/check_compliance.py @@ -291,37 +291,37 @@ def run(self): cmd = [checkpatch] cmd.extend(['--mailback', '--no-tree', '-']) - diff = subprocess.Popen(('git', 'diff', '--no-ext-diff', COMMIT_RANGE), + with subprocess.Popen(('git', 'diff', '--no-ext-diff', COMMIT_RANGE), stdout=subprocess.PIPE, - cwd=GIT_TOP) - try: - subprocess.run(cmd, - check=True, - stdin=diff.stdout, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - shell=False, cwd=GIT_TOP) + cwd=GIT_TOP) as diff: + try: + subprocess.run(cmd, + check=True, + stdin=diff.stdout, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + shell=False, cwd=GIT_TOP) - except subprocess.CalledProcessError as ex: - output = ex.output.decode("utf-8") - regex = r'^\s*\S+:(\d+):\s*(ERROR|WARNING):(.+?):(.+)(?:\n|\r\n?)+' \ - r'^\s*#(\d+):\s*FILE:\s*(.+):(\d+):' + except subprocess.CalledProcessError as ex: + output = ex.output.decode("utf-8") + regex = r'^\s*\S+:(\d+):\s*(ERROR|WARNING):(.+?):(.+)(?:\n|\r\n?)+' \ + r'^\s*#(\d+):\s*FILE:\s*(.+):(\d+):' - matches = re.findall(regex, output, re.MULTILINE) + matches = re.findall(regex, output, re.MULTILINE) - # add a guard here for excessive number of errors, do not try and - # process each one of them and instead push this as one failure. - if len(matches) > 500: - self.failure(output) - return + # add a guard here for excessive number of errors, do not try and + # process each one of them and instead push this as one failure. + if len(matches) > 500: + self.failure(output) + return - for m in matches: - self.fmtd_failure(m[1].lower(), m[2], m[5], m[6], col=None, - desc=m[3]) + for m in matches: + self.fmtd_failure(m[1].lower(), m[2], m[5], m[6], col=None, + desc=m[3]) - # If the regex has not matched add the whole output as a failure - if len(matches) == 0: - self.failure(output) + # If the regex has not matched add the whole output as a failure + if len(matches) == 0: + self.failure(output) class BoardYmlCheck(ComplianceTest): From 69db8bf713698436041cbf414c7a53e0aced2157 Mon Sep 17 00:00:00 2001 From: Torsten Rasmussen Date: Tue, 12 Aug 2025 12:45:02 +0200 Subject: [PATCH 4/4] [nrf fromlist] scripts: parse module.yml for dts bindings Change BoardYml compliance check from using get_module_setting_root() to load and parse module.yml directly. get_module_setting_root cannot be used directly with module.yml. Instead adjust the code so that the module.yml file is loaded locally and the dts_root setting is extracted from the dictionary. Upstream PR #: 94397 Signed-off-by: Torsten Rasmussen (cherry picked from commit a922397417e49858aa7a6fd98d8ef79f6dee3666) --- scripts/ci/check_compliance.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/scripts/ci/check_compliance.py b/scripts/ci/check_compliance.py index 332f6cf08c3..5958fa52a7d 100755 --- a/scripts/ci/check_compliance.py +++ b/scripts/ci/check_compliance.py @@ -345,15 +345,24 @@ def check_board_file(self, file, vendor_prefixes): def run(self): path = resolve_path_hint(self.path_hint) + module_ymls = [path / "zephyr" / "module.yml", path / "zephyr" / "module.yaml"] vendor_prefixes = {"others"} # add vendor prefixes from the main zephyr repo vendor_prefixes |= get_vendor_prefixes(ZEPHYR_BASE / "dts" / "bindings" / "vendor-prefixes.txt", self.error) - # add vendor prefixes from the current repo - dts_roots = get_module_setting_root('dts', path / "zephyr" / "module.yml") - for dts_root in dts_roots: - vendor_prefix_file = dts_root / "dts" / "bindings" / "vendor-prefixes.txt" + dts_root = None + for module_yml in module_ymls: + if module_yml.is_file(): + with module_yml.open('r', encoding='utf-8') as f: + meta = yaml.load(f.read(), Loader=SafeLoader) + section = meta.get('build', dict()) + build_settings = section.get('settings', None) + if build_settings: + dts_root = build_settings.get('dts_root', None) + + if dts_root: + vendor_prefix_file = Path(dts_root) / "dts" / "bindings" / "vendor-prefixes.txt" if vendor_prefix_file.exists(): vendor_prefixes |= get_vendor_prefixes(vendor_prefix_file, self.error)