Skip to content

ci: apply Depends-On dependencies to the memory report - #20042

Open
zhangning21 wants to merge 1 commit into
apache:masterfrom
zhangning21:feature/membrowse-depends-on
Open

ci: apply Depends-On dependencies to the memory report#20042
zhangning21 wants to merge 1 commit into
apache:masterfrom
zhangning21:feature/membrowse-depends-on

Conversation

@zhangning21

Copy link
Copy Markdown
Contributor

Summary

Teaches the MemBrowse memory report to honour the Depends-On: declarations
that build.yml already understands, so a pull request that builds only with
another one can produce a report from the declared combined source set.

Requested in #19867.

For pull requests targeting master, the workflow re-reads the current
description, parses dependencies with the existing
.github/scripts/depends_on.py, and applies accepted apache/nuttx and
apache/nuttx-apps dependencies to the matching checkout in declaration order.
A dependency already reachable from the checkout is recorded without being
cherry-picked again. Unsupported references are dropped with a warning; a
declaration with no valid references retains the default sources.

Failures that leave the declaration unevaluated, or a parsed dependency
unapplied, stop the job rather than produce a report for the wrong source set.
Checkout deepening is best effort, but the subsequent common-base and
commit-list checks remain fail-closed.

Changes

  • .github/workflows/membrowse-report.yml — add an Apply depends-on PRs step
    and grant pull-requests: read so it can re-read the pull request
    description.
  • .github/scripts/depends_on.py — document that --print-state is a parsed
    output contract with a second consumer.
  • Documentation/testing/nuttx-ci.rst — cross-reference the dependency and
    memory-footprint sections.

Impact

The functional change affects only MemBrowse reports for pull requests
targeting master. Pull requests without a declaration retain the normal source
selection, but the step still re-reads the description and invokes the parser;
a failure in either operation fails the job. Push runs and release-branch pull
requests do not run the dependency step.

The workflow's GITHUB_TOKEN gains pull-requests: read. Its GitHub
permissions remain read-only, and it does not post pull request comments;
commenting stays in the separate privileged membrowse-comment.yml.

Declared dependency code shares the upload job's trust boundary, so
secret-bearing same-repository runs must reference trusted dependency heads;
details are documented below.

No NuttX runtime, board, hardware, or build.yml behaviour changes.
.github/scripts/depends_on.py changes only its module docstring, and the CI
documentation is updated in Documentation/testing/nuttx-ci.rst.

Known limitations

The source set is not sampled atomically across the matrix. Each matrix leg
independently checks out the unpinned nuttx-apps default branch, re-reads the
live pull request body, and resolves dependencies from mutable
refs/pull/N/head refs. The apps baseline was already a mutable input; the live
body read and the mutable dependency refs are the two this change adds. Run
33403284777 observed the same checkout and dependency SHAs in both legs, but
that does not guarantee atomicity. Eliminating the race would require resolving
the complete source set once and distributing immutable SHAs or a shared source
artifact to every leg.

A stale or mixed downstream comment is a conditional risk. The comment
workflow was not exercised by the staging runs. It runs after every
non-cancelled report conclusion, including failures, and queries MemBrowse by
workflow_run.head_sha without an originating run ID or target manifest. A
re-run after editing Depends-On: reuses the same head SHA, so MemBrowse's
overwrite and retention behaviour could expose stale targets or a mixture from
different runs. This was not observed: the staging upload identity and
workflow_run.head_sha matched.

Provenance is per-leg. Each job summary records the pre-apply
apache/nuttx and apache/nuttx-apps checkout SHAs and every dependency head
SHA in declaration order, including already-contained dependencies. These
values can reconstruct the source set while the Git objects remain available,
but there is no combined post-apply tree SHA or matrix-wide manifest.

Dependency inclusion uses commit reachability, not patch equivalence. Like
build.yml, the step cherry-picks HEAD..dep and treats a dependency as
included only when its commits are reachable by identity. If equivalent changes
land under rewritten SHAs, or overlapping stack members are declared
separately, replay may stop as empty, conflict, or apply cleanly and reach the
same tree. Declare only the unlanded stack tip, rebase or update it after an
ancestor lands under rewritten SHAs, and remove declarations once their
dependencies land.

Declared dependencies share the upload job's trust boundary. Unlike the
limitations above, this one is introduced here: before this change the report
workflow built only its own checkouts. Dependency code is built in the same job
before the final upload step receives MEMBROWSE_API_KEY. The key is scoped to
that step, and fork-triggered runs do not receive repository secrets, but
earlier code can still affect the shared workspace and runner environment files.
Secret-bearing same-repository runs must therefore declare only trusted
dependency heads. Full isolation would require separate build and upload jobs
connected by an artifact.

Testing

