[nrf fromlist] scripts: parse module.yml for dts bindings

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

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