From 4da97eb4fdc886bd304abe98add8cef1ea95f11d Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Thu, 5 Mar 2026 16:06:25 -0800 Subject: [PATCH] ci(workflows): add commit message and PR title quality checks Add CI enforcement of conventional commit format for PR titles and commit messages. Includes three Python scripts under Tools/ci/: - conventional_commits.py: shared parsing/validation library - check_pr_title.py: validates PR title format, suggests fixes - check_commit_messages.py: checks commits for blocking errors (fixup/squash/WIP leftovers) and advisory warnings (review-response, formatter-only commits) The workflow (.github/workflows/commit_checks.yml) posts concise GitHub PR comments with actionable suggestions and auto-removes them once issues are resolved. Also updates CONTRIBUTING.md and docs with the conventional commits convention. Signed-off-by: Ramon Roche --- .github/workflows/commit_checks.yml | 148 +++++++++++++ CONTRIBUTING.md | 146 ++++++++++-- Tools/ci/check_commit_messages.py | 331 ++++++++++++++++++++++++++++ Tools/ci/check_pr_title.py | 163 ++++++++++++++ Tools/ci/conventional_commits.py | 146 ++++++++++++ Tools/ci/pr_comment.py | 95 ++++++++ docs/en/contribute/code.md | 49 ++-- docs/en/contribute/git_examples.md | 6 +- 8 files changed, 1036 insertions(+), 48 deletions(-) create mode 100644 .github/workflows/commit_checks.yml create mode 100755 Tools/ci/check_commit_messages.py create mode 100755 Tools/ci/check_pr_title.py create mode 100644 Tools/ci/conventional_commits.py create mode 100755 Tools/ci/pr_comment.py diff --git a/.github/workflows/commit_checks.yml b/.github/workflows/commit_checks.yml new file mode 100644 index 0000000000..a011a47d6f --- /dev/null +++ b/.github/workflows/commit_checks.yml @@ -0,0 +1,148 @@ +name: Commit Quality + +on: + pull_request: + types: [opened, edited, synchronize, reopened] + +permissions: + contents: read + pull-requests: write + issues: write + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +env: + PR_NUMBER: ${{ github.event.pull_request.number }} + IS_FORK: ${{ github.event.pull_request.head.repo.full_name != github.repository }} + +jobs: + pr-title: + name: PR Title + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + sparse-checkout: Tools/ci + fetch-depth: 1 + + - name: Check PR title + id: check + run: | + python3 Tools/ci/check_pr_title.py "${{ github.event.pull_request.title }}" --markdown-file comment.md && rc=0 || rc=$? + echo "exit_code=$rc" >> "$GITHUB_OUTPUT" + + - name: Post or clear comment + if: env.IS_FORK == 'false' + env: + GH_TOKEN: ${{ github.token }} + run: | + if [ "${{ steps.check.outputs.exit_code }}" != "0" ]; then + python3 Tools/ci/pr_comment.py --marker pr-title --pr "$PR_NUMBER" --result fail < comment.md + else + python3 Tools/ci/pr_comment.py --marker pr-title --pr "$PR_NUMBER" --result pass + fi + + - name: Result + if: steps.check.outputs.exit_code != '0' + run: | + echo "::error::PR title does not follow conventional commits format. See the PR comment for details." + exit 1 + + commit-messages: + name: Commit Messages + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + sparse-checkout: Tools/ci + fetch-depth: 1 + + - name: Check commit messages + id: check + env: + GH_TOKEN: ${{ github.token }} + run: | + gh api \ + "repos/${{ github.repository }}/pulls/${PR_NUMBER}/commits?per_page=100" \ + | python3 Tools/ci/check_commit_messages.py --markdown-file comment.md && rc=0 || rc=$? + echo "exit_code=$rc" >> "$GITHUB_OUTPUT" + # Check for warnings (non-empty markdown on exit 0) + if [ "$rc" -eq 0 ] && [ -s comment.md ]; then + echo "has_warnings=true" >> "$GITHUB_OUTPUT" + else + echo "has_warnings=false" >> "$GITHUB_OUTPUT" + fi + + - name: Post or clear comment + if: env.IS_FORK == 'false' + env: + GH_TOKEN: ${{ github.token }} + run: | + if [ "${{ steps.check.outputs.exit_code }}" != "0" ]; then + python3 Tools/ci/pr_comment.py --marker commit-msgs --pr "$PR_NUMBER" --result fail < comment.md + elif [ "${{ steps.check.outputs.has_warnings }}" == "true" ]; then + python3 Tools/ci/pr_comment.py --marker commit-msgs --pr "$PR_NUMBER" --result warn < comment.md + else + python3 Tools/ci/pr_comment.py --marker commit-msgs --pr "$PR_NUMBER" --result pass + fi + + - name: Result + if: steps.check.outputs.exit_code != '0' + run: | + echo "::error::Commit message errors found. See the PR comment for details." + exit 1 + + pr-body: + name: PR Description + runs-on: ubuntu-latest + steps: + - name: Checkout + if: env.IS_FORK == 'false' + uses: actions/checkout@v4 + with: + sparse-checkout: Tools/ci + fetch-depth: 1 + + - name: Check PR body + id: check + env: + PR_BODY: ${{ github.event.pull_request.body }} + run: | + message="" + if [ -z "$PR_BODY" ]; then + message="PR description is empty. Please add a summary of the changes." + echo "::warning::PR description is empty." + else + cleaned=$(echo "$PR_BODY" | sed 's///g' | tr -d '[:space:]') + if [ -z "$cleaned" ]; then + message="PR description contains only template comments. Please fill in the details." + echo "::warning::PR description contains only template comments." + fi + fi + echo "message=$message" >> "$GITHUB_OUTPUT" + + - name: Post or clear comment + if: env.IS_FORK == 'false' + env: + GH_TOKEN: ${{ github.token }} + run: | + if [ -n "${{ steps.check.outputs.message }}" ]; then + printf '%s\n' \ + "## PR Description (advisory)" \ + "" \ + "This is **not blocking**, but your PR description appears to be empty or incomplete." \ + "" \ + "${{ steps.check.outputs.message }}" \ + "" \ + "A good PR description helps reviewers understand what changed and why." \ + "" \ + "---" \ + "*This comment will be automatically removed once the issue is resolved.*" \ + | python3 Tools/ci/pr_comment.py --marker pr-body --pr "$PR_NUMBER" --result warn + else + python3 Tools/ci/pr_comment.py --marker pr-body --pr "$PR_NUMBER" --result pass + fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cce5340cc7..5c4d19a922 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,44 +1,150 @@ -# Contributing to PX4 Firmware +# Contributing to PX4-Autopilot -We follow the [Github flow](https://guides.github.com/introduction/flow/) development model. +We follow the [GitHub flow](https://guides.github.com/introduction/flow/) development model. -### Fork the project, then clone your repo +## Fork the project, then clone your repo -First [fork and clone](https://help.github.com/articles/fork-a-repo) the project project. +First [fork and clone](https://help.github.com/articles/fork-a-repo) the project. -### Create a feature branch +## Create a feature branch -*Always* branch off main for new features. +Always branch off `main` for new features. ``` git checkout -b mydescriptivebranchname ``` -### Edit and build the code +## Edit and build the code The [developer guide](https://docs.px4.io/main/en/development/development.html) explains how to set up the development environment on Mac OS, Linux or Windows. Please take note of our [coding style](https://docs.px4.io/main/en/contribute/code.html) when editing files. -### Commit your changes +## Commit message convention -Always write descriptive commit messages and add a fixes or relates note to them with an [issue number](https://github.com/px4/Firmware/issues) (Github will link these then conveniently) +PX4 uses [conventional commits](https://www.conventionalcommits.org/) for all commit messages and PR titles. -**Example:** +### Format ``` -Change how the attitude controller works - -- Fixes rate feed forward -- Allows a local body rate override - -Fixes issue #123 +type(scope): short description of the change ``` -### Test your changes +| Part | Rule | +|------|------| +| **type** | Category of change (see types table below) | +| **scope** | The module, driver, board, or area of PX4 affected | +| **`!`** (optional) | Append before `:` to mark a breaking change | +| **description** | What the change does, at least 5 characters, written in imperative form | -Since we care about safety, we will regularly ask you for test results. Best is to do a test flight (or bench test where it applies) and upload the logfile from it (on the microSD card in the logs directory) to Google Drive or Dropbox and share the link. +### Types -### Push your changes +| Type | Description | +|------|-------------| +| `feat` | A new feature | +| `fix` | A bug fix | +| `docs` | Documentation only changes | +| `style` | Formatting, whitespace, no code change | +| `refactor` | Code change that neither fixes a bug nor adds a feature | +| `perf` | Performance improvement | +| `test` | Adding or correcting tests | +| `build` | Build system or external dependencies | +| `ci` | CI configuration files and scripts | +| `chore` | Other changes that don't modify src or test files | +| `revert` | Reverts a previous commit | -Push changes to your repo and send a [pull request](https://github.com/PX4/Firmware/compare/). +### Scopes + +The scope identifies which part of PX4 is affected. Common scopes: + +| Scope | Area | +|-------|------| +| `ekf2` | Extended Kalman Filter (state estimation) | +| `mavlink` | MAVLink messaging protocol | +| `commander` | Commander and mode management | +| `navigator` | Mission, RTL, Land, and other navigation modes | +| `sensors` | Sensor drivers and processing | +| `drivers` | Hardware drivers | +| `boards/px4_fmu-v6x` | Board-specific changes (use the board name) | +| `mc_att_control` | Multicopter attitude control | +| `mc_pos_control` | Multicopter position control | +| `fw_att_control` | Fixed-wing attitude control | +| `vtol` | VTOL-specific logic | +| `actuators` | Mixer and actuator output | +| `battery` | Battery monitoring and estimation | +| `logger` | On-board logging | +| `param` | Parameter system | +| `simulation` | SITL, Gazebo, SIH | +| `ci` | Continuous integration and workflows | +| `docs` | Documentation | +| `build` | CMake, toolchain, build system | +| `uorb` | Inter-module messaging | + +For changes spanning multiple subsystems, use the primary one affected. Look at the directory path of the files you changed to find the right scope: `src/modules/ekf2/` uses `ekf2`, `src/drivers/imu/` uses `drivers/imu`, `.github/workflows/` uses `ci`. + +### Breaking changes + +Append `!` before the colon to indicate a breaking change: + +``` +feat(ekf2)!: remove deprecated height fusion API +``` + +### Good commit messages + +``` +feat(ekf2): add height fusion timeout +fix(mavlink): correct BATTERY_STATUS_V2 parsing +refactor(navigator): simplify RTL altitude logic +ci(workflows): migrate to reusable workflows +docs(ekf2): update tuning guide +feat(boards/px4_fmu-v6x)!: remove deprecated driver API +perf(mc_rate_control): reduce loop latency +``` + +### Commits to avoid + +These will be flagged by CI and should be squashed or reworded before merging: + +``` +fix # too vague, no type or scope +update # too vague, no type or scope +ekf2: fix something # missing type prefix +apply suggestions from code review # squash into parent commit +do make format # squash into parent commit +WIP: trying something # not ready for main +oops # not descriptive +``` + +### PR titles + +The PR title follows the same `type(scope): description` format. This is enforced by CI and is especially important because the PR title becomes the commit message when a PR is squash-merged. + +### Merge policy + +Commits should be atomic and independently revertable. Squash at reviewer discretion for obvious cases (multiple WIP commits, messy review-response history). When your commits are clean and logical, they will be preserved as individual commits on `main`. + +### Cleaning up commits + +If CI flags your commit messages, you can fix them with an interactive rebase: + +```bash +# Squash all commits into one: +git rebase -i HEAD~N # replace N with the number of commits +# mark all commits except the first as 'squash' or 'fixup' +# reword the remaining commit to follow the format +git push --force-with-lease + +# Or reword specific commits: +git rebase -i HEAD~N +# mark the bad commits as 'reword' +git push --force-with-lease +``` + +## Test your changes + +Since we care about safety, we will regularly ask you for test results. Best is to do a test flight (or bench test where it applies) and upload the log file from it (on the microSD card in the logs directory) to Google Drive or Dropbox and share the link. + +## Push your changes + +Push changes to your repo and send a [pull request](https://github.com/PX4/PX4-Autopilot/compare/). Make sure to provide some testing feedback and if possible the link to a flight log file. Upload flight log files to [Flight Review](http://logs.px4.io) and link the resulting report. diff --git a/Tools/ci/check_commit_messages.py b/Tools/ci/check_commit_messages.py new file mode 100755 index 0000000000..5359f737cb --- /dev/null +++ b/Tools/ci/check_commit_messages.py @@ -0,0 +1,331 @@ +#!/usr/bin/env python3 +"""Validate commit messages in a PR against conventional commits format. + +Reads a JSON array of GitHub commit objects from stdin (as returned by the +GitHub API's /pulls/{n}/commits endpoint) and checks each message for +blocking errors and advisory warnings. + +With --markdown, outputs a formatted PR comment body instead of plain text. +""" + +import json +import sys + +from conventional_commits import ( + EXEMPT_PREFIXES, + parse_header, +) + +# Blocking: prefixes that indicate unsquashed fixup commits +FIXUP_PREFIXES = ('fixup!', 'squash!', 'amend!') + +# Blocking: single-word throwaway messages (case-insensitive exact match) +THROWAWAY_WORDS = frozenset({ + 'fix', 'fixed', 'fixes', + 'update', 'updated', 'updates', + 'test', 'tests', 'testing', + 'tmp', 'temp', + 'oops', 'wip', + 'debug', 'cleanup', +}) + +# Blocking: debug session leftovers +DEBUG_KEYWORDS = ('tmate',) + +# Warning: review-response messages (case-insensitive substring match) +REVIEW_RESPONSE_PATTERNS = ( + 'address review', + 'apply suggestions from code review', + 'code review', +) + +# Warning: formatter-only commits +FORMATTER_PATTERNS = ( + 'do make format', + 'make format', + 'run formatter', + 'apply format', +) + +MIN_MESSAGE_LENGTH = 5 + + +def check_commit(message: str) -> tuple[list[str], list[str]]: + """Return (errors, warnings) for a single commit message.""" + errors: list[str] = [] + warnings: list[str] = [] + + first_line = message.split('\n', 1)[0].strip() + lower = first_line.lower() + + # --- Blocking checks --- + + for prefix in FIXUP_PREFIXES: + if lower.startswith(prefix): + errors.append(f'Unsquashed commit: starts with "{prefix}"') + + if lower == 'wip' or lower.startswith('wip ') or lower.startswith('wip:'): + errors.append('WIP commit should not be merged') + + if len(first_line) < MIN_MESSAGE_LENGTH: + errors.append(f'Message too short ({len(first_line)} chars, minimum {MIN_MESSAGE_LENGTH})') + + if first_line.strip() and first_line.strip().lower() in THROWAWAY_WORDS: + errors.append(f'Single-word throwaway message: "{first_line.strip()}"') + + for kw in DEBUG_KEYWORDS: + if kw in lower: + errors.append(f'Debug session leftover: contains "{kw}"') + + # --- Warning checks --- + + for pattern in REVIEW_RESPONSE_PATTERNS: + if pattern in lower: + warnings.append('Review-response commit') + break + + for pattern in FORMATTER_PATTERNS: + if pattern in lower: + warnings.append('Formatter-only commit') + break + + if not parse_header(first_line): + # Exempt merge commits + for prefix in EXEMPT_PREFIXES: + if first_line.startswith(prefix): + break + else: + warnings.append( + 'Missing conventional commit format ' + '(e.g. "feat(ekf2): add something")' + ) + + return errors, warnings + + +def suggest_commit(message: str) -> str | None: + """Suggest how to fix a bad commit message.""" + first_line = message.split('\n', 1)[0].strip() + lower = first_line.lower() + + for prefix in FIXUP_PREFIXES: + if lower.startswith(prefix): + return 'Squash this into the commit it fixes' + + if lower == 'wip' or lower.startswith('wip ') or lower.startswith('wip:'): + return 'Reword with a descriptive message (e.g. "feat(scope): what changed")' + + if len(first_line) < MIN_MESSAGE_LENGTH: + return 'Reword with a descriptive message (e.g. "feat(ekf2): what changed")' + + if first_line.strip().lower() in THROWAWAY_WORDS: + return 'Reword with a descriptive message (e.g. "fix(scope): what changed")' + + return None + + +def format_plain(data: list) -> tuple[bool, bool]: + """Print plain text output. Returns (has_blocking, has_warnings).""" + has_blocking = False + has_warnings = False + + for commit in data: + sha = commit.get('sha', '?')[:10] + message = commit.get('commit', {}).get('message', '') + first_line = message.split('\n', 1)[0].strip() + + errors, warnings = check_commit(message) + + if errors or warnings: + print(f"\n {sha} {first_line}") + + for err in errors: + print(f" ERROR: {err}") + has_blocking = True + + for warn in warnings: + print(f" WARNING: {warn}") + has_warnings = True + + if has_blocking: + print( + "\n" + "ERROR = must fix before merging (CI will block the PR)\n" + "WARNING = advisory, not blocking, but recommended to fix\n" + "\n" + "See the contributing guide for details:\n" + " https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention\n", + ) + + elif has_warnings: + print( + "\n" + "WARNING = advisory, not blocking, but recommended to fix\n" + "\n" + "See the contributing guide for details:\n" + " https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention\n", + ) + + return has_blocking, has_warnings + + +def format_markdown_blocking(data: list) -> str: + """Format a blocking error markdown comment.""" + error_groups: dict[str, list[str]] = {} + unique_commits: list[tuple[str, str, list[str], str]] = [] + + for commit in data: + sha = commit.get('sha', '?')[:10] + message = commit.get('commit', {}).get('message', '') + first_line = message.split('\n', 1)[0].strip() + + errors, _ = check_commit(message) + if not errors: + continue + + suggestion = suggest_commit(message) or '' + unique_commits.append((sha, first_line, errors, suggestion)) + + for err in errors: + error_groups.setdefault(err, []).append(sha) + + lines = [ + "## \u274c Commit messages need attention before merging", + "", + ] + + has_large_group = any(len(shas) > 3 for shas in error_groups.values()) + + if has_large_group: + lines.extend([ + "**Issues found:**", + "", + ]) + for err_msg, shas in error_groups.items(): + if len(shas) > 3: + lines.append(f"- **{len(shas)} commits**: {err_msg} " + f"(`{shas[0]}`, `{shas[1]}`, ... `{shas[-1]}`)") + else: + sha_list = ', '.join(f'`{s}`' for s in shas) + lines.append(f"- {err_msg}: {sha_list}") + + distinct_messages = {msg for _, msg, _, _ in unique_commits} + if len(distinct_messages) <= 5: + lines.extend(["", "**Affected commits:**", ""]) + for sha, msg, errors, suggestion in unique_commits: + safe_msg = msg.replace('|', '\\|') + lines.append(f"- `{sha}` {safe_msg}") + else: + lines.extend([ + "| Commit | Message | Issue | Suggested fix |", + "|--------|---------|-------|---------------|", + ]) + for sha, msg, errors, suggestion in unique_commits: + issues = '; '.join(errors) + safe_msg = msg.replace('|', '\\|') + lines.append(f"| `{sha}` | {safe_msg} | {issues} | {suggestion} |") + + lines.extend([ + "", + "See [CONTRIBUTING.md](https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention) " + "for how to clean up commits.", + "", + "---", + "*This comment will be automatically removed once the issues are resolved.*", + ]) + + return '\n'.join(lines) + + +def format_markdown_advisory(data: list) -> str: + """Format an advisory warning markdown comment.""" + lines = [ + "## \U0001f4a1 Commit messages could be improved", + "", + "Not blocking, but these commit messages could use some cleanup.", + "", + "| Commit | Message | Suggestion |", + "|--------|---------|------------|", + ] + + for commit in data: + sha = commit.get('sha', '?')[:10] + message = commit.get('commit', {}).get('message', '') + first_line = message.split('\n', 1)[0].strip() + + _, warnings = check_commit(message) + if not warnings: + continue + + suggestion = '; '.join(warnings) + safe_msg = first_line.replace('|', '\\|') + lines.append(f"| `{sha}` | {safe_msg} | {suggestion} |") + + lines.extend([ + "", + "See the [commit message convention](https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention) " + "for details.", + "", + "---", + "*This comment will be automatically removed once the issues are resolved.*", + ]) + + return '\n'.join(lines) + + +def main() -> None: + markdown_stdout = '--markdown' in sys.argv + markdown_file = None + for i, a in enumerate(sys.argv): + if a == '--markdown-file' and i + 1 < len(sys.argv): + markdown_file = sys.argv[i + 1] + elif a.startswith('--markdown-file='): + markdown_file = a.split('=', 1)[1] + + try: + data = json.load(sys.stdin) + except json.JSONDecodeError as exc: + print(f"Failed to parse JSON input: {exc}", file=sys.stderr) + sys.exit(2) + + if not isinstance(data, list): + print("Expected a JSON array of commit objects.", file=sys.stderr) + sys.exit(2) + + # Always compute blocking/warning state + has_blocking = False + has_warnings = False + for commit in data: + message = commit.get('commit', {}).get('message', '') + errors, warnings = check_commit(message) + if errors: + has_blocking = True + if warnings: + has_warnings = True + + # Generate markdown if needed + md = None + if has_blocking: + md = format_markdown_blocking(data) + elif has_warnings: + md = format_markdown_advisory(data) + + if md: + if markdown_stdout: + print(md) + if markdown_file: + with open(markdown_file, 'w') as f: + f.write(md + '\n') + elif markdown_file: + with open(markdown_file, 'w') as f: + pass + + # Plain text output to stderr for CI logs (always, unless --markdown only) + if not markdown_stdout: + has_blocking, _ = format_plain(data) + + sys.exit(1 if has_blocking else 0) + + +if __name__ == '__main__': + main() diff --git a/Tools/ci/check_pr_title.py b/Tools/ci/check_pr_title.py new file mode 100755 index 0000000000..5ef2663f8a --- /dev/null +++ b/Tools/ci/check_pr_title.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python3 +"""Validate that a PR title follows conventional commits format. + +Format: type(scope): description + +Can output plain text for CI logs or markdown for PR comments. +""" + +import re +import sys + +from conventional_commits import ( + CONVENTIONAL_TYPES, + EXEMPT_PREFIXES, + parse_header, + suggest_scope, + suggest_type, +) + + +def suggest_title(title: str) -> str | None: + """Try to suggest a corrected title in conventional commits format.""" + stripped = title.strip() + + # Remove common bracket prefixes like [docs], [CI], etc. + bracket_match = re.match(r'^\[([^\]]+)\]\s*(.+)', stripped) + if bracket_match: + prefix = bracket_match.group(1).strip().lower() + rest = bracket_match.group(2).strip() + rest = re.sub(r'^[\-:]\s*', '', rest).strip() + if len(rest) >= 5: + # Try to map bracket content to a type + commit_type = prefix if prefix in CONVENTIONAL_TYPES else suggest_type(rest) + scope = suggest_scope(rest) + if scope: + return f"{commit_type}({scope}): {rest}" + + # Already has old-style "subsystem: description" format - convert it + colon_match = re.match(r'^([a-zA-Z][a-zA-Z0-9_/\-\. ]*): (.+)$', stripped) + if colon_match: + old_subsystem = colon_match.group(1).strip() + desc = colon_match.group(2).strip() + if len(desc) >= 5: + commit_type = suggest_type(desc) + # Use the old subsystem as scope (clean it up) + scope = old_subsystem.lower().replace(' ', '_') + return f"{commit_type}({scope}): {desc}" + + # No format at all - try to guess both type and scope + commit_type = suggest_type(stripped) + scope = suggest_scope(stripped) + if scope: + desc = stripped[0].lower() + stripped[1:] if stripped else stripped + return f"{commit_type}({scope}): {desc}" + + return None + + +def check_title(title: str) -> bool: + title = title.strip() + + if not title: + print("PR title is empty.", file=sys.stderr) + return False + + for prefix in EXEMPT_PREFIXES: + if title.startswith(prefix): + return True + + if parse_header(title): + return True + + types_str = ', '.join(f'`{t}`' for t in CONVENTIONAL_TYPES.keys()) + print( + f"PR title does not match conventional commits format.\n" + f"\n" + f" Title: {title}\n" + f"\n" + f"Expected format: type(scope): description\n" + f"\n" + f"Valid types: {types_str}\n" + f"\n" + f"Good examples:\n" + f" feat(ekf2): add height fusion timeout\n" + f" fix(mavlink): correct BATTERY_STATUS_V2 parsing\n" + f" ci(workflows): migrate to reusable workflows\n" + f" feat(boards/px4_fmu-v6x)!: remove deprecated driver API\n" + f"\n" + f"Bad examples:\n" + f" fix stuff\n" + f" Update file\n" + f" ekf2: fix something (missing type prefix)\n" + f"\n" + f"See the contributing guide for details:\n" + f" https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention\n", + file=sys.stderr, + ) + return False + + +def format_markdown(title: str) -> str: + """Format a markdown PR comment body for a bad title.""" + lines = [ + "## \u274c PR title needs conventional commit format", + "", + "Expected format: `type(scope): description` " + "([conventional commits](https://www.conventionalcommits.org/)).", + "", + "**Your title:**", + f"> {title}", + "", + ] + + suggestion = suggest_title(title) + if suggestion: + lines.extend([ + "**Suggested fix:**", + f"> {suggestion}", + "", + ]) + + lines.extend([ + "**To fix this:** click the ✏️ next to the PR title at the top " + "of this page and update it.", + "", + "See [CONTRIBUTING.md](https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention) " + "for details.", + "", + "---", + "*This comment will be automatically removed once the issue is resolved.*", + ]) + + return '\n'.join(lines) + + +def main() -> None: + import argparse + parser = argparse.ArgumentParser(description='Check PR title format') + parser.add_argument('title', help='The PR title to validate') + parser.add_argument('--markdown', action='store_true', + help='Output markdown to stdout on failure') + parser.add_argument('--markdown-file', metavar='FILE', + help='Write markdown to FILE on failure') + args = parser.parse_args() + + passed = check_title(args.title) + + if not passed: + md = format_markdown(args.title) + if args.markdown: + print(md) + if args.markdown_file: + with open(args.markdown_file, 'w') as f: + f.write(md + '\n') + elif args.markdown_file: + with open(args.markdown_file, 'w') as f: + pass + + sys.exit(0 if passed else 1) + + +if __name__ == '__main__': + main() diff --git a/Tools/ci/conventional_commits.py b/Tools/ci/conventional_commits.py new file mode 100644 index 0000000000..31a18e0580 --- /dev/null +++ b/Tools/ci/conventional_commits.py @@ -0,0 +1,146 @@ +"""Shared constants and helpers for conventional commit validation. + +Format: type(scope): description +Optional breaking change marker: type(scope)!: description +""" + +import re + +CONVENTIONAL_TYPES = { + 'feat': 'A new feature', + 'fix': 'A bug fix', + 'docs': 'Documentation only changes', + 'style': 'Formatting, whitespace, no code change', + 'refactor': 'Code change that neither fixes a bug nor adds a feature', + 'perf': 'Performance improvement', + 'test': 'Adding or correcting tests', + 'build': 'Build system or external dependencies', + 'ci': 'CI configuration files and scripts', + 'chore': 'Other changes that don\'t modify src or test files', + 'revert': 'Reverts a previous commit', +} + +# type(scope)[!]: description +# - type: one of CONVENTIONAL_TYPES keys +# - scope: required, alphanumeric with _/-/. +# - !: optional breaking change marker +# - description: at least 5 chars +HEADER_PATTERN = re.compile( + r'^(' + '|'.join(CONVENTIONAL_TYPES.keys()) + r')' + r'\(([a-zA-Z0-9_/\-\.]+)\)' + r'(!)?' + r': (.{5,})$' +) + +EXEMPT_PREFIXES = ('Merge ',) + +# Common PX4 subsystem scopes for suggestions +KNOWN_SCOPES = [ + 'ekf2', 'mavlink', 'commander', 'navigator', 'sensors', + 'mc_att_control', 'mc_pos_control', 'mc_rate_control', + 'fw_att_control', 'fw_pos_control', 'fw_rate_control', + 'vtol', 'actuators', 'battery', 'param', 'logger', + 'uorb', 'drivers', 'boards', 'simulation', 'sitl', + 'gps', 'rc', 'safety', 'can', 'serial', + 'ci', 'docs', 'build', 'cmake', 'tools', + 'mixer', 'land_detector', 'airspeed', 'gyroscope', + 'accelerometer', 'magnetometer', 'barometer', +] + +# Keyword patterns to suggest scopes from description text +KEYWORD_SCOPES = [ + (r'\b(ekf|estimator|height|fusion|imu|baro)\b', 'ekf2'), + (r'\b(mavlink|MAVLink|MAVLINK|command_int|heartbeat)\b', 'mavlink'), + (r'\b(uorb|orb|pub|sub|topic)\b', 'uorb'), + (r'\b(board|fmu|nuttx|stm32)\b', 'boards'), + (r'\b(mixer|actuator|motor|servo|pwm|dshot)\b', 'actuators'), + (r'\b(battery|power)\b', 'battery'), + (r'\b(param|parameter)\b', 'param'), + (r'\b(log|logger|sdlog)\b', 'logger'), + (r'\b(sensor|accel|gyro)\b', 'sensors'), + (r'\b(land|takeoff|rtl|mission|navigator|geofence)\b', 'navigator'), + (r'\b(position|velocity|attitude|rate)\s*(control|ctrl)\b', 'mc_att_control'), + (r'\b(mc|multicopter|quad)\b', 'mc_att_control'), + (r'\b(fw|fixedwing|fixed.wing|plane)\b', 'fw_att_control'), + (r'\b(vtol|transition)\b', 'vtol'), + (r'\b(ci|workflow|github.action|pipeline)\b', 'ci'), + (r'\b(doc|docs|documentation|readme)\b', 'docs'), + (r'\b(cmake|make|toolchain|compiler)\b', 'build'), + (r'\b(sitl|simulation|gazebo|jmavsim|sih)\b', 'simulation'), + (r'\b(can|uavcan|cyphal|dronecan)\b', 'can'), + (r'\b(serial|uart|spi|i2c)\b', 'serial'), + (r'\b(safety|failsafe|arm|disarm|kill)\b', 'safety'), + (r'\b(rc|radio|sbus|crsf|elrs|dsm)\b', 'rc'), + (r'\b(gps|gnss|rtk|ubx)\b', 'gps'), + (r'\b(optical.flow|flow|rangefinder|lidar|distance)\b', 'sensors'), + (r'\b(orbit|follow|offboard)\b', 'commander'), + (r'\b(driver)\b', 'drivers'), +] + +# Verb patterns to suggest conventional commit type +VERB_TYPE_MAP = [ + (r'^fix(e[ds])?[\s:]', 'fix'), + (r'^bug[\s:]', 'fix'), + (r'^add(s|ed|ing)?[\s:]', 'feat'), + (r'^implement', 'feat'), + (r'^introduce', 'feat'), + (r'^support', 'feat'), + (r'^enable', 'feat'), + (r'^update[ds]?[\s:]', 'feat'), + (r'^improv(e[ds]?|ing)', 'perf'), + (r'^optimi[zs](e[ds]?|ing)', 'perf'), + (r'^refactor', 'refactor'), + (r'^clean\s*up', 'refactor'), + (r'^restructure', 'refactor'), + (r'^simplif(y|ied)', 'refactor'), + (r'^remov(e[ds]?|ing)', 'refactor'), + (r'^delet(e[ds]?|ing)', 'refactor'), + (r'^deprecat', 'refactor'), + (r'^replac(e[ds]?|ing)', 'refactor'), + (r'^renam(e[ds]?|ing)', 'refactor'), + (r'^migrat', 'refactor'), + (r'^revert', 'revert'), + (r'^doc(s|ument)', 'docs'), + (r'^test', 'test'), + (r'^format', 'style'), + (r'^lint', 'style'), + (r'^whitespace', 'style'), + (r'^build', 'build'), + (r'^ci[\s:]', 'ci'), +] + + +def parse_header(text: str) -> dict | None: + """Parse a conventional commit header into components. + + Returns dict with keys {type, scope, breaking, subject} or None if + the text doesn't match conventional commits format. + """ + text = text.strip() + m = HEADER_PATTERN.match(text) + if not m: + return None + return { + 'type': m.group(1), + 'scope': m.group(2), + 'breaking': m.group(3) == '!', + 'subject': m.group(4), + } + + +def suggest_type(text: str) -> str: + """Infer a conventional commit type from description text.""" + lower = text.strip().lower() + for pattern, commit_type in VERB_TYPE_MAP: + if re.search(pattern, lower): + return commit_type + return 'feat' + + +def suggest_scope(text: str) -> str | None: + """Infer a scope from keywords in the text.""" + lower = text.strip().lower() + for pattern, scope in KEYWORD_SCOPES: + if re.search(pattern, lower, re.IGNORECASE): + return scope + return None diff --git a/Tools/ci/pr_comment.py b/Tools/ci/pr_comment.py new file mode 100755 index 0000000000..6306b13ef4 --- /dev/null +++ b/Tools/ci/pr_comment.py @@ -0,0 +1,95 @@ +#!/usr/bin/env python3 +"""Post, update, or delete a PR comment with deduplication. + +Uses hidden HTML markers to find existing comments and avoid duplicates. +Reads comment body from stdin when posting or updating. + +Usage: + echo "comment body" | python3 pr_comment.py --marker pr-title --pr 123 --result fail + python3 pr_comment.py --marker pr-title --pr 123 --result pass + +Results: + fail - post/update comment with body from stdin + warn - post/update comment with body from stdin + pass - delete existing comment if any + +Requires GH_TOKEN and GITHUB_REPOSITORY environment variables. +""" + +import argparse +import json +import os +import subprocess +import sys + + +def gh_api(endpoint: str, method: str = 'GET', body: dict | None = None) -> str: + """Call the GitHub API via gh cli.""" + cmd = ['gh', 'api', endpoint, '-X', method] + if body: + for key, value in body.items(): + cmd.extend(['-f', f'{key}={value}']) + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0 and method != 'DELETE': + print(f"gh api error: {result.stderr}", file=sys.stderr) + return result.stdout + + +def find_comment(repo: str, pr: int, marker: str) -> str | None: + """Find an existing comment by its hidden marker. Returns comment ID or None.""" + response = gh_api(f"repos/{repo}/issues/{pr}/comments?per_page=100") + try: + comments = json.loads(response) + except json.JSONDecodeError: + return None + + if not isinstance(comments, list): + return None + + for comment in comments: + if isinstance(comment, dict) and comment.get('body', '').startswith(marker): + return str(comment['id']) + return None + + +def main() -> None: + parser = argparse.ArgumentParser(description='Manage PR quality comments') + parser.add_argument('--marker', required=True, + help='Marker name (e.g. pr-title, commit-msgs, pr-body)') + parser.add_argument('--pr', required=True, type=int, + help='Pull request number') + parser.add_argument('--result', required=True, choices=['pass', 'fail', 'warn'], + help='Check result: pass deletes comment, fail/warn posts it') + args = parser.parse_args() + + repo = os.environ.get('GITHUB_REPOSITORY', '') + if not repo: + print("GITHUB_REPOSITORY not set", file=sys.stderr) + sys.exit(2) + + marker = f"" + existing_id = find_comment(repo, args.pr, marker) + + if args.result == 'pass': + if existing_id: + gh_api(f"repos/{repo}/issues/comments/{existing_id}", method='DELETE') + return + + # Read comment body from stdin + body_content = sys.stdin.read().strip() + if not body_content: + print("No comment body provided on stdin", file=sys.stderr) + sys.exit(2) + + full_body = f"{marker}\n{body_content}" + + if existing_id: + gh_api(f"repos/{repo}/issues/comments/{existing_id}", method='PATCH', + body={'body': full_body}) + else: + gh_api(f"repos/{repo}/issues/{args.pr}/comments", method='POST', + body={'body': full_body}) + + +if __name__ == '__main__': + main() diff --git a/docs/en/contribute/code.md b/docs/en/contribute/code.md index 0d51f2f120..d2b5239ca4 100644 --- a/docs/en/contribute/code.md +++ b/docs/en/contribute/code.md @@ -149,36 +149,35 @@ else { ## Commits and Commit Messages -Use descriptive, multi-paragraph commit messages for all non-trivial changes. -Structure them well so they make sense in the one-line summary but also provide full detail. +PX4 uses [conventional commits](https://www.conventionalcommits.org/) for all commit messages and PR titles. -```plain -Component: Explain the change in one sentence. Fixes #1234 +### Format -Prepend the software component to the start of the summary -line, either by the module name or a description of it. -(e.g. "mc_att_ctrl" or "multicopter attitude controller"). - -If the issue number is appended as , Github -will automatically close the issue when the commit is -merged to the master branch. - -The body of the message can contain several paragraphs. -Describe in detail what you changed. Link issues and flight -logs either related to this fix or to the testing results -of this commit. - -Describe the change and why you changed it, avoid to -paraphrase the code change (Good: "Adds an additional -safety check for vehicles with low quality GPS reception". -Bad: "Add gps_reception_check() function"). - -Reported-by: Name +``` +type(scope): short description of the change ``` -**Use **`git commit -s`** to sign off on all of your commits.** This will add `signed-off-by:` with your name and email as the last line. +Where **type** is the category of change (`feat`, `fix`, `docs`, `refactor`, `perf`, `test`, `build`, `ci`, `style`, `chore`, `revert`) and **scope** is the module or area affected (e.g. `ekf2`, `mavlink`, `navigator`). See the full [types and scopes tables](https://github.com/PX4/PX4-Autopilot/blob/main/CONTRIBUTING.md#commit-message-convention) in CONTRIBUTING.md. -This commit guide is based on best practices for the Linux Kernel and other [projects maintained](https://github.com/torvalds/subsurface-for-dirk/blob/a48494d2fbed58c751e9b7e8fbff88582f9b2d02/README#L88-L115) by Linus Torvalds. +Append `!` before the colon to mark a breaking change: `feat(ekf2)!: remove deprecated API`. + +### Examples + +``` +feat(ekf2): add height fusion timeout. Fixes #1234 + +The previous implementation did not handle the case where +height fusion data stops arriving mid-flight. This adds a +configurable timeout that falls back to barometric height. + +Tested in SITL with simulated sensor dropout. + +Signed-off-by: Your Name +``` + +The body of the message can contain several paragraphs. Describe in detail what you changed and why. Link related issues and flight logs. Describe the change and why you made it, rather than paraphrasing the code change. + +**Use `git commit -s` to sign off on all of your commits.** This adds `Signed-off-by:` with your name and email as the last line. ## Pull Requests diff --git a/docs/en/contribute/git_examples.md b/docs/en/contribute/git_examples.md index bedd5f07b5..c0140b5280 100644 --- a/docs/en/contribute/git_examples.md +++ b/docs/en/contribute/git_examples.md @@ -45,15 +45,15 @@ Adding a feature to PX4 follows a defined workflow. In order to share your contr git add ``` - If you prefer having a GUI to add your files see [Gitk](https://git-scm.com/book/en/v2/Appendix-A:-Git-in-Other-Environments-Graphical-Interfaces) or [`git add -p`](https://nuclearsquid.com/writings/git-add/). + If you prefer having a GUI to add your files see [Gitk](https://git-scm.com/book/en/v2/Git-in-Other-Environments-Graphical-Interfaces) or [`git add -p`](https://nuclearsquid.com/writings/git-add/). - Commit the added files with a meaningful message explaining your changes ```sh - git commit -m "" + git commit -s -m "feat(ekf2): add height fusion timeout" ``` - For a good commit message, please refer to the [Source Code Management](../contribute/code.md#commits-and-commit-messages) section. + Use [conventional commits](https://www.conventionalcommits.org/) format: `type(scope): description`. For details on types and scopes, see the [Source Code Management](../contribute/code.md#commits-and-commit-messages) section. - Some time might have passed and the [upstream main](https://github.com/PX4/PX4-Autopilot) has changed. PX4 prefers a linear commit history and uses [git rebase](https://git-scm.com/book/en/v2/Git-Branching-Rebasing).