fix(ci): tvOS on the pinned stable toolchain + one toolchain pin everywhere#322
Merged
Conversation
The buildbot re-run after #317 took the recipe from 1/10 to 9/10 jobs (pipeline #91954). The one remaining failure, `libretro-build-tvos-arm64`, was the only job not running on our pinned 1.96.0, and every problem it had followed from that. Putting it back on the pin fixes it and removes three stacked workarounds instead of adding a fourth. The obsolete override --------------------- The upstream tvOS template is the only Apple template that also overrides `script`, replacing the shared Apple build line with: cargo +nightly build -Zbuild-std --target aarch64-apple-tvos --release That dates from when `aarch64-apple-tvos` was a tier-3 target with no distributed `rust-std`, making `-Zbuild-std` -- and therefore nightly -- the only way to build it. The target has since been promoted and rustup now ships a complete prebuilt std for it. Verified against the pinned 1.96.0: `rustup target add aarch64-apple-tvos` installs 26 rlibs, `panic_abort` among them, and `cargo check --release -p rustynes-libretro --target aarch64-apple-tvos` compiles the entire dependency graph, bindgen included. This job now overrides `script` back to `!reference [.libretro-rust-apple-base, script]` -- the generic Apple build/package steps -- rather than duplicating them, so upstream fixes to that script keep reaching us. It drives off `TARGET_ARCH`, which the tvOS template already sets, and honours the same `STRIP_CORE_LIB` / `DSYM_CORE_LIB` flags. The job's shape now matches osx-x64 / osx-arm64 / ios-arm64 exactly: template `before_script` plus `rustup target add`. Three workarounds dissolved --------------------------- 1. MSRV. `+nightly` outranks BOTH `rust-toolchain.toml` and `RUSTUP_TOOLCHAIN`, so the job rode the image's stale 1.94.0-nightly -- below the workspace's `rust-version = "1.96"` -- and cargo's MSRV gate failed before compiling anything. #317 worked around it by uninstalling and reinstalling the nightly channel. No longer needed. 2. `panic_abort`. A bare `-Zbuild-std` builds `std` but NOT `panic_abort`, which `[profile.release] panic = "abort"` requires, so the cdylib failed to link with `E0463`. The crate list is hardcoded in the template and `CARGO_UNSTABLE_BUILD_STD` cannot override it (the CLI `-Z` flag wins), so #317 had to force `panic = "unwind"` for this platform alone. The prebuilt std ships `panic_abort`, so tvOS now honours `panic = "abort"` exactly like every other platform -- this is a small accuracy win over #317, not merely a simplification. 3. `-C ar`. Refreshing to a current nightly then produced the failure that prompted this change: error: failed to run `rustc` to learn about target-specific information error: `-C ar`: this option has been removed The build image injects `-Car=<path>,Clink-arg=-undefined,Clink-arg=dynamic_lookup,-rpath=<path>` into every Apple job -- it is absent from the `rust-apple.yml` template (which sets no RUSTFLAGS at all) and from our `.cargo/config.toml` (wasm32-only). `-C ar` was a deprecated no-op for years and became a hard error in Rust 1.97; bisected against local toolchains, 1.93.0-nightly / 1.96.0 / 1.96.1 warn, while 1.97.1 and 1.99.0-nightly error. On the pinned 1.96.0 it is still only a warning, exactly as it already was for the three Apple jobs that stayed green throughout. Latent hazard recorded, not patched ----------------------------------- All four Apple jobs receive `-C ar` and are green only because 1.96.0 treats it as a warning. Bumping `channel` to 1.97+ breaks all four together, so the warning and its remedy live in `rust-toolchain.toml`, at the line such a bump would edit. The remedy is `unset CARGO_ENCODED_RUSTFLAGS` + `export RUSTFLAGS=""`: cargo takes extra flags from exactly one source, first match wins (CARGO_ENCODED_RUSTFLAGS -> RUSTFLAGS -> target.<triple>.rustflags -> build.rustflags), so that pair wins without needing to know where the image sets them. Discarding them is behaviour-preserving -- rustc splits `-C` at the first `=`, so the whole comma-joined string is consumed as the `ar` value and those link arguments have never reached the linker for any core. Verified locally against the same class of source (a global `~/.cargo/config.toml` `build.rustflags`): `RUSTFLAGS=""` removes every injected `-C` flag, and an empty value means zero flags rather than one empty argument. Not a defect: `android-x86` also failed in the `main` pipeline (#91965), but its log shows the runner could not pull the Docker image (`Client.Timeout exceeded while awaiting headers`). It never ran a build and passed on the branch pipeline with identical code. Verification ------------ No Rust source changes, so the emulation core is untouched by construction -- AccuracyCoin stays 141/141 and every golden vector is unaffected. Ran: `pre-commit` (scoped), `cargo fmt --all --check`, `cargo clippy --workspace --all-targets -D warnings`, the `no_std` thumbv7em build, both `libretro-cross` guard legs (`x86_64-pc-windows-gnu`, `aarch64-linux-android`), and a `cargo check` for `aarch64-apple-tvos` itself. Linking tvOS still cannot be reproduced locally -- there is no tvOS SDK or linker here, and we cannot run a pipeline on git.libretro.com. Addresses #311, which stays open until the buildbot is confirmed green.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe shared Rust setup action now resolves the toolchain from an explicit input or ChangesRust toolchain centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI workflow
participant rust-setup
participant rust-toolchain.toml
participant Rust installer
CI workflow->>rust-setup: invoke without a toolchain override
rust-setup->>rust-toolchain.toml: read channel
rust-setup->>Rust installer: install resolved channel
Rust installer-->>CI workflow: provide configured Rust toolchain
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Updates the libretro GitLab CI tvOS job to build on the workspace-pinned stable Rust toolchain (1.96.0) instead of the upstream +nightly -Zbuild-std override, and documents the remaining Apple-job -C ar hazard for the next MSRV bump.
Changes:
- Restore
libretro-build-tvos-arm64to the shared Apple build script and install the tvOS target into the pinned toolchain viarustup target add. - Add a pinned-toolchain warning in
rust-toolchain.tomlabout the injected-C arflag becoming a hard error on Rust 1.97+. - Record the CI outcome and guidance updates in
CHANGELOG.mdandAGENTS.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.gitlab-ci.yml |
Removes the tvOS-only nightly/build-std path; installs aarch64-apple-tvos into the pinned toolchain and uses the shared Apple script. |
rust-toolchain.toml |
Documents the -C ar injected-flag hazard that will break Apple jobs on a future bump to 1.97+. |
CHANGELOG.md |
Adds release notes describing the tvOS job moving back to pinned stable and dropping prior workarounds. |
AGENTS.md |
Updates contribution guidance around GitHub issue auto-close keywords (including quoted cases). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…erywhere
Completes the "one toolchain, no nightly" sweep the tvOS fix started. After
this there is no `toolchain:` version literal anywhere in `.github/`, and
every build path -- GitHub Actions, the libretro buildbot, and local builds --
runs on the single pin in `rust-toolchain.toml`.
What was actually wrong
-----------------------
`.github/actions/rust-setup` defaulted to `stable`, and 5 of its 12 call sites
overrode that with an explicit `toolchain: 1.96.0`. The remaining 7 -- `test`,
`test-roms`, `no_std`, `wasm`, `bench`, `release.yml`, `web.yml` -- took the
default.
That was misleading rather than broken. `rust-toolchain.toml` is a directory
override and outranks the `rustup default` this action performs, so every one
of those jobs was ALREADY compiling on 1.96.0; rustup says so in the log:
toolchain '1.96.0' is currently in use (overridden by .../rust-toolchain.toml)
So the `stable` default had two effects, both bad and neither a build error:
it downloaded a second toolchain that nothing subsequently used, and it made
the workflows *read* as though the matrix tested against latest stable -- a
guarantee the repo appeared to offer and never actually provided. With stable
now at 1.97.1, that misreading was about to matter: `-C ar` is a hard error
there (see the previous commit), so a reader could reasonably assume the Apple
jobs were already exercising 1.97.
The fix
-------
`rust-setup` gains a `Resolve Rust toolchain` step that parses the `channel`
out of `rust-toolchain.toml` and passes it to `dtolnay/rust-toolchain`. The
`toolchain` input still exists and still wins when set, but now means "install
something deliberately different from the project pin" rather than "the
default is a different channel from the one we build with".
Fail-closed: an unparseable or channel-less `rust-toolchain.toml` aborts the
job under `set -euo pipefail` with a `::error::` annotation, rather than
silently falling through to some other channel. Verified both branches locally
against the real file (resolves `1.96.0`) and a channel-less one (empty ->
abort).
The 5 explicit `toolchain: 1.96.0` inputs are removed as redundant; each site
had other `with:` keys, so no empty mappings result. Net effect on bumping the
project toolchain: 6 edits -> 1.
No behaviour change to any compile. Every job compiled on 1.96.0 before this
commit and compiles on 1.96.0 after it; what changes is that the declaration
now matches, one `rust-std` download per job is saved, and a future call site
cannot silently drift onto a different channel.
Nightly audit
-------------
Swept the whole tree. Nightly survives in exactly two places, neither of them
a build, test, lint, docs, release, or packaging gate:
* `fuzz/` -- `cargo fuzz` is a hard requirement: libFuzzer's sanitizer flags
(`-Z sanitizer=...`) are nightly-only. Not run in CI.
* `crates/rustynes-monetization/uniffi-bindgen.rs` -- UniFFI's standalone
bindgen needs nightly to discover its metadata. Dormant crate, not in CI.
Neither can be moved to stable without breaking the tool, so both are left
alone and documented rather than quietly worked around. Every other `nightly`
match in the tree is prose: scheduled-cron wording in the testing docs,
release-channel wording in ADR 0003, and the historical narrative in
CHANGELOG / AGENTS / the `.gitlab-ci.yml` comment explaining the removal.
Docs
----
`docs/build-and-tooling.md` said "Channel: stable" -- now corrected to the
pinned 1.96.0, with the single-source-of-truth rule, the pointer to the `-C ar`
warning that guards a 1.97+ bump, and the two nightly exceptions stated
explicitly.
Verification
------------
No Rust source changes. All workflows and the composite action parse; the
resolve step was simulated against the real `rust-toolchain.toml` and a
malformed one; `pre-commit` clean on every touched file; and `grep` confirms
zero toolchain version literals remain under `.github/`.
doublegate
changed the title
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/actions/rust-setup/action.yml:
- Line 52: Update the comment near the pinned channel in the Rust setup action
to describe the overridden project toolchain without embedding a Rust version
literal. Keep the CI configuration behavior unchanged and ensure no version
literal is added under .github/.
- Around line 71-75: The channel resolver in the setup step currently matches
the first channel key anywhere in rust-toolchain.toml; update the parsing around
resolved to read only channel from the [toolchain] table, using a TOML parser or
explicit table-scoped extraction. Preserve the existing fail-closed error and
exit behavior when [toolchain].channel cannot be resolved.
In `@docs/build-and-tooling.md`:
- Around line 9-10: Update the Nightly statement in the build-and-tooling
documentation to say it is used for exactly two things, while preserving the
existing descriptions of cargo fuzz and the dormant rustynes-monetization
uniffi-bindgen helper.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: db641c18-ee90-4e83-a0fd-675d19fe1542
📒 Files selected for processing (9)
.github/actions/rust-setup/action.yml.github/workflows/ci.yml.github/workflows/pgo.yml.github/workflows/security.yml.gitlab-ci.ymlAGENTS.mdCHANGELOG.mddocs/build-and-tooling.mdrust-toolchain.toml
💤 Files with no reviewable changes (2)
- .github/workflows/security.yml
- .github/workflows/pgo.yml
Adopts all three CodeRabbit findings on the toolchain-resolution change.
1. (Major, real bug) The resolver matched the first `channel = "..."` key
anywhere in `rust-toolchain.toml`, not `[toolchain].channel`. A `channel`
key in any earlier table would have been installed instead -- and the
worked example is not hypothetical:
[other]
channel = "nightly"
[toolchain]
channel = "1.96.0"
The old sed pipeline returns `nightly` on that input (verified), which
would have silently reintroduced the very toolchain this branch removes,
while the step still reported success. That also contradicted the
fail-closed property claimed one comment above it.
Extraction is now table-scoped via awk: it tracks the current TOML table
header and only reads `channel` while inside `[toolchain]`. Verified
against the real file (1.96.0), a decoy `channel` before the table
(1.96.0), a decoy after (1.96.0), a file with no `[toolchain]` table
(empty -> abort), a single-quoted value (empty -> abort), and an empty
file (empty -> abort).
Only TOML basic (double-quoted) strings are accepted. A literal
single-quoted channel is valid TOML but unused here, and failing loudly
on one beats guessing at its value -- so it lands on the fail-closed path
by design, now stated in the comment.
2. (Minor) A comment quoted rustup's override message with the literal
version in it, under `.github/` -- the one place this change otherwise
removes version literals from, and a spot that would go stale on a bump.
Replaced with `<channel>`.
3. (Minor) `docs/build-and-tooling.md` said nightly is used for "exactly one
thing" and then described two, contradicting both AGENTS.md and this
branch's own CHANGELOG entry. Corrected to two.
No behaviour change for the committed `rust-toolchain.toml`, which resolves
to 1.96.0 before and after; the difference is that a malformed or adversarial
file now aborts instead of installing something unintended.
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 11 minutes. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
The libretro buildbot re-run after #317 took the recipe from 1/10 to 9/10 jobs (pipeline #91954). This closes out the last one,
libretro-build-tvos-arm64.It was the only job not running on our pinned 1.96.0, and every problem it had followed from that. Putting it back on the pin fixes it and removes three stacked workarounds instead of adding a fourth.
The obsolete override
The upstream tvOS template is the only Apple template that also overrides
script:That dates from when
aarch64-apple-tvoswas a tier-3 target with no distributedrust-std, making-Zbuild-std— and therefore nightly — the only way to build it. The target has since been promoted and rustup now ships a complete prebuilt std. Verified against the pinned 1.96.0:rustup target add aarch64-apple-tvosinstalls 26 rlibs,panic_abortamong themcargo check --release -p rustynes-libretro --target aarch64-apple-tvoscompiles the entire graph, bindgen includedThe job now overrides
scriptback to!reference [.libretro-rust-apple-base, script]rather than duplicating the steps, so upstream fixes keep reaching it. Its shape now matches osx-x64 / osx-arm64 / ios-arm64 exactly.Three workarounds dissolved
+nightlyoutranks bothrust-toolchain.tomlandRUSTUP_TOOLCHAIN, so the job rode the image's stale 1.94.0-nightly — belowrust-version = "1.96", failing cargo's MSRV gateCARGO_PROFILE_RELEASE_PANIC=unwind-Zbuild-stdomitspanic_abort, whichpanic = "abort"requires (CARGO_UNSTABLE_BUILD_STDcan't override the hardcoded crate list — the CLI-Zwins)panic = "abort"like every other platform — a small accuracy win, not just a simplification-C arThe failure that prompted this
The build image injects
-Car=<path>,Clink-arg=-undefined,Clink-arg=dynamic_lookup,-rpath=<path>into every Apple job — it is absent fromrust-apple.yml(which sets noRUSTFLAGSat all) and from our.cargo/config.toml(wasm32-only).-C arwas a deprecated no-op for years and became a hard error in Rust 1.97. Bisected against local toolchains:-C arwarning: … deprecated and does nothingerror: … has been removedSo #317's nightly refresh — necessary at the time — walked straight into it. On the pinned 1.96.0 it is only a warning, exactly as it already was for the three Apple jobs that stayed green throughout.
Latent hazard recorded, not patched
All four Apple jobs receive
-C arand are green only because 1.96.0 treats it as a warning. Bumpingchannelto 1.97+ breaks all four together, so the warning and its remedy live inrust-toolchain.toml, at the line such a bump would edit.The remedy is
unset CARGO_ENCODED_RUSTFLAGS+export RUSTFLAGS="". Cargo takes extra flags from exactly one source, first match wins (CARGO_ENCODED_RUSTFLAGS→RUSTFLAGS→target.<triple>.rustflags→build.rustflags), so that pair wins without knowing where the image sets them. Discarding them is behaviour-preserving: rustc splits-Cat the first=, so the whole comma-joined string is consumed as thearvalue and those link arguments have never reached the linker for any core. Verified locally against the same class of source (a global~/.cargo/config.tomlbuild.rustflags):RUSTFLAGS=""removes every injected-Cflag, and an empty value means zero flags, not one empty argument.Not a defect
android-x86also failed in themainpipeline (#91965), but its log shows the runner could not pull the Docker image (Client.Timeout exceeded while awaiting headers). It never ran a build, and passed on the branch pipeline with identical code. Nothing to fix.Verification
No Rust source changes, so the emulation core is untouched by construction — AccuracyCoin stays 141/141 and every golden vector is unaffected.
pre-commit(scoped),cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warningsno_stdthumbv7em buildlibretro-crossguard legs (x86_64-pc-windows-gnu,aarch64-linux-android)cargo check --release -p rustynes-libretro --target aarch64-apple-tvosHonest caveat: linking tvOS still cannot be reproduced locally — there is no tvOS SDK or linker here, and we cannot run a pipeline on
git.libretro.com. The compile path is verified; the link step is not.Addresses #311, which stays open until
hizzlekizzleconfirms a green pipeline.Second commit: one toolchain pin everywhere (no
nightly, no floatingstable)Completes the sweep the tvOS fix started.
What was actually wrong
.github/actions/rust-setupdefaulted tostable, and 5 of its 12 call sites overrode that with an explicittoolchain: 1.96.0. The other 7 (test,test-roms,no_std,wasm,bench,release.yml,web.yml) took the default.That was misleading rather than broken.
rust-toolchain.tomlis a directory override and outranks therustup defaultthe action performs, so all of those jobs were already compiling on 1.96.0 — rustup says so in the log:So the
stabledefault did two things, both bad, neither a build error: it downloaded a second toolchain nothing then used, and it made the workflows read as though the matrix tested latest stable — a guarantee the repo appeared to offer and never provided. With stable now at 1.97.1, where-C aris a hard error, that misreading was about to matter.The fix
rust-setupnow parses thechannelfromrust-toolchain.tomland installs that. Thetoolchaininput still wins when set, but now means "install something deliberately different from the project pin".rust-toolchain.tomlaborts the job underset -euo pipefailwith a::error::. Both branches verified locally — real file resolves1.96.0, channel-less file aborts.with:keys, so no empty mappings).toolchain:version literal under.github/.No compile changes anywhere — every job built on 1.96.0 before and after. What changes is that the declaration matches reality, one
rust-stddownload per job is saved, and a future call site cannot silently drift onto another channel.Nightly audit
Swept the whole tree. Nightly survives in exactly two places, neither a build/test/lint/docs/release/packaging gate:
fuzz/cargo fuzzis a hard requirement — libFuzzer's sanitizer flags are nightly-only. Not run in CI.crates/rustynes-monetization/uniffi-bindgen.rsNeither can move to stable without breaking the tool, so both are documented rather than quietly worked around. Every other
nightlymatch in the tree is prose (cron wording in testing docs, release-channel wording in ADR 0003, historical narrative in CHANGELOG / AGENTS / the.gitlab-ci.ymlcomment explaining the removal).docs/build-and-tooling.mdclaimed "Channel: stable" — corrected to the pinned 1.96.0, with the single-source rule, the pointer to the-C arwarning guarding a 1.97+ bump, and both nightly exceptions stated explicitly.🤖 Generated with Claude Code
Summary by CodeRabbit
CI & Tooling
Documentation