The staging runs used GitHub-hosted ubuntu-latest runners and the NuttX CI
container. They ran in the zhn-test mirror, so logs show
zhn-test/nuttx-apps where upstream uses apache/nuttx-apps. The staging
revisions also carried mirror mapping and a two-target matrix; neither change is
part of this pull request.

Base-ref gate. Two runs used identical source trees and declarations but
targeted different branches:

Run Base ref Apply depends-on PRs Build
33255928602 membrowse-base-20260824 (master-equivalent) applied both legs success
33373845058 releases/membrowse-e2e skipped both legs success

The corresponding merge commits (7879d12b72, e3e9e6228d) had identical
parents, trees, and workflow content. Staging replaced the literal master with
its master-equivalent branch, so this verifies the gating mechanism rather than
the literal branch name.

Dependency application. Run 33255928602 applied both a same-repository
NuttX dependency (pull/33 @ 995cd8dde0…) and a companion nuttx-apps
dependency (pull/18 @ d826dcfc8a…) in both the ARM
stm32-nucleo-f103rb and RISC-V hifive1-revb legs, then built successfully.

Order, already-included handling, and provenance. Run
33403284777
applied two dependencies in declaration order, recognised a third as already
included, recorded full checkout and dependency SHAs, and built both targets.
The submitted workflow and the tested candidate 4a77864b… are byte-identical
after comments are removed, so the commands and parser behaviour exercised by
the run are unchanged.

The mirror had no MemBrowse API key, so its tokenless uploads returned HTTP 404
and were tolerated by a staging-only continue-on-error. The run therefore
verifies dependency handling, provenance, and builds, but not a successful
MemBrowse upload or the downstream comment workflow. The upload identity still
matched the pull request head SHA.

Local checks. The submitted commit passed 40 parser tests, py_compile,
YAML parsing, bash -n over all 14 shell blocks, and git diff --check.
Extracted-step tests confirmed that body/parser, fetch/history, commit-list, and
cherry-pick failures stop the job. none and invalid retain the default
sources; invalid declarations emit one warning, and unsupported repositories
warn without preventing supported entries in the same declaration from being
applied. The recorded summary contains full source SHAs in declaration order,
including already-contained dependencies.

The RST section references were also checked, and parsing introduced no new
diagnostics.

The memory report builds this pull request merged into master, together
with the nuttx-apps default branch, and nothing else, so a companion or
predecessor pull request it declares is absent. For a breaking change the
target then fails to build and no report is produced at all, even though
the Build workflow already tests the declared sources through Depends-On.

Apply the declared dependencies before building, reusing the parser and
the fetch/cherry-pick sequence that build.yml uses, and mapping each
repository to its checkout exactly as build.yml does. A stacked nuttx
dependency is no more optional than an apps one: a pull request that uses
an API its predecessor introduces does not build without it.

As build.yml does, read the description through the API rather than
trusting the event payload, so that a manual re-run after editing a
Depends-On line applies the current declaration instead of the one the run
was created with. Unlike build.yml, a failed read stops the job rather
than falling back to the payload: build.yml resolves this once and hands
every target the same tree, while this job runs per target, so a fallback
could leave targets on different declarations while their results are
filed under one SHA.

A declared dependency that cannot be applied fails the job, and so does a
missing parser, a parser crash, or a status this step does not recognise:
each of those means the declaration was never evaluated, and continuing
would measure a combination nobody asked for. build.yml fails Fetch-Source
on the same conditions, and no other step in this job carries
continue-on-error, so falling back silently would be inconsistent with
both. A declaration that parses to nothing valid only warns, again
matching build.yml.

Forward the parser's warnings too. --print-state prints only the state, so
an entry the parser drops -- an unsupported repository, say -- would
otherwise leave no trace here at all, although build.yml annotates it, and
the source set named below would be silently incomplete.

The report is filed under the pull request head SHA rather than the SHA of
the tree that was built, so the measurement cannot be reproduced from that
SHA alone and cannot be split per dependency. That limits provenance, not
the measurement: a combined result is what the declaration asks for, and a
regression that only appears in combination is still a regression. Name
the whole source set in the step summary so the reader knows which heads
went into the number.

Note in the parser that the --print-state output is a parsed contract; the
edit gate that used to be its only caller is gone.

Update the CI documentation to match. Its Pull Request Dependencies
section attributes dependency application to build.yml's Fetch-Source
job alone, so after this change it would read as if the memory report
measured the normal source selection. Cross-reference the two sections
rather than restating the rules, which stay shared.

Signed-off-by: zhangning21 <zhangning21@xiaomi.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

@acassis

acassis commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@michael-membrowse please take a look

@michael-membrowse

Copy link
Copy Markdown
Contributor

@acassis looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CI Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants