From ec2687abc374a3d4191e336b50efb0f5f52a1c12 Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Mon, 28 Sep 2026 13:22:50 -0700 Subject: [PATCH] ci(usb-ids): check board USB IDs on PRs that change them The workflow triggers on pull requests that touch a board defconfig or the USB ID tooling, and checks only the defconfigs the PR adds or modifies. The file list comes from diffing the checked-out PR merge commit against its base parent, so no API call or token is needed. A PR that changes the tooling itself is checked against every board, so a checker change is proven on the real tree and not only by its unit tests. A test keeps the tooling list and the workflow paths filter in sync. A manual run checks every board. There is no push or scheduled run: the registry repo checks its own changes against PX4 main, and board changes reach main only through PRs. The mypy and flake8 step for the checker and runner lives in python_checks.yml with the other Python tooling checks. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Ramon Roche --- .github/workflows/python_checks.yml | 8 +- .github/workflows/usb_ids.yml | 42 +++++++++ Tools/ci/test_usb_ids_runner.py | 140 ++++++++++++++++++++++++++++ Tools/ci/usb_ids_runner.py | 78 ++++++++++++++++ 4 files changed, 267 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/usb_ids.yml create mode 100644 Tools/ci/test_usb_ids_runner.py create mode 100644 Tools/ci/usb_ids_runner.py diff --git a/.github/workflows/python_checks.yml b/.github/workflows/python_checks.yml index 7f65c0322bb..bbaafd54730 100644 --- a/.github/workflows/python_checks.yml +++ b/.github/workflows/python_checks.yml @@ -27,7 +27,7 @@ jobs: python-version: "3.10" - name: Install tools - run: pip install mypy types-requests flake8 pyyaml + run: pip install mypy types-requests types-PyYAML flake8 pyyaml - name: Check MAVSDK test scripts with mypy run: mypy --strict test/mavsdk_tests/*.py @@ -35,6 +35,12 @@ jobs: - name: Check MAVSDK test scripts with flake8 run: flake8 test/mavsdk_tests/*.py + - name: Check USB ID tooling with mypy and flake8 + working-directory: Tools/ci + run: | + mypy --strict check_usb_ids.py usb_ids_runner.py test_check_usb_ids.py test_usb_ids_runner.py + flake8 check_usb_ids.py usb_ids_runner.py test_check_usb_ids.py test_usb_ids_runner.py + - name: Check ROS workspace tooling run: | flake8 Tools/ros2/*.py diff --git a/.github/workflows/usb_ids.yml b/.github/workflows/usb_ids.yml new file mode 100644 index 00000000000..1de0d6a4715 --- /dev/null +++ b/.github/workflows/usb_ids.yml @@ -0,0 +1,42 @@ +name: Dronecode USB ID Registry + +on: + pull_request: + branches: + - '**' + # Keep in sync with TOOLING in Tools/ci/usb_ids_runner.py + paths: + - 'boards/*/*/nuttx-config/*/defconfig' + - '.github/workflows/usb_ids.yml' + - 'Tools/ci/check_usb_ids.py' + - 'Tools/ci/test_check_usb_ids.py' + - 'Tools/ci/test_usb_ids_runner.py' + - 'Tools/ci/usb_ids_runner.py' + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + check: + name: Board USB VID/PID must be registered + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + with: + # the PR merge commit and its base parent, to diff the PR locally + fetch-depth: 2 + + - name: Install PyYAML + run: pip install pyyaml --break-system-packages + + - name: Test the USB ID checker and runner + working-directory: Tools/ci + run: python3 -m unittest -v test_check_usb_ids test_usb_ids_runner + + - name: Check USB IDs against the Dronecode registry + run: python3 Tools/ci/usb_ids_runner.py diff --git a/Tools/ci/test_usb_ids_runner.py b/Tools/ci/test_usb_ids_runner.py new file mode 100644 index 00000000000..5dd500e7d1f --- /dev/null +++ b/Tools/ci/test_usb_ids_runner.py @@ -0,0 +1,140 @@ +"""Check which defconfigs usb_ids_runner.py hands to the checker. + +A selection bug fails open: if a PR resolves to the wrong file list, a +changed board is never checked and the job still goes green. PR diffs +run against a real throwaway git repo shaped like the checkout +actions/checkout makes for a pull request: a merge commit whose first +parent is the base branch. +""" + +import contextlib +import io +import os +from pathlib import Path +import subprocess +import tempfile +from typing import List, Tuple +import unittest +from unittest import mock + +import yaml + +import check_usb_ids +import usb_ids_runner +from usb_ids_runner import changed_files + +WORKFLOW = (Path(__file__).resolve().parents[2] + / '.github/workflows/usb_ids.yml') +ALL = ['boards/px4/fmu-v6xrt/nuttx-config/nsh/defconfig'] +PR_ENV = {'GITHUB_EVENT_NAME': 'pull_request'} +KEPT = 'boards/siyi/n7/nuttx-config/nsh/defconfig' +GONE = 'boards/old/fc/nuttx-config/nsh/defconfig' + + +class ChangedFilesTest(unittest.TestCase): + def setUp(self) -> None: + patcher = mock.patch.object(usb_ids_runner, 'all_defconfigs', + return_value=ALL) + patcher.start() + self.addCleanup(patcher.stop) + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.repo = Path(tmp.name) + cwd = os.getcwd() + os.chdir(self.repo) + self.addCleanup(os.chdir, cwd) + self.git('init', '-q', '-b', 'base') + self.write(GONE) + self.write('README.md') + self.git('add', '.') + self.git('commit', '-q', '-m', 'base') + + def git(self, *args: str) -> None: + subprocess.run(['git', '-c', 'user.name=t', '-c', 'user.email=t@t', + *args], check=True, capture_output=True) + + def write(self, path: str) -> None: + (self.repo / path).parent.mkdir(parents=True, exist_ok=True) + (self.repo / path).write_text(path) + + def merge_pr(self, *files: str) -> None: + """Commit files on a PR branch that also removes GONE, then merge + it into base with a merge commit, as the PR checkout does.""" + self.git('checkout', '-q', '-b', 'pr') + for f in files: + self.write(f) + self.git('rm', '-q', GONE) + self.git('add', '.') + self.git('commit', '-q', '-m', 'pr') + self.git('checkout', '-q', 'base') + self.write('base-moved-on.txt') + self.git('add', '.') + self.git('commit', '-q', '-m', 'base moves on') + self.git('merge', '-q', '--no-ff', '-m', 'merge', 'pr') + + def test_manual_run_checks_the_full_tree(self) -> None: + env = {'GITHUB_EVENT_NAME': 'workflow_dispatch'} + self.assertEqual(changed_files(env), (ALL, True)) + + def test_pull_request_diffs_against_base_without_removed(self) -> None: + # base's own later commits and the PR's deletion are left out + self.merge_pr(KEPT) + self.assertEqual(changed_files(PR_ENV), ([KEPT], False)) + + def test_tooling_change_checks_the_full_tree(self) -> None: + self.merge_pr(KEPT, 'Tools/ci/check_usb_ids.py') + self.assertEqual(changed_files(PR_ENV), (ALL, True)) + + def test_shallow_checkout_without_base_fails_loudly(self) -> None: + # HEAD has no parent here, as with the default fetch-depth 1 + with self.assertRaises(SystemExit) as cm: + changed_files(PR_ENV) + self.assertIn('fetch-depth', str(cm.exception.code)) + + def test_tooling_matches_the_workflow_paths_filter(self) -> None: + # a file missing from the filter never triggers the workflow, and + # one missing from TOOLING never forces the full-tree check + on = yaml.safe_load(WORKFLOW.read_text())[True] + paths = set(on['pull_request']['paths']) + self.assertEqual(paths - {'boards/*/*/nuttx-config/*/defconfig'}, + set(usb_ids_runner.TOOLING)) + + +class MainTest(unittest.TestCase): + def run_main(self, paths: List[str] + ) -> Tuple[int, str, mock.MagicMock, mock.MagicMock]: + out = io.StringIO() + env = {'GITHUB_EVENT_NAME': 'workflow_dispatch'} + with mock.patch.dict(os.environ, env, clear=True), \ + mock.patch.object(usb_ids_runner, 'changed_files', + return_value=(paths, False)), \ + mock.patch.object(check_usb_ids, 'load_registry') as load, \ + mock.patch.object(check_usb_ids, 'cmd_check', + return_value=0) as check, \ + contextlib.redirect_stdout(out): + rc = usb_ids_runner.main() + return rc, out.getvalue(), load, check + + def test_only_board_defconfigs_reach_the_checker(self) -> None: + wanted = ['boards/px4/fmu-v6xrt/nuttx-config/nsh/defconfig', + 'boards/siyi/n7/nuttx-config/bootloader/defconfig'] + ignored = ['boards/px4/fmu-v6xrt/default.px4board', + 'boards/px4/fmu-v6xrt/nuttx-config/nsh/defconfig.orig', + 'boards/px4/defconfig', + 'platforms/nuttx/NuttX/nuttx/boards/arm/stm32/x/configs/' + 'nsh/defconfig', + 'Tools/ci/usb_ids_runner.py'] + rc, _, _, check = self.run_main(ignored + wanted) + self.assertEqual(rc, 0) + self.assertEqual(check.call_args.args[1], wanted) + + def test_no_defconfigs_exits_before_fetching_the_registry(self) -> None: + rc, out, load, check = self.run_main(['Tools/ci/usb_ids_runner.py']) + self.assertEqual(rc, 0) + self.assertIn('No board defconfig changes', out) + load.assert_not_called() + check.assert_not_called() + + +if __name__ == '__main__': + unittest.main() diff --git a/Tools/ci/usb_ids_runner.py b/Tools/ci/usb_ids_runner.py new file mode 100644 index 00000000000..51794a13be6 --- /dev/null +++ b/Tools/ci/usb_ids_runner.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +"""Check board defconfigs against the Dronecode USB ID registry +(https://github.com/Dronecode/usb-ids). + +Meant for the usb_ids.yml workflow; run from the repository root. +A pull request checks the defconfigs it adds or modifies, or every board +defconfig when it changes the USB ID tooling itself. A manual run checks +every board defconfig. + +On a pull request, actions/checkout checks out the test merge commit, +whose first parent is the base branch; the checkout needs fetch-depth 2 +so the PR's changes can be diffed locally against it. +""" + +import glob +import os +import re +import subprocess +import sys +from typing import List, Mapping, Tuple + +import check_usb_ids + +DEFCONFIG_RE = re.compile(r'^boards/[^/]+/[^/]+/nuttx-config/.+/defconfig$') +# Keep in sync with the pull_request paths filter in usb_ids.yml +TOOLING = frozenset(( + '.github/workflows/usb_ids.yml', + 'Tools/ci/check_usb_ids.py', + 'Tools/ci/test_check_usb_ids.py', + 'Tools/ci/test_usb_ids_runner.py', + 'Tools/ci/usb_ids_runner.py', +)) + + +def all_defconfigs() -> List[str]: + return sorted(glob.glob('boards/*/*/nuttx-config/*/defconfig')) + + +def pr_files() -> List[str]: + """Files the checked-out PR merge commit adds or modifies.""" + diff = subprocess.run( + ['git', 'diff', '--name-only', '--diff-filter=d', 'HEAD^1', 'HEAD'], + capture_output=True, text=True) + if diff.returncode != 0: + sys.exit(f'error: cannot diff the PR merge commit against its base ' + f'(is fetch-depth 2?): {diff.stderr.strip()}') + return diff.stdout.splitlines() + + +def changed_files(env: Mapping[str, str]) -> Tuple[List[str], bool]: + """Return (paths, full_tree) for the triggering event.""" + if env.get('GITHUB_EVENT_NAME') != 'pull_request': + return all_defconfigs(), True + + files = pr_files() + if TOOLING.intersection(files): + # a change to the checker itself is proven against every board + return all_defconfigs(), True + return files, False + + +def main() -> int: + paths, full_tree = changed_files(os.environ) + defconfigs = [p for p in paths if DEFCONFIG_RE.match(p)] + if not defconfigs: + print('No board defconfig changes, nothing to check.') + return 0 + + if full_tree: + print(f'Checking all {len(defconfigs)} board defconfigs.') + else: + print('Changed defconfigs:') + print('\n'.join(defconfigs)) + return check_usb_ids.cmd_check(check_usb_ids.load_registry(), defconfigs) + + +if __name__ == '__main__': + sys.exit(main())