From 1c399cf3b5d0a304635b0f68df12404190acb1b7 Mon Sep 17 00:00:00 2001 From: zhangning21 Date: Tue, 21 Jul 2026 11:51:23 +0800 Subject: [PATCH] ci: Support pull request dependencies via Depends-On. Allow pull requests targeting master to declare same- and cross-repository dependencies. Parse declarations with a tested Python helper, apply exact dependency commits before the existing build matrix, and rerun heavy CI only when an edited description changes the dependency state. Keep fork builds read-only and use a trusted workflow_run to validate artifacts and post per-build dependency results. Document the supported declaration forms and operational limits. Assisted-by: Kiro:gpt-5.6-sol Signed-off-by: zhangning21 --- .github/scripts/depends_on.py | 197 +++++++++++++++ .github/scripts/test_depends_on.py | 293 +++++++++++++++++++++++ .github/workflows/build.yml | 216 ++++++++++++++++- .github/workflows/depends-on-comment.yml | 208 ++++++++++++++++ Documentation/testing/nuttx-ci.rst | 74 ++++++ 5 files changed, 987 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/depends_on.py create mode 100644 .github/scripts/test_depends_on.py create mode 100644 .github/workflows/depends-on-comment.yml diff --git a/.github/scripts/depends_on.py b/.github/scripts/depends_on.py new file mode 100644 index 0000000000000..14a756634f51b --- /dev/null +++ b/.github/scripts/depends_on.py @@ -0,0 +1,197 @@ +#!/usr/bin/env python3 +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +"""Parse ``depends-on:`` declarations from a NuttX pull request body. + +Supported forms are a single reference, an inline list, or multiple declaration +lines. References may use ``owner/repo/pull/N`` or a full GitHub URL. Bullet-list +continuations are not supported. + +Declarations must start a line with at most three spaces and must be outside +Markdown code blocks. Repositories are restricted to ``NUTTX_REPO`` and +``APPS_REPO``; PR numbers are positive JavaScript-safe integers. + +CLI: + python3 depends_on.py # print result JSON + python3 depends_on.py --print-state # print state used by the edit gate + python3 depends_on.py --github-output # write workflow outputs and report +""" + +from __future__ import annotations + +import json +import os +import re +import sys + +# Limit PR numbers to JavaScript's exact integer range because the comment +# workflow reads the report. The 19-digit regex limit also avoids huge int() +# conversions; it is not a GitHub business limit. +_MAX_SAFE_PR_NUMBER = 9007199254740991 # 2**53 - 1 +_TOKEN_RE = re.compile( + r"^(?:https://github\.com/)?" + r"(?P[A-Za-z0-9._-]+/[A-Za-z0-9._-]+)/pull/(?P[1-9][0-9]{0,18})$" +) + +# CommonMark treats four leading spaces or a tab as indented code. Anchoring +# also excludes prose and prefixes such as "not-depends-on:". +_MARKER_RE = re.compile(r"^ {0,3}depends-on:[ \t]*(?P.*)$", re.IGNORECASE) + +# CommonMark fences may be indented by at most three spaces. +_FENCE_RE = re.compile(r"^ {0,3}(?:```|~~~)") + + +def allowed_repos_from_env(): + return ( + os.environ.get("NUTTX_REPO", "apache/nuttx"), + os.environ.get("APPS_REPO", "apache/nuttx-apps"), + ) + + +def _split_tokens(text): + # GitHub Markdown does not treat Unicode separators as declaration + # boundaries, so split tokens on ASCII separators only. + return [t for t in re.split(r"[ \t\r\n\[\],]+", text.strip()) if t] + + +def _lines(body): + """Split on newline forms rendered by GitHub Markdown.""" + return (body or "").replace("\r\n", "\n").replace("\r", "\n").split("\n") + + +def has_declaration(body): + """True if the body has a line-anchored depends-on: outside code fences.""" + in_fence = False + for ln in _lines(body): + if _FENCE_RE.match(ln): + in_fence = not in_fence + continue + if not in_fence and _MARKER_RE.match(ln): + return True + return False + + +def _declared_tokens(body): + """Yield tokens from single-line declarations outside code fences.""" + in_fence = False + for ln in _lines(body): + if _FENCE_RE.match(ln): + in_fence = not in_fence + continue + if in_fence: + continue + m = _MARKER_RE.match(ln) + if m: + for t in _split_tokens(m.group("rest")): + yield t + + +def parse_dependencies(body, allowed_repos=None, pr_number=None, head_sha=None): + """Return the structured dependency result for ``body``.""" + if allowed_repos is None: + allowed_repos = allowed_repos_from_env() + + deps = [] + warnings = [] + seen = set() + for t in _declared_tokens(body): + m = _TOKEN_RE.match(t) + if not m: + continue # stray text on a declaration line; ignore + repo = m.group("repo") + num = int(m.group("num")) + if num > _MAX_SAFE_PR_NUMBER: + continue # beyond JS safe integer; comment workflow can't handle it + if repo not in allowed_repos: + warnings.append("Ignoring unsupported dependency repo: " + repo) + continue + key = (repo, num) + if key not in seen: + seen.add(key) + deps.append({"repo": repo, "number": num}) + + has_decl = not deps and has_declaration(body) + if has_decl: + warnings.append( + "Found a 'depends-on:' line but no valid dependency was parsed. " + "Declare dependencies on the same line as 'depends-on:', e.g. " + "depends-on: [{}/pull/ {}/pull/]".format(*allowed_repos) + ) + + return { + "version": 1, + "pr_number": pr_number, + "head_sha": head_sha, + "status": "ok" if deps else "invalid" if has_decl else "none", + "dependencies": deps, + "warnings": warnings, + } + + +def dep_ref(dep): + return "{}/pull/{}".format(dep["repo"], dep["number"]) + + +def _int_or_none(value): + try: + return int(value) + except (TypeError, ValueError): + return None + + +def main(argv): + body = os.environ.get("PR_BODY", "") + result = parse_dependencies( + body, + pr_number=_int_or_none(os.environ.get("PR_NUMBER")), + head_sha=os.environ.get("HEAD_SHA") or None, + ) + refs = [dep_ref(d) for d in result["dependencies"]] + + if "--print-state" in argv: + print(result["status"]) + for r in refs: + print(r) + return 0 + + if "--github-output" in argv: + for w in result["warnings"]: + print("::warning::" + w) + out_lines = [ + "depends_on=" + " ".join(refs), + "status=" + result["status"], + ] + gh_out = os.environ.get("GITHUB_OUTPUT") + if gh_out: + with open(gh_out, "a", encoding="utf-8") as f: + f.write("\n".join(out_lines) + "\n") + else: + print("\n".join(out_lines)) + # Write the report only when a depends-on declaration is present + # (status ok/invalid). status=none means there is nothing to report and + # nothing to comment; existing (historical) comments are left untouched. + report_path = os.environ.get("REPORT_PATH") + if report_path and result["status"] != "none": + parent = os.path.dirname(report_path) + if parent: + os.makedirs(parent, exist_ok=True) + with open(report_path, "w", encoding="utf-8") as f: + json.dump(result, f) + return 0 + + print(json.dumps(result)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) diff --git a/.github/scripts/test_depends_on.py b/.github/scripts/test_depends_on.py new file mode 100644 index 0000000000000..c33bb7cb33250 --- /dev/null +++ b/.github/scripts/test_depends_on.py @@ -0,0 +1,293 @@ +#!/usr/bin/env python3 +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +"""Unit tests for depends_on. Run: python3 -m unittest test_depends_on -v""" + +import io +import os +import unittest +from contextlib import redirect_stdout +from unittest import mock + +from depends_on import has_declaration, main, parse_dependencies + +OS_REPO = "apache/nuttx" +APPS_REPO = "apache/nuttx-apps" +ALLOWED = (OS_REPO, APPS_REPO) + + +def dep(repo, number): + return {"repo": repo, "number": number} + + +def parsed(body, allowed_repos): + result = parse_dependencies(body, allowed_repos) + return result["dependencies"], result["warnings"] + + +class ParseBasicsTest(unittest.TestCase): + def test_none(self): + self.assertEqual(parsed("normal body\nno deps", ALLOWED), ([], [])) + + def test_empty(self): + self.assertEqual(parsed("", ALLOWED), ([], [])) + self.assertEqual(parsed(None, ALLOWED), ([], [])) + + def test_single(self): + d, w = parsed("depends-on: %s/pull/1234" % APPS_REPO, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1234)]) + self.assertEqual(w, []) + + def test_full_url(self): + d, _ = parsed("depends-on: https://github.com/%s/pull/1" % APPS_REPO, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1)]) + + def test_array_mixed(self): + body = "depends-on: [%s/pull/1 https://github.com/%s/pull/2]" % ( + APPS_REPO, + OS_REPO, + ) + d, _ = parsed(body, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1), dep(OS_REPO, 2)]) + + def test_dedup(self): + body = "depends-on: [%s/pull/1 %s/pull/1]" % (APPS_REPO, APPS_REPO) + d, _ = parsed(body, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1)]) + + def test_case_insensitive(self): + d, _ = parsed("Depends-On: %s/pull/7" % APPS_REPO, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 7)]) + + def test_non_allowlisted(self): + d, w = parsed("depends-on: someone/other/pull/1", ALLOWED) + self.assertEqual(d, []) + self.assertTrue(any("unsupported" in x.lower() for x in w)) + + def test_typo_push(self): + d, w = parsed("depends-on: %s/push/1" % APPS_REPO, ALLOWED) + self.assertEqual(d, []) + self.assertTrue(w) + + def test_non_numeric(self): + d, w = parsed("depends-on: %s/pull/abc" % APPS_REPO, ALLOWED) + self.assertEqual(d, []) + self.assertTrue(w) + + +class NumberBoundsTest(unittest.TestCase): + def test_zero_rejected(self): + self.assertEqual(parsed("depends-on: %s/pull/0" % APPS_REPO, ALLOWED)[0], []) + + def test_leading_zero_rejected(self): + self.assertEqual(parsed("depends-on: %s/pull/007" % APPS_REPO, ALLOWED)[0], []) + + def test_16_digit_within_safe_ok(self): + # 16-digit but <= MAX_SAFE_INTEGER is valid (no business cap). + d, _ = parsed("depends-on: %s/pull/1234567890123456" % APPS_REPO, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1234567890123456)]) + + def test_max_safe_integer_ok(self): + d, _ = parsed("depends-on: %s/pull/9007199254740991" % APPS_REPO, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 9007199254740991)]) + + def test_above_safe_integer_rejected(self): + # MAX_SAFE_INTEGER + 1 + self.assertEqual( + parsed("depends-on: %s/pull/9007199254740992" % APPS_REPO, ALLOWED)[0], + [], + ) + + def test_twenty_digits_rejected(self): + self.assertEqual( + parsed("depends-on: %s/pull/99999999999999999999" % APPS_REPO, ALLOWED)[0], + [], + ) + + def test_huge_number_does_not_crash(self): + # thousands of digits must be rejected without raising (no int() blowup) + body = "depends-on: %s/pull/%s" % (APPS_REPO, "9" * 5000) + d, _ = parsed(body, ALLOWED) + self.assertEqual(d, []) + + +class UnicodeLineTest(unittest.TestCase): + def test_u2028_not_a_new_line(self): + # U+2028 must not be treated as a line break that starts a new marker. + body = "intro text\u2028depends-on: %s/pull/1" % APPS_REPO + self.assertFalse(has_declaration(body)) + self.assertEqual(parsed(body, ALLOWED)[0], []) + + def test_crlf_and_cr_are_line_breaks(self): + d1, _ = parsed("x\r\ndepends-on: %s/pull/1" % APPS_REPO, ALLOWED) + self.assertEqual(d1, [dep(APPS_REPO, 1)]) + d2, _ = parsed("x\rdepends-on: %s/pull/2" % APPS_REPO, ALLOWED) + self.assertEqual(d2, [dep(APPS_REPO, 2)]) + + def test_u2028_within_declaration_line_not_split(self): + # On a real depends-on line, a U+2028-joined pair is not split into two + # tokens (ASCII-only separators); it becomes one invalid token. + body = "depends-on: %s/pull/1\u2028%s/pull/2" % (APPS_REPO, APPS_REPO) + self.assertEqual(parsed(body, ALLOWED)[0], []) + + +class LineAnchorTest(unittest.TestCase): + def test_midline_prose_ignored(self): + body = "Please see depends-on: %s/pull/13 for the example." % APPS_REPO + self.assertFalse(has_declaration(body)) + self.assertEqual(parsed(body, ALLOWED), ([], [])) + + def test_not_depends_on_ignored(self): + body = "not-depends-on: %s/pull/1" % APPS_REPO + self.assertFalse(has_declaration(body)) + self.assertEqual(parsed(body, ALLOWED), ([], [])) + + def test_code_fence_ignored_backtick(self): + body = "```\ndepends-on: %s/pull/1\n```" % APPS_REPO + self.assertFalse(has_declaration(body)) + self.assertEqual(parsed(body, ALLOWED), ([], [])) + + def test_code_fence_ignored_tilde(self): + body = "~~~\ndepends-on: %s/pull/1\n~~~" % APPS_REPO + self.assertEqual(parsed(body, ALLOWED), ([], [])) + + def test_leading_whitespace_ok(self): + d, _ = parsed(" depends-on: %s/pull/5" % APPS_REPO, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 5)]) + + def test_four_space_indent_is_code_ignored(self): + # 4+ leading spaces = CommonMark indented code block -> ignored. + body = " depends-on: %s/pull/1" % APPS_REPO + self.assertFalse(has_declaration(body)) + self.assertEqual(parsed(body, ALLOWED), ([], [])) + + def test_tab_indent_is_code_ignored(self): + body = "\tdepends-on: %s/pull/1" % APPS_REPO + self.assertFalse(has_declaration(body)) + self.assertEqual(parsed(body, ALLOWED), ([], [])) + + def test_indented_fence_is_not_a_fence(self): + # A ``` indented by 4+ spaces is not a fence opener (CommonMark), so the + # following column-0 depends-on: must still be parsed. + body = " ```\ndepends-on: %s/pull/1\n ```" % APPS_REPO + d, _ = parsed(body, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1)]) + + def test_real_after_prose_still_parsed(self): + body = "See docs.\ndepends-on: %s/pull/9\nthanks" % APPS_REPO + d, _ = parsed(body, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 9)]) + + +class HostBoundaryTest(unittest.TestCase): + def test_gitlab_rejected(self): + d, w = parsed("depends-on: gitlab.com/%s/pull/7" % APPS_REPO, ALLOWED) + self.assertEqual(d, []) + self.assertTrue(w) # declaration present but nothing valid + + def test_bare_github_com_without_https_rejected(self): + d, _ = parsed("depends-on: github.com/%s/pull/7" % APPS_REPO, ALLOWED) + self.assertEqual(d, []) + + def test_http_github_rejected(self): + # only https://github.com/ is accepted as URL form + d, _ = parsed("depends-on: http://github.com/%s/pull/7" % APPS_REPO, ALLOWED) + self.assertEqual(d, []) + + +class MultiDependencyTest(unittest.TestCase): + def test_multiple_depends_on_lines(self): + # Multiple dependencies via several depends-on: lines (no continuation). + body = ( + "depends-on: %s/pull/1\n" + "depends-on: https://github.com/%s/pull/2\n" % (APPS_REPO, OS_REPO) + ) + d, w = parsed(body, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1), dep(OS_REPO, 2)]) + self.assertEqual(w, []) + + def test_inline_array(self): + body = "depends-on: [%s/pull/1 %s/pull/2]" % (APPS_REPO, OS_REPO) + d, _ = parsed(body, ALLOWED) + self.assertEqual(d, [dep(APPS_REPO, 1), dep(OS_REPO, 2)]) + + def test_bullet_list_not_parsed(self): + # A bullet list under a bare marker is not a supported form: the marker + # line has no ref and the '- ...' lines are not declarations. + body = "depends-on:\n" "- %s/pull/1\n" "- %s/pull/2\n" % (APPS_REPO, APPS_REPO) + d, w = parsed(body, ALLOWED) + self.assertEqual(d, []) # nothing parsed + self.assertTrue(w) # declaration present but invalid + + +class ParseResultStatusTest(unittest.TestCase): + def test_none(self): + self.assertEqual( + parse_dependencies("hi", allowed_repos=ALLOWED)["status"], "none" + ) + + def test_ok(self): + r = parse_dependencies( + "depends-on: %s/pull/9" % APPS_REPO, + pr_number=42, + head_sha="abc", + allowed_repos=ALLOWED, + ) + self.assertEqual(r["status"], "ok") + self.assertEqual(r["dependencies"], [dep(APPS_REPO, 9)]) + self.assertEqual((r["pr_number"], r["head_sha"], r["version"]), (42, "abc", 1)) + + def test_invalid(self): + self.assertEqual( + parse_dependencies( + "depends-on: %s/push/1" % APPS_REPO, allowed_repos=ALLOWED + )["status"], + "invalid", + ) + + def test_code_fence_is_none(self): + self.assertEqual( + parse_dependencies( + "```\ndepends-on: %s/pull/1\n```" % APPS_REPO, allowed_repos=ALLOWED + )["status"], + "none", + ) + + +class PrintStateTest(unittest.TestCase): + def test_dependency_order_is_part_of_state(self): + bodies = ( + "depends-on: [apache/nuttx/pull/1 apache/nuttx-apps/pull/2]", + "depends-on: [apache/nuttx-apps/pull/2 apache/nuttx/pull/1]", + ) + outputs = [] + for body in bodies: + output = io.StringIO() + with mock.patch.dict(os.environ, {"PR_BODY": body}, clear=True): + with redirect_stdout(output): + self.assertEqual(main(["--print-state"]), 0) + outputs.append(output.getvalue().splitlines()) + + self.assertEqual( + outputs[0], + ["ok", "apache/nuttx/pull/1", "apache/nuttx-apps/pull/2"], + ) + self.assertEqual( + outputs[1], + ["ok", "apache/nuttx-apps/pull/2", "apache/nuttx/pull/1"], + ) + self.assertNotEqual(outputs[0], outputs[1]) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a00cd8c193191..1771a88a13e52 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -14,6 +14,7 @@ name: Build on: pull_request: + types: [opened, synchronize, reopened, edited] paths-ignore: - "AUTHORS" - "CONTRIBUTING.md" @@ -35,16 +36,104 @@ permissions: concurrency: group: build-${{ github.event.pull_request.number || github.ref }} - cancel-in-progress: true + # Edited runs do not request cancellation of an active code build. + # GitHub may still replace an older pending run in this concurrency group. + cancel-in-progress: ${{ github.event.action != 'edited' }} jobs: + # Gate heavy CI on dependency-changing edits. + Changes: + runs-on: ubuntu-latest + outputs: + should_build: ${{ steps.gate.outputs.should_build }} + steps: + # Do not let PR code control its own edit gate. + - name: Checkout base-branch CI scripts + if: ${{ github.event_name == 'pull_request' && github.event.action == 'edited' }} + uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.base.sha }} + sparse-checkout: .github/scripts + sparse-checkout-cone-mode: false + fetch-depth: 1 + path: base-ci + continue-on-error: true + - name: Checkout PR CI scripts (fallback) + if: ${{ github.event_name == 'pull_request' && github.event.action == 'edited' }} + uses: actions/checkout@v7 + with: + sparse-checkout: .github/scripts + sparse-checkout-cone-mode: false + fetch-depth: 1 + path: pr-ci + - name: Decide whether to run CI + id: gate + shell: bash + env: + ACTION: ${{ github.event.action }} + NEW_BODY: ${{ github.event.pull_request.body }} + OLD_BODY: ${{ github.event.changes.body.from }} + BODY_CHANGE: ${{ toJSON(github.event.changes.body) }} + BASE_CHANGE: ${{ toJSON(github.event.changes.base) }} + run: | + set -euo pipefail + + if [ "${ACTION:-}" != "edited" ]; then + echo "Event '${ACTION:-push}': running CI." + echo "should_build=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + if [ "$BASE_CHANGE" != "null" ]; then + echo "::notice::PR base branch changed; running CI." + echo "should_build=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + if [ "$BODY_CHANGE" = "null" ]; then + echo "::notice::PR edited but body unchanged; no code/dependency change, skipping CI." + echo "should_build=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + PARSER="pr-ci/.github/scripts/depends_on.py" + if [ -f "base-ci/.github/scripts/depends_on.py" ]; then + PARSER="base-ci/.github/scripts/depends_on.py" + echo "Using base-branch parser for the gate." + else + echo "::notice::Base branch has no depends_on.py yet; using PR parser for the gate (bootstrap)." + fi + + # Include status so invalid declarations also retrigger reporting. + NEW_STATE="$(PR_BODY="$NEW_BODY" python3 "$PARSER" --print-state)" + OLD_STATE="$(PR_BODY="$OLD_BODY" python3 "$PARSER" --print-state)" + if [ "$NEW_STATE" != "$OLD_STATE" ]; then + echo "depends-on state changed; running CI." + echo "should_build=true" >> "$GITHUB_OUTPUT" + else + echo "::notice::No depends-on change on this edit; no code change, skipping CI." + echo "should_build=false" >> "$GITHUB_OUTPUT" + fi + # Fetch the source from nuttx and nuttx-apps repos Fetch-Source: + needs: Changes + if: ${{ needs.Changes.outputs.should_build == 'true' }} runs-on: ubuntu-latest steps: + - name: Checkout CI scripts + uses: actions/checkout@v7 + with: + sparse-checkout: .github/scripts + sparse-checkout-cone-mode: false + fetch-depth: 1 - name: Determine Target Branches id: gittargets shell: bash + env: + PR_BODY: ${{ github.event.pull_request.body }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + REPORT_PATH: depends-on-report/result.json run: | OS_REF="" APPS_REF="" @@ -91,6 +180,11 @@ jobs: esac fi + # Release and backport PRs ignore dependencies. + if [ -n "$PR_BODY" ] && [ "$GITHUB_BASE_REF" = "master" ]; then + python3 .github/scripts/depends_on.py --github-output + fi + echo "os_ref=$OS_REF" >> $GITHUB_OUTPUT echo "apps_ref=$APPS_REF" >> $GITHUB_OUTPUT @@ -112,6 +206,126 @@ jobs: path: sources/apps fetch-depth: 1 + - name: Apply depends-on PRs + if: ${{ steps.gittargets.outputs.depends_on != '' }} + shell: bash + run: | + set -uo pipefail + + git config --global user.email "actions@github.com" + git config --global user.name "github-actions" + + # Pass only fixed error codes to the trusted comment workflow. + mark_failed() { + echo "::error::could not apply $1 ($2)" + python3 - "$2" <<'PY' + import json, sys + p = "depends-on-report/result.json" + try: + d = json.load(open(p)) + except Exception: + d = {"version": 1, "pr_number": None, "head_sha": None, "dependencies": [], "warnings": []} + d["status"] = "failed" + d["error_code"] = sys.argv[1] + open(p, "w").write(json.dumps(d)) + PY + } + + # Parse before the loop so process substitution cannot hide errors. + if ! python3 - > depends-on-report/deps.tsv <<'PY' + import json + with open("depends-on-report/result.json", encoding="utf-8") as f: + d = json.load(f) + for x in d["dependencies"]: + print("%s\t%d" % (x["repo"], x["number"])) + PY + then + echo "::error::could not read the dependency report" + mark_failed "depends-on" "report_parse_failed"; exit 1 + fi + + : > depends-on-report/applied.tsv + + while IFS=$'\t' read -r DEP_REPO DEP_PR_NUM; do + [ -n "$DEP_REPO" ] || continue + DEP="${DEP_REPO}/pull/${DEP_PR_NUM}" + + case "$DEP_REPO" in + "apache/nuttx") REPO_PATH="sources/nuttx" ;; + "apache/nuttx-apps") REPO_PATH="sources/apps" ;; + *) + echo "::error::Unsupported dependency repo: $DEP_REPO" + mark_failed "$DEP" "unsupported_repo"; exit 1 ;; + esac + + echo "Applying dependency ${DEP}" + if [ -f "$REPO_PATH/.git/shallow" ]; then + git -C "$REPO_PATH" fetch --unshallow origin || true + fi + if ! git -C "$REPO_PATH" fetch origin "pull/${DEP_PR_NUM}/head:dep-${DEP_PR_NUM}"; then + echo "::error::Could not fetch ${DEP} (the PR may not exist)." + mark_failed "$DEP" "fetch_failed"; exit 1 + fi + + DEP_SHA=$(git -C "$REPO_PATH" rev-parse "dep-${DEP_PR_NUM}") + printf '%s\t%s\t%s\n' "$DEP_REPO" "$DEP_PR_NUM" "$DEP_SHA" >> depends-on-report/applied.tsv + + # Stop on unrelated histories; HEAD..dep would otherwise include + # every dependency commit and could cherry-pick unrelated changes. + COMMON_BASE=$(git -C "$REPO_PATH" merge-base "dep-${DEP_PR_NUM}" HEAD || true) + if [ -z "$COMMON_BASE" ]; then + echo "::error::Could not find common base for ${DEP}" + mark_failed "$DEP" "no_common_base"; exit 1 + fi + + COMMITS=$(git -C "$REPO_PATH" rev-list --reverse "HEAD..dep-${DEP_PR_NUM}") || { + echo "::error::Could not list commits for ${DEP}" + mark_failed "$DEP" "rev_list_failed"; exit 1 + } + + if [ -z "$COMMITS" ]; then + echo "Dependency ${DEP} is already included" + continue + fi + + # shellcheck disable=SC2086 + if ! git -C "$REPO_PATH" cherry-pick $COMMITS; then + echo "::error::cherry-pick failed for ${DEP}." + echo "::error::If your PR contains merge commits, please rebase instead of merge." + git -C "$REPO_PATH" cherry-pick --abort || true + mark_failed "$DEP" "cherry_pick_conflict"; exit 1 + fi + done < depends-on-report/deps.tsv + + python3 - <<'PY' + import json + shas = {} + try: + with open("depends-on-report/applied.tsv", encoding="utf-8") as f: + for line in f: + p = line.rstrip("\n").split("\t") + if len(p) == 3: + shas[(p[0], p[1])] = p[2] + except FileNotFoundError: + pass + with open("depends-on-report/result.json", encoding="utf-8") as f: + d = json.load(f) + for dep in d.get("dependencies", []): + key = (dep.get("repo"), str(dep.get("number"))) + if key in shas: + dep["head_sha"] = shas[key] + with open("depends-on-report/result.json", "w", encoding="utf-8") as f: + json.dump(d, f) + PY + + # Apply failures rewrite the report file; the step output remains "ok". + - name: Upload depends-on report + if: ${{ always() && (steps.gittargets.outputs.status == 'ok' || steps.gittargets.outputs.status == 'invalid') }} + uses: actions/upload-artifact@v7.0.1 + with: + name: depends-on-report + path: depends-on-report/ + - name: Tar sources run: tar zcf sources.tar.gz sources diff --git a/.github/workflows/depends-on-comment.yml b/.github/workflows/depends-on-comment.yml new file mode 100644 index 0000000000000..a45d4b23056cf --- /dev/null +++ b/.github/workflows/depends-on-comment.yml @@ -0,0 +1,208 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Posts validated dependency reports without checking out PR code. +# workflow_run isolates write permission and uses the default-branch workflow. + +name: Depends-On Comment + +on: + workflow_run: + workflows: ["Build"] + types: [completed] + +permissions: + actions: read # download an artifact from the triggering run + pull-requests: write + +# Keep this artifact allow-list in sync with build.yml. +env: + NUTTX_REPO: apache/nuttx + APPS_REPO: apache/nuttx-apps + +jobs: + comment: + if: ${{ github.event.workflow_run.event == 'pull_request' }} + runs-on: ubuntu-latest + steps: + - name: Download depends-on report + id: dl + uses: actions/download-artifact@v8 + with: + name: depends-on-report + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + path: report + continue-on-error: true + + - name: Comment on the PR + if: ${{ steps.dl.outcome == 'success' }} + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const owner = context.repo.owner; + const repo = context.repo.repo; + const allow = [process.env.NUTTX_REPO, process.env.APPS_REPO].filter(Boolean); + + // Validate the untrusted Build artifact's structure and safe + // rendering fields without executing fork code. + let report; + try { + report = JSON.parse(fs.readFileSync('report/result.json', 'utf8')); + } catch (e) { + core.info('No valid depends-on report; nothing to comment.'); + return; + } + + if (report.version !== 1) { + core.info('Unexpected report version; skipping.'); + return; + } + const status = report.status; + if (status !== 'ok' && status !== 'invalid' && status !== 'failed') { + core.info(`Report status='${status}'; nothing to comment.`); + return; + } + + // Match the Python parser's numeric and repository limits. Reject + // the whole report instead of silently dropping unsafe entries. + const isSafeNumber = (n) => Number.isSafeInteger(n) && n > 0; + const rawDeps = Array.isArray(report.dependencies) ? report.dependencies : null; + if (rawDeps === null) { + core.info('Report dependencies are not an array; skipping.'); + return; + } + const deps = rawDeps.filter((d) => d && typeof d.repo === 'string' + && allow.includes(d.repo) && isSafeNumber(d.number)); + if (deps.length !== rawDeps.length) { + core.info('Report contains an invalid dependency entry; skipping.'); + return; + } + const keys = deps.map((d) => `${d.repo}#${d.number}`); + if (new Set(keys).size !== keys.length) { + core.info('Report contains duplicate dependencies; skipping.'); + return; + } + if (status === 'invalid' && deps.length !== 0) { + core.info('status=invalid but dependencies are present; skipping.'); + return; + } + const isFullSha = (s) => typeof s === 'string' && /^[0-9a-f]{40}$/i.test(s); + if (status === 'ok' && !deps.every((d) => isFullSha(d.head_sha))) { + core.info('status=ok but a full dependency head SHA is missing; skipping.'); + return; + } + + // Bind the report to both the triggering run and the PR's current + // head. Stale, cancelled, or mismatched reports must not comment. + const headSha = context.payload.workflow_run.head_sha; + if (report.head_sha !== headSha) { + core.info('Report head_sha does not match workflow_run head_sha; skipping.'); + return; + } + const claimed = Number(report.pr_number); + + let prNum = null; + if (Number.isSafeInteger(claimed) && claimed > 0) { + try { + const { data: pr } = await github.rest.pulls.get({ + owner, repo, pull_number: claimed }); + if (pr.head.sha === headSha) prNum = claimed; + } catch (e) { + prNum = null; + } + } + if (prNum === null) { + core.info( + "workflow_run head_sha does not match the claimed PR's current " + + 'head (stale/cancelled run, or artifact mismatch); skipping comment.'); + return; + } + + // Preserve one idempotent comment per Build run. + const runId = context.payload.workflow_run.id; + const marker = ``; + const runUrl = context.payload.workflow_run.html_url; + const shortSha = (s) => + (typeof s === 'string' && /^[0-9a-f]{7,40}$/i.test(s)) ? ` @ ${s.slice(0, 10)}` : ''; + + let body; + if (status === 'ok') { + if (deps.length === 0) { + core.info('status=ok but no valid dependencies after validation; skipping.'); + return; + } + const list = deps + .map((d) => `- https://github.com/${d.repo}/pull/${d.number}${shortSha(d.head_sha)}`) + .join('\n'); + body = + `${marker}\n` + + `### 🔗 Cross-repo PR dependencies\n\n` + + `The read-only Build run reported the following dependent ` + + `PR(s) and fetched head SHA(s):\n\n` + + `${list}\n\n` + + `CI run: ${runUrl}`; + } else if (status === 'failed') { + if (deps.length === 0) { + core.info('status=failed but no valid dependencies after validation; skipping.'); + return; + } + const list = deps + .map((d) => `- https://github.com/${d.repo}/pull/${d.number}${shortSha(d.head_sha)}`) + .join('\n'); + // Render only fixed text selected by an allowed error code. + const REASONS = { + fetch_failed: 'the dependency PR could not be fetched (it may not exist)', + no_common_base: 'no common base with the dependency PR', + rev_list_failed: 'could not determine the dependency commits', + cherry_pick_conflict: 'cherry-pick failed (if your PR has merge commits, rebase instead)', + unsupported_repo: 'the dependency repository is not supported', + report_parse_failed: 'the dependency report could not be read', + }; + const code = typeof report.error_code === 'string' ? report.error_code : ''; + const reason = REASONS[code] ? `\n\nReason: ${REASONS[code]}` : ''; + body = + `${marker}\n` + + `### ❌ Cross-repo dependency could not be applied\n\n` + + `The Build report says the declared dependency PR(s) could not ` + + `be applied, so CI did **not** run against the combined code:\n\n` + + `${list}${reason}\n\n` + + `CI run: ${runUrl}`; + } else { + const example = `depends-on: [${allow[0] || 'owner/repo'}/pull/ ` + + `${allow[1] || 'owner/repo'}/pull/]`; + body = + `${marker}\n` + + `### ⚠️ \`depends-on\` could not be parsed\n\n` + + `A \`depends-on:\` line was found in the PR description, but no ` + + `valid dependency was parsed. Supported repositories: ` + + `\`${allow.join('`, `')}\`; the PR id must be numeric.\n\n` + + `Expected format:\n\n\`\`\`\n${example}\n\`\`\`\n\n` + + `CI run: ${runUrl}`; + } + + // Update only this run's bot comment; otherwise create one. + const comments = await github.paginate( + github.rest.issues.listComments, + { owner, repo, issue_number: prNum, per_page: 100 }); + const existing = comments.find((c) => + c.user && c.user.type === 'Bot' && c.body && c.body.includes(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body }); + core.info(`Updated this run's comment on PR #${prNum}.`); + } else { + await github.rest.issues.createComment({ + owner, repo, issue_number: prNum, body }); + core.info(`Created a new comment on PR #${prNum}.`); + } diff --git a/Documentation/testing/nuttx-ci.rst b/Documentation/testing/nuttx-ci.rst index b238bd5704331..cbfcb76d25f5e 100644 --- a/Documentation/testing/nuttx-ci.rst +++ b/Documentation/testing/nuttx-ci.rst @@ -108,6 +108,80 @@ The name of this workflow is deceiving, as it does not only build configurations, but also normalizes them and runs :doc:`NTFC ` tests on architectures that support it. +Pull Request Dependencies +------------------------- + +NuttX and ``nuttx-apps`` are built together. A change that spans both +repositories may therefore need to test two pull requests together before +either one is merged. A pull request targeting ``master`` can declare same- or +cross-repository dependencies in its description. + +The recommended form is one dependency per line: + +.. code-block:: text + + Depends-On: https://github.com/apache/nuttx-apps/pull/1234 + Depends-On: https://github.com/apache/nuttx/pull/5678 + +Alternatively, multiple dependencies can use a single bracket list: + +.. code-block:: text + + Depends-On: [apache/nuttx-apps/pull/1234 https://github.com/apache/nuttx/pull/5678] + +The ``Depends-On:`` marker is case-insensitive; ``Depends-On:``, +``depends-on:``, and mixed-case spellings are equivalent. This documentation +uses ``Depends-On:`` as the canonical form. + +References may use either a complete ``https://github.com/...`` URL or the +short ``owner/repository/pull/number`` form, and the forms may be mixed. Each +accepted reference must identify a pull request in ``apache/nuttx`` or +``apache/nuttx-apps``. Other tokens are ignored; a declaration is invalid only +when a ``Depends-On:`` marker is present but no valid dependency can be parsed. +Multiple declarations are applied in their listed order, and duplicate +references are applied only once. Each declaration must remain on one line; +Markdown bullet-list continuations are not supported. Pull requests without a +``Depends-On:`` declaration retain the normal CI source selection. +Declarations in release and backport pull requests are ignored because +dependencies are applied only when the target branch is ``master``. + +The ``Fetch-Source`` job fetches each valid dependency's exact head SHA and +cherry-picks its commits into the corresponding checkout before the existing +build matrix runs. Invalid declarations are not applied: CI continues with the +normal source selection and posts a warning that the declaration could not be +parsed. If a valid dependency cannot be fetched, has no common history, its +commit list cannot be determined, or it causes a cherry-pick conflict, +``Fetch-Source`` fails instead of silently testing without the requested +dependency. + +When a valid dependency report is available, the follow-up comment reports one +of three outcomes: + +* successfully applied dependencies and their fetched head SHAs (abbreviated in + the comment) +* a declaration from which no valid dependency could be parsed +* valid dependencies that could not be applied and the failure reason + +The write-capable follow-up workflow does not execute fork code. It validates +the untrusted report's structure, repository allow-list, run and current PR +head binding, and fixed rendering fields before commenting. These checks make +posting the report safe, but do not independently attest that the dependency +was applied; the comment reflects the result produced by the read-only Build +workflow. + +Editing the pull request description triggers the CI dependency gate. The +resource-intensive build jobs run again when the base branch or ordered parsed +dependency state changes, so reordering dependencies also triggers a build. +Unrelated description edits run only the gate and do not request cancellation +of an already-running Build. GitHub may still replace an older pending run in +the same concurrency group. Updating a dependency pull request does not +automatically trigger the initiating pull request, so its CI must be rerun to +test the new dependency head. + +The combined result belongs to the initiating pull request. It does not set a +status on dependency pull requests, merge them automatically, or replace the +need to coordinate their merge order. + The steps executed in the build workflow are: 1. Fetch source: checks out ``nuttx`` and ``nuttx-apps`` repos. The source files