ci: extract skit.yml job prelude into a composite action (fixes #436)#437
ci: extract skit.yml job prelude into a composite action (fixes #436)#437staging-devin-ai-integration[bot] wants to merge 2 commits into
Conversation
The Lint, Test, Coverage, and Build jobs in .github/workflows/skit.yml
each repeated the same prelude (free-disk-space, Bun, UI build, apt
packages, Rust toolchain, rust-cache). Bumping a tool version meant
editing the same block in four places, which has already produced at
least one mismatch (different action SHAs on otherwise-identical steps).
Move the prelude to a new local composite action
.github/actions/setup-skit, with inputs covering the job-to-job
variations:
- rust-cache-key (default "skit"; Coverage overrides to
"skit-coverage")
- rust-cache-workspaces (default ""; Coverage overrides to
". -> target/coverage")
- rust-components (Lint: "rustfmt, clippy"; Coverage:
"llvm-tools-preview"; others: empty)
- skip-ui-build (default "false"; reserved for future
non-Rust-server jobs)
Each migrated job now collapses to actions/checkout +
uses: ./.github/actions/setup-skit + inputs that differ. sccache stays
per-job because Coverage opts out of it entirely, and the issue scoped
sccache out of the composite.
Out of scope per #436: Test (GPU) uses a different self-hosted prelude;
.github/workflows/ui.yml has less duplication and is not in scope;
.github/workflows/e2e.yml has significant variation and is not in
scope. Closes #436.
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #437 +/- ##
=======================================
Coverage 62.90% 62.90%
=======================================
Files 215 215
Lines 55435 55435
Branches 1597 1597
=======================================
Hits 34874 34874
Misses 20555 20555
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| shared-key: ${{ inputs.rust-cache-key }} | ||
| workspaces: ${{ inputs.rust-cache-workspaces }} | ||
| save-if: ${{ github.ref == 'refs/heads/main' }} | ||
| cache-on-failure: true |
There was a problem hiding this comment.
🔴 Rust cache now runs before sccache is installed
The new composite action always invokes Swatinem/rust-cache as part of setup, but the reusable workflow still sets RUSTC_WRAPPER: sccache globally (.github/workflows/skit.yml:6-9) and only installs the sccache binary later in the lint/test/build jobs (.github/workflows/skit.yml:22-30, .github/workflows/skit.yml:65-73, .github/workflows/skit.yml:201-209). Those jobs now run rust-cache while Cargo/Rust tooling is configured to use a wrapper that is not on PATH yet, the exact failure mode the coverage job avoids by clearing RUSTC_WRAPPER before rust-cache (.github/workflows/skit.yml:94-101). As a result, the main CI lint/test/build jobs can fail during setup before reaching their cargo commands.
Prompt for agents
The `setup-skit` composite action currently runs `Swatinem/rust-cache` before the lint/test/build jobs install sccache, while those jobs inherit `RUSTC_WRAPPER=sccache` from `.github/workflows/skit.yml`. Move the rust-cache restore back to a point after `mozilla-actions/sccache-action` for jobs that use sccache, or otherwise ensure sccache is installed/on PATH before any rust-cache/Cargo/Rust tooling runs. Keep the coverage job's special handling intact because it intentionally clears `RUSTC_WRAPPER` and uses a separate coverage target directory/cache key.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Thanks — I tracked this one carefully before pushing. Believe this is a false positive; flagging the evidence so a human reviewer can decide whether to override:
-
Swatinem/rust-cachedoes not consultRUSTC_WRAPPER. The action callsrustc -vVdirectly via@actions/execto detect the toolchain version — that's a direct process spawn, andRUSTC_WRAPPERis a Cargo concept (Cargo prepends it when Cargo invokes rustc, not when the OS executes rustc as a top-level command). Likewise the action'scargo metadata --all-featuresdoes not need to compile anything;cargo metadatadoes not run rustc-via-wrapper for the dependency graph. -
CI confirms. This PR's first run (commit
9e4a018, before any further changes) finished with all four migrated jobs green:Skit / Lint (fmt + clippy)✅,Skit / Test✅,Skit / Coverage✅,Skit / Build✅. If rust-cache were tripping on the missing sccache binary during its setup, Lint/Test/Build would have failed at the rust-cache step, well before anycargo …command. The Coverage job'sRUSTC_WRAPPER: ""override is for a different reason called out in the comment block at.github/workflows/skit.yml:94-101: instrumented Coverage builds don't cache usefully across PRs, and the workflow-level wrapper would otherwise route Coverage'scargo llvm-covinvocations through sccache (with sccache uninstalled in that job) — i.e. the override protects thecargo llvm-covstep, not therust-cachestep.
If a future change makes rust-cache start probing the wrapper (or you'd rather not rely on the above contract), the safe knob is to either move mozilla-actions/sccache-action into the composite (gated on a new skip-sccache input) or set env: RUSTC_WRAPPER: "" job-wide in the composite caller until after sccache is installed. Happy to switch to either if you'd prefer — let me know.
| @@ -0,0 +1,80 @@ | |||
| name: Set up Skit CI environment | |||
There was a problem hiding this comment.
🔴 New composite action is missing the required SPDX header
CONTRIBUTING.md requires SPDX license headers on all new files, and AGENTS.md explicitly incorporates that requirement for agent-assisted changes. This PR adds .github/actions/setup-skit/action.yml without the required SPDX-FileCopyrightText and SPDX-License-Identifier header, so the new file violates the repository's mandatory contribution rules.
| name: Set up Skit CI environment | |
| # SPDX-FileCopyrightText: © 2025 StreamKit Contributors | |
| # | |
| # SPDX-License-Identifier: MPL-2.0 | |
| name: Set up Skit CI environment |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good catch on the convention even though the file was already REUSE-compliant via the **/*.yml aggregate annotation in REUSE.toml (REUSE Compliance CI check is green on this PR). Added the explicit header in 56a09e6 so the new action.yml matches the inline-header convention used by docker.yml, docs.yml, e2e.yml, plugins.yml, and release.yml. (The pre-existing skit.yml, ui.yml, ci.yml rely on the toml annotation — left those alone since they're out of scope here.)
| - uses: ./.github/actions/setup-skit | ||
| with: | ||
| bun-version: "1.3.5" | ||
|
|
||
| - name: Build UI | ||
| working-directory: ./ui | ||
| run: | | ||
| bun install --frozen-lockfile | ||
| bun run build | ||
|
|
||
| - name: Install system dependencies (libvpx for VP9, nasm for AV1 assembly) | ||
| run: sudo apt-get update && sudo apt-get install -y libvpx-dev nasm | ||
|
|
||
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: "1.95.0" | ||
| components: llvm-tools-preview | ||
|
|
||
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| shared-key: skit-coverage | ||
| # Point the cache at the same target dir we redirected Cargo to | ||
| # via CARGO_LLVM_COV_TARGET_DIR. Without this, rust-cache would | ||
| # cache the (empty) default target/ and skip the actual build | ||
| # artifacts under target/coverage. | ||
| workspaces: ". -> target/coverage" | ||
| save-if: ${{ github.ref == 'refs/heads/main' }} | ||
| cache-on-failure: true | ||
| rust-cache-key: skit-coverage | ||
| rust-cache-workspaces: ". -> target/coverage" | ||
| rust-components: llvm-tools-preview |
There was a problem hiding this comment.
📝 Info: Coverage cache isolation is preserved by the refactor
The coverage job still passes the separate skit-coverage cache key and ". -> target/coverage" workspaces override into the composite action, while its job-level environment continues to clear RUSTC_WRAPPER. This preserves the previous intent of keeping instrumented coverage artifacts out of the normal shared rust-cache and avoiding sccache in coverage runs (.github/workflows/skit.yml:94-109).
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Acknowledged — that was the explicit intent. The composite action's rust-cache-key and rust-cache-workspaces inputs were sized precisely to let Coverage keep its skit-coverage shared-key + . -> target/coverage redirect, and the job-level env: RUSTC_WRAPPER: "" override stays in skit.yml because the env behaviour is per-job, not part of the prelude.
| skip-ui-build: | ||
| description: > | ||
| If "true", skip building ./ui/dist. streamkit-server's RustEmbed | ||
| directive requires ./ui/dist to exist at compile time, so most Rust | ||
| jobs need this false. | ||
| required: false | ||
| default: "false" |
There was a problem hiding this comment.
📝 Info: The new skip-ui-build input is currently unused
The composite action adds a skip-ui-build input, but all current call sites in skit.yml use the default, so this PR does not actually change which Skit CI jobs build ./ui/dist. That matters because streamkit-server embeds ./ui/dist at compile time, so any future caller that sets this input will need to ensure it only runs Cargo commands that do not compile the embedding crate or otherwise creates ui/dist first.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Yes, intentional — called out in the PR description as well. The issue (#436) proposed skip-ui-build because "Lint job doesn't need it", but in practice cargo clippy --workspace --all-targets does compile streamkit-server, which carries #[derive(RustEmbed)] #[folder = "../../ui/dist/"] (apps/skit/src/server/mod.rs:76-78) — RustEmbed needs the folder to exist at compile time, so Lint still needs the UI build. I kept the input as a future-proofing knob (so a follow-up that adds e.g. a cargo check -p streamkit-core-only job can flip it on) but no current caller sets it.
Matches the convention used by docker.yml, docs.yml, e2e.yml, plugins.yml, and release.yml under .github/workflows/. The file was already REUSE compliant via the **/*.yml annotation in REUSE.toml, but inlining the header makes the licensing self-evident on first read. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
3 participants
Summary
Closes #436.
The
Lint,Test,Coverage, andBuildjobs in.github/workflows/skit.ymlall repeated the same prelude (jlumbroso/free-disk-space→oven-sh/setup-bun→ UI build →apt-get install libvpx-dev nasm→dtolnay/rust-toolchain→Swatinem/rust-cache). Bumping a tool version meant editing the same block in four places and we already had at least one mismatch from manual sync drift.This PR moves the prelude to a new local composite action at
.github/actions/setup-skit/action.ymland migrates the four jobs to call it. Each migrated job now collapses toactions/checkout@v5+uses: ./.github/actions/setup-skit(with the inputs that differ) + the job-specific cargo command.Composite action inputs
rust-cache-keyskitSwatinem/rust-cacheshared-key. Coverage overrides toskit-coverage.rust-cache-workspaces"".. Coverage overrides to. -> target/coverage.rust-components""rustfmt, clippy; Coverage setsllvm-tools-preview; Test/Build use the default.skip-ui-build"false"ui/dist).Things I deliberately did NOT change
Test (GPU)— self-hosted runner, nofree-disk-space, different apt package set; left untouched..github/workflows/ui.yml— out of scope per ci: extract per-job prelude (free-disk → checkout → bun → ui-build → apt → rust) into a composite action #436..github/workflows/e2e.yml— out of scope per ci: extract per-job prelude (free-disk → checkout → bun → ui-build → apt → rust) into a composite action #436.RUSTC_WRAPPER=""), so the per-jobactions/cachesave/restore for~/.cache/sccacheandmozilla-actions/sccache-action@v0.0.9remain in each non-Coverage job.One small behavioural side-effect to flag
Inside the migrated jobs, the order is now
checkout → setup-skit (ends with rust-cache) → restore sccache → sccache-action → cargo …, where previously it was… → restore sccache → sccache-action → rust-cache → cargo ….Swatinem/rust-cacheand the sccache disk cache touch disjoint paths (~/.cargo+target/vs~/.cache/sccache), andRUSTC_WRAPPER=sccacheis still set at the workflowenv:level before any cargo command runs, so this should be a no-op semantically. The first run on this PR is the proof point.actions/checkout@v5has to run inside each job beforeuses: ./.github/actions/setup-skitbecause GitHub Actions resolves local-path composites from the workspace at runtime — that's why checkout is the only step the composite cannot absorb.Verification
python3 -c 'import yaml; ...'parses both files.actionlint 1.7.12clean against.github/workflows/skit.yml.reuse --no-multiprocessing lintpasses (**/*.ymlis annotated byREUSE.toml).-scommit.Review & Testing Checklist for Human
Risk: yellow (CI-only refactor, but it changes the prelude path for 4 jobs and CI runs in the cloud cost real time to validate).
Coveragejob — it has the most variation (custom cache key, redirected workspace, extra rust component, andcontinue-on-error: trueis preserved). Make sure it still producestarget/coverage/lcov.infoand uploads to Codecov.skip-ui-buildshould actually be set totrueforLintin a follow-up. The issue body called it out as the motivating case, butstreamkit-server's#[derive(RustEmbed)] #[folder = "../../ui/dist/"]requires the directory to exist at compile time, socargo clippy --workspacewould fail withoutui/dist. I kept Lint building the UI; if you want to drop it, the change is single-line and lives inlint:inskit.yml.Notes
name:,description:, and per-inputdescription:strings are where the documentation lives; the workflow file itself stays comment-free per the repo's comment guidelines.Test (GPU),ui.yml,e2e.yml,codecov.yml,justfile, or any Rust/TS source.Link to Devin session: https://staging.itsdev.in/sessions/585f144b7c2d4f72ab78b25793feedc7
Requested by: @streamer45
Devin Review
9e4a018(HEAD is56a09e6)