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())