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..5958fa52a7d 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 @@ -21,6 +21,7 @@ import shutil import textwrap import unidiff +import yaml from yamllint import config, linter @@ -30,11 +31,40 @@ 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 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): @@ -261,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): @@ -315,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) @@ -380,32 +419,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): """ @@ -713,6 +795,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 @@ -755,14 +854,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 @@ -779,7 +905,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 @@ -1984,17 +2110,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