perf(ci): parallelize release bundle builds - #596
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds validated firmware bundle generation, MOD archive flashing and verification, release-target scripts, xsdb commands, and parallel CI jobs for firmware and WASM artifacts. Documentation is updated for the revised build, flashing, debugging, and output flows. ChangesFirmware release and device tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FirmwareBuild
participant WebBuild
participant BundleBuild
FirmwareBuild->>BundleBuild: publish release target artifacts
WebBuild->>BundleBuild: publish WASM simulator artifact
BundleBuild->>BundleBuild: download artifacts and run bundle:package
BundleBuild-->>BundleBuild: upload final firmware bundle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cbe23a12e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function resolveModArchivePath({ outputDirectory, mode, projectName }) { | ||
| if (!outputDirectory) throw new Error('MOD output directory is required') | ||
| if (!/^[^/\\]+$/.test(projectName)) throw new Error(`Invalid MOD project name: ${projectName || 'missing'}`) | ||
| const outputMode = mode === 'debug' ? 'debug' : 'release' |
There was a problem hiding this comment.
When npm run mod -- <manifest> --mode=instrument is used, firmware.mjs passes -i to mcrun, so the archive is emitted under the instrument build-mode directory. This mapping treats every non-debug mode as release, causing the subsequent install step to read a nonexistent or stale release archive instead of the newly built instrument archive. Map instrument to its own output directory, as is already done for other managed build outputs.
Useful? React with 👍 / 👎.
Cloudflare PR previewOpen the latest preview for commit Immutable deployment: https://93e33283.stack-chan-pr-preview.pages.dev Warning Pull request previews contain untrusted web and firmware code. Review the changes before granting WebSerial/Bluetooth permissions or flashing a device. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
firmware/scripts/lib/firmware-bundle.mjs (1)
132-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the generated wrapper manifest out of
host/app.
prepareBundleManifestcreatesbundleManifestPathunderpath.dirname(target.manifestPath), while both included manifests are absolute and the override manifest already lives underoutputDirectory/generated/bundle-manifests/<target>. Keeping the temporary wrapper in the trackedfirmware/host/appsource directory is unnecessary and leaves a stray file behind ifmcconfigterminates before the cleanupfinallyblock runs.♻️ Proposed fix to co-locate the wrapper manifest with the override manifest
function prepareBundleManifest(target, sdkconfigDirectory, outputDirectory) { const directory = path.join(outputDirectory, 'generated', 'bundle-manifests', target.name) const overrideManifestPath = path.join(directory, 'sdkconfig.json') - const bundleManifestPath = path.join( - path.dirname(target.manifestPath), - `${firmwareBundleName}.${target.name}.${process.pid}.manifest.json`, - ) + const bundleManifestPath = path.join( + directory, + `${firmwareBundleName}.${target.name}.${process.pid}.manifest.json`, + ) mkdirSync(directory, { recursive: true })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firmware/scripts/lib/firmware-bundle.mjs` at line 132, Update prepareBundleManifest and its caller to create the generated wrapper manifest under outputDirectory/generated/bundle-manifests/<target>, alongside the override manifest, instead of deriving its location from path.dirname(target.manifestPath). Preserve absolute manifest paths and ensure cleanup uses the new generated location.firmware/scripts/lib/mod-flash.mjs (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider defaulting scratch files under
firmware/dist/tmpinstead of the OS temp dir.
temporaryDirectorydefaults totmpdir()(OS-wide temp), so the partition-table/app-header scratch files land outside the repo's managed output tree. It's cleaned viafinallyon the happy path, but a killed process leaves stray directories outsidenpm run clean's reach.♻️ Suggested default
- temporaryDirectory = tmpdir(), + temporaryDirectory = path.join(buildOutputDirectory, 'tmp'),As per coding guidelines: "Normal host, MOD, and test build outputs must remain under
firmware/dist/bin/andfirmware/dist/tmp/; do not bypass the repository output isolation."Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firmware/scripts/lib/mod-flash.mjs` at line 60, Update the temporaryDirectory default in the module’s flash workflow to use the repository-managed firmware/dist/tmp location instead of the OS tmpdir(). Preserve any caller-provided temporary directory and ensure the default directory is created or resolved consistently before scratch files are written, so outputs remain within the repository’s managed tree.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/bundle.yml:
- Line 61: Update the checkout steps in the firmware, web, and build jobs to set
with.persist-credentials to false, matching the existing Pages baseline/preview
checkout configuration. Preserve the current checkout actions and job behavior
otherwise.
---
Nitpick comments:
In `@firmware/scripts/lib/firmware-bundle.mjs`:
- Line 132: Update prepareBundleManifest and its caller to create the generated
wrapper manifest under outputDirectory/generated/bundle-manifests/<target>,
alongside the override manifest, instead of deriving its location from
path.dirname(target.manifestPath). Preserve absolute manifest paths and ensure
cleanup uses the new generated location.
In `@firmware/scripts/lib/mod-flash.mjs`:
- Line 60: Update the temporaryDirectory default in the module’s flash workflow
to use the repository-managed firmware/dist/tmp location instead of the OS
tmpdir(). Preserve any caller-provided temporary directory and ensure the
default directory is created or resolved consistently before scratch files are
written, so outputs remain within the repository’s managed tree.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21d4485c-15c2-4635-a4d0-29a8873c6e5c
📒 Files selected for processing (18)
.github/workflows/build.yml.github/workflows/bundle.ymlAGENTS.mdfirmware/README.mdfirmware/README_ja.mdfirmware/docs/flashing-firmware.mdfirmware/docs/flashing-firmware_ja.mdfirmware/package.jsonfirmware/scripts/bundle-target.mjsfirmware/scripts/bundle.mjsfirmware/scripts/firmware.mjsfirmware/scripts/lib/devices.mjsfirmware/scripts/lib/firmware-bundle.mjsfirmware/scripts/lib/firmware-bundle.test.mjsfirmware/scripts/lib/mod-flash.mjsfirmware/scripts/lib/mod-flash.test.mjsfirmware/scripts/lib/moddable-version.mjsfirmware/scripts/package-bundle.mjs
💤 Files with no reviewable changes (1)
- .github/workflows/build.yml
1 participant
Summary
build.ymlwhile preserving per-platform failure visibility in the bundle workflow.npm run bundleas a local compatibility entry point.mcrun, then discover and write the livexspartition with esptool, including firmware/version/size preflight and digest verification.debug:xsdbscripts for interactive debugging.Why
The bundle workflow rebuilt standard firmware targets serially through
mcbundle, making one run take roughly 20 minutes. Reusing the existing target build coverage as release builds makes the critical path the slowest target instead of the sum of all target builds.The remaining blocker was MOD installation: it depended on the debug-oriented
mcrunwrite path. Moving MOD writes to esptool makes MOD installation independent of whether the host is a debug or release build, so CI can validate release builds without losing the developer debug/xsdb path.Impact
fail-fast: false).npm run modnow writes and verifies the livexspartition instead of usingmcrunfor transport.Validation
npm run test:unit— 219 tests passed on the rebased headnpm run formatnpm run lintnode --test .github/scripts/*.test.mjsgit diff --check/dev/ttyACM1: flashed a release host, installed and verified a MOD through esptool, and ran the servo/LED smoke MOD to completionserial2xsbug; the main thread was reported as runningnpm run debug:xsdbitself still requires the expected physical CoreS3 download-button/re-enumeration step; the underlying deploy and xsdb connection paths were verified separately.Release impact
none— this changes CI/developer tooling and documentation only; it does not change released firmware or web behavior.Summary by CodeRabbit
New Features
xsdbdebug commands and improved MOD installation with device partition discovery, validation, and optional serial port selection.Documentation
Tests
Chores