Tools: consolidate new board checks into test_new_boards.py

some checks went into check_branch_conventions.py instead.

Adds a comment so this doesn't happen again

Author: peterbarker

Tools/scripts/check_branch_conventions.py

@@ -9,9 +9,8 @@
   - commit messages have a well-formed subsystem prefix before ':'
   - commit subject lines are <= 160 characters
   - changed markdown files pass markdownlint-cli2
-  - new hwdef board directories include a README.md, except ODID variants
-  - new hwdef board READMEs contain at least one local image added in the PR,
-    except AP_Periph and ODID variants
+
+(new hwdef board README/image requirements are validated by test_new_boards.py)
 
 AP_FLAKE8_CLEAN
 '''
@@ -28,7 +27,6 @@
 import urllib.error
 import urllib.request
 
-import board_list
 import build_script_base
 
 DOCS_URL = "https://ardupilot.org/dev/docs/submitting-patches-back-to-master.html"
@@ -58,14 +56,12 @@
 class CheckBranchConventions(build_script_base.BuildScriptBase):
 
     DEFAULT_UPSTREAM = "origin/master"
-    HWDEF_PREFIX = "libraries/AP_HAL_ChibiOS/hwdef/"
 
     def __init__(self, base_branch: str | None = None) -> None:
         super().__init__()
         self.base_branch = base_branch
         repo_root = self.run_git(['rev-parse', '--show-toplevel'], show_output=False).strip()
         self.board_types_path = pathlib.Path(repo_root, 'Tools', 'AP_Bootloader', 'board_types.txt')
-        self._board_list: board_list.BoardList | None = None
 
     def progress_prefix(self) -> str:
         return "CBC"
@@ -77,34 +73,6 @@ def run_git(self, args, show_output=True, source_dir=None):
             show_output=show_output, show_command=False, cwd=source_dir,
         )
 
-    def board_list(self) -> board_list.BoardList:
-        if self._board_list is None:
-            self._board_list = board_list.BoardList()
-        return self._board_list
-
-    def is_odid_board(self, board_name: str) -> bool:
-        try:
-            hwdef = self.board_list().hwdef_for_board(board_name)
-        except KeyError:
-            return False
-        return hwdef.intdefines.get('AP_OPENDRONEID_ENABLED', 0) == 1
-
-    def is_ap_periph_board(self, board_name: str) -> bool:
-        try:
-            return bool(self.board_list().board_by_name(board_name).is_ap_periph)
-        except KeyError:
-            return False
-
-    def check_board_readme_required(self, board_name: str) -> bool:
-        return not self.is_odid_board(board_name)
-
-    def check_board_image_required(self, board_name: str) -> bool:
-        if self.is_odid_board(board_name):
-            return False
-        if self.is_ap_periph_board(board_name):
-            return False
-        return True
-
     def check_merge_commits(self) -> bool:
         merge_commits = self.run_git(
             ["log", f"{self.base_branch}..HEAD", "--merges", "--oneline"],
@@ -591,132 +559,10 @@ def check_board_ids(self) -> bool:
 
         return all_board_ids_are_valid
 
-    def check_new_board_has_readme(self) -> bool:
-        '''Every new hwdef board directory must include a README.md.'''
-
-        added_raw = self.run_git(
-            ["diff", "--name-only", "--diff-filter=A",
-             f"{self.base_branch}...HEAD"],
-            show_output=False,
-        ).strip()
-        added_files = set(added_raw.splitlines()) if added_raw else set()
-
-        # Board dirs that have at least one newly added file at depth 1 under hwdef/
-        candidate_boards = {
-            f.split("/")[3]
-            for f in added_files
-            if f.startswith(self.HWDEF_PREFIX) and f.count("/") == 4
-        }
-
-        if not candidate_boards:
-            print(f"{PASS} No new hwdef board directories added.")
-            return True
-
-        # Determine which of those dirs are genuinely new (absent in base branch)
-        existing_raw = self.run_git(
-            ["ls-tree", "--name-only", self.base_branch,
-             self.HWDEF_PREFIX],
-            show_output=False,
-        ).strip()
-        existing_boards = {
-            os.path.basename(p)
-            for p in existing_raw.splitlines()
-            if p.strip()
-        }
-
-        new_boards = sorted(candidate_boards - existing_boards)
-        if not new_boards:
-            print(f"{PASS} No new hwdef board directories added.")
-            return True
-
-        ok = True
-        for board_name in new_boards:
-            if not self.check_board_readme_required(board_name):
-                print(f"{PASS} {board_name}: README.md not required for ODID variant.")
-                continue
-
-            board_prefix = f"{self.HWDEF_PREFIX}{board_name}/"
-            has_readme = any(
-                os.path.basename(f) == "README.md"
-                for f in added_files
-                if f.startswith(board_prefix)
-            )
-            if has_readme:
-                print(f"{PASS} {board_name}: README.md present.")
-            else:
-                print(f"{FAIL} {board_name}: new board directory has no README.md.")
-                ok = False
-
-        return ok
-
-    def check_new_board_images(self) -> bool:
-        '''For each newly added hwdef board README, verify it contains at least one
-        local image reference that is also a newly added file in the PR.'''
-
-        # All files added (not modified) in this PR
-        added_raw = self.run_git(
-            ["diff", "--name-only", "--diff-filter=A",
-             f"{self.base_branch}...HEAD"],
-            show_output=False,
-        ).strip()
-        added_files = set(added_raw.splitlines()) if added_raw else set()
-
-        # New READMEs exactly one directory level under hwdef/
-        # e.g. libraries/AP_HAL_ChibiOS/hwdef/NewBoard/README.md → 4 slashes
-        new_readmes = sorted(
-            f for f in added_files
-            if f.startswith(self.HWDEF_PREFIX)
-            and os.path.basename(f) == "README.md"
-            and f.count("/") == 4
-        )
-
-        if not new_readmes:
-            print(f"{PASS} No new hwdef board READMEs added.")
-            return True
-
-        img_md_re = re.compile(r'!\[[^\]]*\]\(([^)#?\s]+)')
-
-        ok = True
-        for readme_path in new_readmes:
-            board_name = readme_path.split("/")[3]
-
-            if not self.check_board_image_required(board_name):
-                print(f"{PASS} {board_name}: README.md image not required.")
-                continue
-
-            if not os.path.exists(readme_path):
-                print(f"{SKIP} {board_name}: README not present locally, skipping image check.")
-                continue
-
-            with open(readme_path, encoding="utf-8", errors="replace") as fh:
-                content = fh.read()
-
-            readme_dir = os.path.dirname(readme_path)
-            local_refs = []
-            for m in img_md_re.finditer(content):
-                src = m.group(1).strip()
-                if not src.startswith(("http://", "https://", "//")):
-                    local_refs.append(src)
-
-            if not local_refs:
-                print(f"{FAIL} {board_name}: README.md contains no local image references.")
-                ok = False
-                continue
-
-            found = any(
-                os.path.normpath(os.path.join(readme_dir, src)) in added_files
-                for src in local_refs
-            )
-            if found:
-                print(f"{PASS} {board_name}: README.md includes at least one image added in this PR.")
-            else:
-                print(f"{FAIL} {board_name}: README.md has no local images added in this PR.")
-                for src in local_refs:
-                    resolved = os.path.normpath(os.path.join(readme_dir, src))
-                    print(f"         {src!r} → {resolved}")
-                ok = False
-
-        return ok
+    # NOTE: checks concerning new hwdef boards (README.md presence, README
+    # images, defaults.parm contents, and the board build itself) live in
+    # test_new_boards.py, not here.  Add new-board-related checks there to keep
+    # them in one place and avoid the duplication this file once had.
 
     def check_markdown(self) -> bool:
         changed_md = self.run_git(
@@ -762,8 +608,6 @@ def run(self) -> None:
             self.check_author_emails(),
             self.check_submodule_isolation(),
             self.check_submodule_references_exist(),
-            self.check_new_board_has_readme(),
-            self.check_new_board_images(),
             self.check_board_ids(),
             self.check_markdown(),
             self.check_markdown_rst_hyperlinks(),

Tools/scripts/test_new_boards.py

@@ -3,7 +3,13 @@
 '''
 Look at the difference between this and its merge-base with master
 
