ci: support PR dependencies via depends-on#19075
Conversation
This adds a depends-on declaration in the PR description, e.g.: depends-on: [apache/pull/100 apache/nuttx-apps/pull/200] Cross-repo PR dependencies, multiple dependent PRs, and dependencies on other PRs within the same repository can all be combined and supported. Once CI parses the dependencies, it fetches each dependent PR in the corresponding local repository and cherry-picks its commits, so the build runs against the combined code of all involved PRs. The current approach is intentionally conservative: it requires neither a GitHub App nor extra write permissions (which we do not have), making it safe. Its limitation is that it does not write status or comments back to the dependent PRs, i.e. a failing CI cannot mark the dependent PR as failed; when the PRs must be merged together, the PR owners need to coordinate the merge. Signed-off-by: zhangning21 <zhangning21@xiaomi.com>
zhangning21
requested review from
lupyuen,
raiden00pl and
simbit18
as code owners
github-actions
Bot
added
Area: CI
Size: S
|
|
I noticed that you're using an AI Agent for OpenVela. I'm curious if you used it for creating this PR, because this PR doesn't seem to follow the NuttX Contributing Guidelines. Thanks :-) |
| id: gittargets | ||
| shell: bash | ||
| env: | ||
| PR_BODY: ${{ github.event.pull_request.body }} |
There was a problem hiding this comment.
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.
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.
@zhangning21 This GitHub Actions Design is very unusual for NuttX CI. If I understand correctly:
- We expect the PR Author to specify inside the PR Body the dependency:
depends-on: apache/nuttx/pull/88888888 - What if the PR Author edits the dependency in the PR Body? Will the dependency be rechecked?
- I'm concerned about parsing the Untrusted Input from the PR Body. Isn't a PR Label a better way to provide the dependency? E.g.
depends-on=nuttx/88888888 - Are there any other projects using this? I wonder if they are also OK with parsing Untrusted Input from the PR Body.
- @simbit18 @linguini1 @cederom Do we think it's a good idea to parse the dependency from the PR Body?
depends-on: apache/nuttx/pull/88888888
There was a problem hiding this comment.
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_request trigger 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 request
description: https://zuul-ci.org/docs/zuul/latest/gating.html#cross-project-dependencies
There was a problem hiding this comment.
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.
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 :-(
|
Hi NuttX Admins: Please don't click "Approve Workflows To Run", I have concerns about the Safety of the GitHub Actions: |
@lupyuen The I'll update the PR to follow the guidelines and ping you again. Apologies for the rough first pass, and thanks for the careful review! |
| 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) |
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
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.
Could you show us a working version of this code in your Own NuttX Repo? Also we need the Test Logs for the various test cases thanks!
|
@lupyuen Here's a working version running in my own forks (
Note: cases 4 and 5 are silently ignored (no error, no warning) because the values don't match the dependency regex. If preferred, I can add a warning when a |
|
@lupyuen Thanks again — good point about release branches and backports. I've gone with the simplest and safest rule:
if [ -n "$PR_BODY" ] && [ "$GITHUB_BASE_REF" = "master" ]; then
# parse depends-on ...
fiThis avoids the backport problem without adding GitHub API calls or extra permissions. What this means:
One limitation: this checks only the current PR's target branch. Without an API lookup, the workflow does not verify the dependency PR's own base branch, so this feature is intended for the normal master-branch cross-repo dependency case. A stricter version could query each dependency PR's base.ref and require it to match, but that would reintroduce an API call and extra permission surface. For now, restricting depends-on to PRs targeting master seems the simpler and safer trade-off. I've validated this in my test fork with a backport-style PR targeting releases/12.13 whose body contains a copied depends-on: line. The Apply depends-on PRs step is skipped and CI builds against the matching releases/12.13 branch, so the copied dependency is ignored: Validation run: https://github.com/zhn-test/nuttx/actions/runs/27262401865/job/80511061759 Please see the Apply depends-on PRs step in the Fetch-Source job. It is skipped. Any downstream build differences in my fork are unrelated to this depends-on handling. If this approach looks OK, I'll update this PR first. After this PR is reviewed and the approach is agreed, I will submit the matching |
|
@zhangning21 Please hang on to the changes, I would like to hear from other maintainers about:
|
|
I'm not sure we should allow metadata within the PR body. This is a cool idea but maybe it needs to be put on hold for now. |
|
Yep I agree that embedding PR Metadata (e.g.
If we ever need to support PR Metadata in future, we would need a lot more work:
|
|
Hi @zhangning21: I'm sorry that we have to close this PR because:
I'm also sorry for your time wasted in preparing this meticulous PR. Perhaps in future, you could create a NuttX Issue first, then assign it to me for discussion, so we can agree on the best solution? I hope you understand that ASF Infrastructure Team is closely watching our usage of GitHub Actions. They nearly banned NuttX Project twice from using GitHub Actions, due to overuse and security concerns. Thanks :-) |
|
@lupyuen please don't close PR without voting:
Do you have other better method to fix the patch which has the cross-git dependence, which is a must have feature, but doesn't fix for a long time.
the dependence is described in the commit message, why change in commit message doesn't trigger a rebuild?
if the description is wrong, ci doesn't cherry-pick the related patch on apps side, then ci will fail loudly.
If awk/sed isn't good, let's stick to pure shell script or switch to python.
How to verify the cross-git patch correctly is a well-known issue for our ci build system, not a new issue. |
|
@xiaoxiang781216 I'm afraid I can't commit to these enhancements for NuttX CI. I'm already overwhelmed by the maintenance of NuttX CI, keeping it performant and secure, as mandated by ASF Infra Team. From now on: I'm stepping away from all NuttX CI Duties, and focusing instead on my Family Matters. @simbit18 @acassis @cederom @linguini1 @raiden00pl @hartmannathan @jerpelea Sorry I need to take a break from NuttX Project, it's hurting my hypertension. Hope to catch you again in future. Goodbye! |
|
@lupyuen best wishes! Hope to hear from you again soon :) |
Hi @lupyuen that is completely understandable. Take a break and focus on your health and your family! I'm sure you will continue to do great things! I wish you all the best! |
acassis
left a comment
acassis
left a comment
There was a problem hiding this comment.
@zhangning21 could you please add those information you added in the Summary into our CI Documentation: https://nuttx.apache.org/docs/latest/testing/nuttx-ci.html
linguini1
left a comment
linguini1
left a comment
There was a problem hiding this comment.
This seems like a great solution, I think it's really cool and it does solve a huge headache NuttX has had for a while. However, given the concerns from Lup about parsing the PR body, I think before this is merged we should check with Apache Infra about what they think about this solution.
5 participants
Summary
This PR adds support for cross-repo (and same-repo) PR dependencies to the NuttX CI, driven by a
depends-ondeclaration in the PR description.Why it is needed
nuttxandnuttx-appsare built together in CI. For a normal PR, theFetch-Sourcejob always checks out themasterof the other repo. When a single feature must change both repos, each PR's CI fails because the other repo'smasterdoes not yet contain the matching change. Today the only workaround is to force-merge one side with CI skipped, which risks breakingmaster.What is changed
Only
.github/workflows/build.ymlis modified, in two places inside the existingFetch-Sourcejob:Determine Target Branches— parse an optionaldepends-ondeclaration from the PR body and output it asdepends_on.Apply depends-on PRs— for each declared dependency, fetch the dependent PR'spull/<N>/headinto the corresponding local checkout andcherry-pickits commits, so the build runs against the combined code.How it is used (in the PR description):
depends-on: [apache/nuttx/pull/XXX apache/nuttx-apps/pull/YYY]
https://github.com/...URLs are both accepted and may be mixed; entries are de-duplicated.Impact
Apply depends-on PRsstep is guarded byif: steps.gittargets.outputs.depends_on != ''. PRs without adepends-ondeclaration (the vast majority) behave exactly as today — the step is skipped.grep/awk, no network, sub-second). For PRs that do declare a dependency, the extra work (git fetch --unshallow+git fetch pull/<N>/head+cherry-pick) runs once in the singleFetch-Sourcejob, never multiplied across the target matrix. So there is no measurable runner-budget increase for regular PRs.depends-onis declared. When declared and a dependency conflicts or contains a merge commit,Fetch-Sourcefails fast with a clear message (intended signal to rebase); this only affects the PR that opted in.env:variable (the GitHub-recommended mitigation against script injection) and used only as quoted data forgrep/awk/sed— nevereval'd. Parsed values are constrained by a fixed regex, a 2-entry repo allow-list, and a numeric PR number. The workflow runs onpull_request(read-only token, no secrets).Testing
Build Host: Ubuntu 22.04 (GitHub-hosted
ubuntu-latest)Targets: All targets currently built by NuttX CI (full
build.ymlmatrix).Full-target logs from this PR's own run will be linked once the workflow is
approved to run.
Feature validation (on personal forks):
depends-onApply depends-on PRsskipped; behavior identical to baselinecherry-pick --abort