ci: support PR dependencies via depends-on #19075
+89
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,9 +45,12 @@ jobs: | |
| - name: Determine Target Branches | ||
| id: gittargets | ||
| shell: bash | ||
| env: | ||
| PR_BODY: ${{ github.event.pull_request.body }} | ||
| run: | | ||
| OS_REF="" | ||
| APPS_REF="" | ||
| DEPENDS_ON="" | ||
|
|
||
| REF=$GITHUB_REF | ||
|
|
||
|
|
@@ -91,8 +94,35 @@ jobs: | |
| esac | ||
| fi | ||
|
|
||
| # Parse cross-repo PR dependencies from PR body | ||
| # Format: depends-on: [apache/nuttx-apps/pull/1234 https://github.com/apache/nuttx/pull/5678] | ||
| if [ -n "$PR_BODY" ]; then | ||
| ARRAY_DEPS=$(echo "$PR_BODY" | grep -oE 'depends-on:[[:space:]]*\[[^]]+\]' | head -1) || true | ||
| if [ -n "$ARRAY_DEPS" ]; then | ||
| DEPS=$(echo "$ARRAY_DEPS" | grep -oE '(https://github.com/)?apache/nuttx(-apps)?/pull/[0-9]+') || true | ||
| else | ||
| DEPS=$(echo "$PR_BODY" | grep -oE 'depends-on:[[:space:]]*(https://github.com/)?apache/nuttx(-apps)?/pull/[0-9]+' | sed 's/depends-on:[[:space:]]*//' | head -1) || true | ||
| fi | ||
|
|
||
| for DEP in $DEPS; do | ||
| DEP=$(echo "$DEP" | sed 's|https://github.com/||') | ||
| DEP_REPO=$(echo "$DEP" | awk -F'/pull/' '{print $1}') | ||
| DEP_PR_NUM=$(echo "$DEP" | awk -F'/pull/' '{print $2}') | ||
|
|
||
| if [[ "$DEP_REPO" != "apache/nuttx" && "$DEP_REPO" != "apache/nuttx-apps" ]]; then | ||
| echo "::warning::Ignoring unsupported dependency repo: $DEP_REPO" | ||
| continue | ||
| fi | ||
|
|
||
| DEPENDS_ON="$DEPENDS_ON ${DEP_REPO}/pull/${DEP_PR_NUM}" | ||
| done | ||
|
|
||
| DEPENDS_ON=$(echo "$DEPENDS_ON" | tr ' ' '\n' | awk 'NF && !a[$0]++' | xargs) | ||
|
Comment on lines
+100
to
+120
Member
There was a problem hiding this comment. Hi NuttX Admins: This script will parse the Untrusted Input from the PR Body to extract the Dependency Info safely, which will prevent Injection Attacks inside the PR Body. I'm afraid the current NuttX CI Team doesn't have sufficient expertise to maintain this, we might introduce Injection Attacks in future. I strongly suggest that we engage a NuttX Team Member familiar with GitHub Actions Script Security, who will be able to maintain this script, to prevent Injection Attacks in future. We must comply with the Apache Guidelines for GitHub Actions Security: https://infra.apache.org/github-actions-policy.html
Contributor
There was a problem hiding this comment. I think this is a big concern. For now it might be a good idea to forgo this change. |
||
| fi | ||
|
|
||
| echo "os_ref=$OS_REF" >> $GITHUB_OUTPUT | ||
| echo "apps_ref=$APPS_REF" >> $GITHUB_OUTPUT | ||
| echo "depends_on=$DEPENDS_ON" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Checkout nuttx repo | ||
| uses: actions/checkout@v6 | ||
|
|
@@ -112,6 +142,65 @@ jobs: | |
| path: sources/apps | ||
| fetch-depth: 1 | ||
|
|
||
| - name: Apply depends-on PRs | ||
| if: ${{ steps.gittargets.outputs.depends_on != '' }} | ||
| shell: bash | ||
| env: | ||
| DEPENDS_ON: ${{ steps.gittargets.outputs.depends_on }} | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| git config --global user.email "actions@github.com" | ||
| git config --global user.name "github-actions" | ||
|
|
||
| for DEPENDENCY in $DEPENDS_ON; do | ||
| DEP_REPO=$(echo "$DEPENDENCY" | awk -F'/pull/' '{print $1}') | ||
| DEP_PR_NUM=$(echo "$DEPENDENCY" | awk -F'/pull/' '{print $2}') | ||
|
|
||
| case "$DEP_REPO" in | ||
| "apache/nuttx") | ||
| REPO_PATH="sources/nuttx" | ||
| ;; | ||
| "apache/nuttx-apps") | ||
| REPO_PATH="sources/apps" | ||
| ;; | ||
| *) | ||
| echo "::error::Unsupported dependency repo: $DEP_REPO" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
| echo "Applying dependency ${DEP_REPO}/pull/${DEP_PR_NUM}" | ||
| if [ -f "$REPO_PATH/.git/shallow" ]; then | ||
| git -C "$REPO_PATH" fetch --unshallow origin | ||
| fi | ||
| git -C "$REPO_PATH" fetch origin "pull/${DEP_PR_NUM}/head:dep-${DEP_PR_NUM}" | ||
|
|
||
| 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_REPO}/pull/${DEP_PR_NUM}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| COMMITS=$(git -C "$REPO_PATH" rev-list --reverse "HEAD..dep-${DEP_PR_NUM}") || { | ||
| echo "::error::Could not list commits for ${DEP_REPO}/pull/${DEP_PR_NUM}" | ||
| exit 1 | ||
| } | ||
|
|
||
| if [ -z "$COMMITS" ]; then | ||
| echo "Dependency ${DEP_REPO}/pull/${DEP_PR_NUM} is already included" | ||
| continue | ||
| fi | ||
|
|
||
| # shellcheck disable=SC2086 | ||
| if ! git -C "$REPO_PATH" cherry-pick $COMMITS; then | ||
| echo "::error::cherry-pick failed for ${DEP_REPO}/pull/${DEP_PR_NUM}." | ||
| echo "::error::If your PR contains merge commits, please rebase instead of merge." | ||
| git -C "$REPO_PATH" cherry-pick --abort || true | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| - name: Tar sources | ||
| run: tar zcf sources.tar.gz sources | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe? Do we need to escape the PR Body? Otherwise we could have an Injection Attack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising this. Yes, the PR body is untrusted input, so we need to be careful here.
The reason we pass the body through an env: variable instead of inlining ${{ github.event.pull_request.body }} directly into the run: script is precisely to avoid script injection. This is the mitigation GitHub documents in Security hardening for GitHub Actions
(https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable):
│
│ - With env:, the body is delivered to the step as the value of an environment variable. It is never substituted into the script source, so shell metacharacters in the body (`, $(
), ;, &&, …) are treated as literal data, not executed.
│ - The dangerous form would be inlining it directly, e.g. run: echo "${{ github.event.pull_request.body }}", where a body like "; rm -rf / # would be injected into the script text.
We deliberately do not do that.
│
│ On top of the env: indirection, the body is only ever consumed as quoted data by text-processing tools — echo "$PR_BODY" | grep -oE … — never eval'd or executed. The values we extract are further constrained:
│ - dependencies must match a fixed regex (?:https://github.com/)?apache/nuttx(\?:-apps)?/pull/[0-9]+;
│ - the repo is checked against a 2-entry allow-list (apache/nuttx, apache/nuttx-apps);
│ - the PR number is [0-9]+ only, so the later git fetch origin "pull/${DEP_PR_NUM}/head:…" can't be abused.
│
│ Finally, this workflow runs on pull_request (not pull_request_target), so the job has a read-only GITHUB_TOKEN and no secrets — the blast radius is minimal even in the worst case.
│
│ If helpful, I can switch the echo "$PR_BODY" calls to printf '%s\n' "$PR_BODY" for slightly more robust handling of arbitrary text (purely a robustness nicety, not a security fix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhangning21 This GitHub Actions Design is very unusual for NuttX CI. If I understand correctly:
depends-on: apache/nuttx/pull/88888888depends-on=nuttx/88888888depends-on: apache/nuttx/pull/88888888There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purpose
│ nuttx and nuttx-apps are built together in CI, and for a normal PR the Fetch-Source job always checks out the master of the other repo. The main problem this solves is cross-repo PR interdependency: when one feature must change both repos, each PR's CI fails because the other repo's master doesn't yet contain the matching change — today the only workaround is to force-merge one side with CI skipped, which risks breaking master. The same mechanism also covers the case where a PR depends on another PR in the same repo. The author declares this in the PR body, e.g. depends-on: [apache/nuttx-apps/pull/XXX], and CI builds the combined code. It's fully opt-in — without a depends-on line, CI behaves exactly as today.
Now to your specific questions:
│
│ 1. Yes, the author specifies the dependency in the PR body.
│
│ 2. "If the author edits the dependency in the body, is it rechecked?"
│ If the author edits only the PR body, it is not rechecked immediately. This follows the current workflow behavior: the existing
pull_requesttrigger does not run CI for PR description edits, only for normal CI-triggering events such as new commits. The dependency will be re-read on the next CI run.│
│ 3. "Isn't a PR Label better than parsing untrusted body text?"
│ Labels would be more controlled, but they are not very practical here because external contributors usually cannot apply labels to upstream PRs, and dependency values are dynamic PR numbers rather than fixed categories. Using the PR body lets the contributor declare the dependency directly, while the workflow still validates it with a strict allowlist and numeric PR ID.
│
│ 4. "Do other projects parse dependencies from the PR body, and are they OK with the untrusted input?"
│ Yes . A similar approach is used by Zuul CI for cross-project dependencies. Zuul supports a
Depends-On:directive, and for GitHub-based projects it is placed in the pull requestdescription: https://zuul-ci.org/docs/zuul/latest/gating.html#cross-project-dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if my Fellow Maintainers agree with me: But here's what I think about Breaking Changes that require both NuttX Repo and NuttX Apps Repo to be in sync...
Breaking Changes need to be carefully and manually managed. I expect the PR Author to test the changes in their own NuttX Repo and NuttX Apps Repo, and provide evidence that All NuttX Builds were successful. Then CI Team needs to standby and make sure that both NuttX Repo and NuttX Apps Repo are merged at the same time.
If we allow PR Authors to specify which version of NuttX / NuttX Apps to build: We might forget to do the manual checking and the simultaneous merging. And when NuttX / NuttX Apps repos go out of sync, we will have lots more problems :-(