-for each new hwdef file, build the board
+for each new hwdef file, build the board.
+
+Also validates each new board directory:
+  - it contains a README.md (except ODID variants, which are exempt)
+  - that README.md references at least one image added in this branch
+    (except AP_Periph boards, which need only the README, no image)
+  - defaults.parm does not set parameters that belong in hwdef.dat
 
 AP_FLAKE8_CLEAN
 '''
@@ -32,9 +38,9 @@ def progress_prefix(self) -> str:
         '''pretty-print progress'''
         return 'TNB'
 
-    def get_new_hwdef_paths(self) -> Set[str]:
-        '''returns list of newly added hwdef filepaths relative to the root.
-        Only .dat files for both main and bootloader firmwares'''
+    def get_added_files(self) -> Set[str]:
+        '''returns the set of files newly added (git status 'A') relative to the
+        merge-base with master, as paths relative to the repo root'''
         current_commitish = self.find_current_git_branch_or_sha1()
         merge_base = self.find_git_branch_merge_base(current_commitish, self.master_branch)
         delta_files = self.run_git([
@@ -44,7 +50,6 @@ def get_new_hwdef_paths(self) -> Set[str]:
         ], show_output=False)
 
         ret = set()
-
         for line in delta_files.split("\n"):
             if not line:
                 continue
@@ -55,11 +60,19 @@ def get_new_hwdef_paths(self) -> Set[str]:
             # Only process added files (status 'A')
             if status != 'A':
                 continue
+            ret.add(filepath)
+
+        return ret
+
+    def get_added_hwdef_paths(self) -> Set[str]:
+        '''returns the newly added hwdef filepaths relative to the repo root.
+        Only .dat files for both main and bootloader firmwares'''
+        ret = set()
+        for filepath in self.get_added_files():
             if "/hwdef/" not in filepath:
                 continue
             if not filepath.endswith(("hwdef.dat", "hwdef-bl.dat")):
                 continue
-
             ret.add(filepath)
 
         return ret
@@ -97,6 +110,54 @@ def check_defaults_parm(self, defaults_parm_path: str, board_name: str) -> None:
                 f"defined in hwdef.dat instead:\n" + "\n".join(issues)
             )
 
+    # markdown image reference: ![alt](path), capturing the path
+    IMG_MD_RE = re.compile(r'!\[[^\]]*\]\(([^)#?\s]+)')
+
+    def is_odid_board(self, board: board_list.Board) -> bool:
+        '''True if the board enables OpenDroneID in its hwdef'''
+        hwdef = board.get_hwdef()
+        return hwdef.intdefines.get('AP_OPENDRONEID_ENABLED', 0) == 1
+
+    def check_new_board_readme(self, board_name: str, hwdef_dir: str, added_files: Set[str],
+                               image_required: bool = True) -> None:
+        '''A new board directory must contain a README.md.  Unless image_required
+        is False, that README must also reference at least one local image that is
+        also added in this branch (AP_Periph boards need only the README).'''
+        readme_path = os.path.join(hwdef_dir, 'README.md')
+        if not os.path.exists(readme_path):
+            raise ValueError(f"Missing README.md for new board: {readme_path} does not exist")
+
+        if not image_required:
+            return
+
+        with open(readme_path, encoding="utf-8", errors="replace") as fh:
+            content = fh.read()
+
+        local_refs = []
+        for m in self.IMG_MD_RE.finditer(content):
+            src = m.group(1).strip()
+            if not src.startswith(("http://", "https://", "//")):
+                local_refs.append(src)
+
+        if not local_refs:
+            raise ValueError(
+                f"README.md for new board {board_name} contains no local image references"
+            )
+
+        found = any(
+            os.path.normpath(os.path.join(hwdef_dir, src)) in added_files
+            for src in local_refs
+        )
+        if not found:
+            resolved = "\n".join(
+                f"  {src!r} -> {os.path.normpath(os.path.join(hwdef_dir, src))}"
+                for src in local_refs
+            )
+            raise ValueError(
+                f"README.md for new board {board_name} references no image added in "
+                f"this branch:\n{resolved}"
+            )
+
     def build_board(self, board : board_list.Board):
         self.run_waf(["configure", "--board", board.name], show_output=False)
         if board.is_ap_periph:
@@ -124,7 +185,8 @@ def build_bootloader(self, board : board_list.Board) -> None:
         self.run_waf(["bootloader"], show_output=False)
 
     def run(self) -> None:
-        hwdef_filepaths = self.get_new_hwdef_paths()
+        added_files = self.get_added_files()
+        hwdef_filepaths = self.get_added_hwdef_paths()
 
         @dataclass
         class BoardToTest():
@@ -154,14 +216,17 @@ class BoardToTest():
         # Build each unique board (find it in board list as an additional check)
         bl = board_list.BoardList()
 
-        # First pass: check for README.md for non-AP_Periph boards
+        # First pass: validate README.md and defaults.parm for each new board.
+        # A board directory may contribute both hwdef.dat and hwdef-bl.dat, so
+        # only validate each directory once.
+        checked_board_dirs = set()
         for hwdef_filepath in hwdef_filepaths:
             # Extract board name from path like libraries/AP_HAL_ChibiOS/hwdef/BoardName/hwdef.dat
             parts = hwdef_filepath.split('/')
             hwdef_idx = parts.index('hwdef')
             board_name = parts[hwdef_idx + 1]
 
-            # Find the board object to check if it's AP_Periph
+            # Find the board object to check if it's AP_Periph / ODID
             board = None
             for b in bl.boards:
                 if b.name == board_name:
@@ -172,12 +237,20 @@ class BoardToTest():
                 raise ValueError(f"Board {board_name} not found in board list")
 
             hwdef_dir = os.path.join(*parts[:hwdef_idx + 2])
+            if hwdef_dir in checked_board_dirs:
+                continue
+            checked_board_dirs.add(hwdef_dir)
 
-            # Only require README.md for non-AP_Periph boards
-            if not board.is_ap_periph:
-                readme_path = os.path.join(hwdef_dir, 'README.md')
-                if not os.path.exists(readme_path):
-                    raise ValueError(f"Missing README.md for new board: {readme_path} does not exist")
+            # ODID variants are exempt from the README requirement entirely.
+            # AP_Periph boards must ship a README but are not required to embed
+            # an image.  All other boards require both a README and an image.
+            if self.is_odid_board(board):
+                self.progress(f"README.md not required for {board_name} (ODID)")
+            else:
+                self.check_new_board_readme(
+                    board_name, hwdef_dir, added_files,
+                    image_required=not board.is_ap_periph,
+                )
 
             defaults_parm_path = os.path.join(hwdef_dir, 'defaults.parm')
             if os.path.exists(defaults_parm_path):