Skip to content

fix(ci): make all ten libretro buildbot jobs build (pipeline #91899)#317

Merged
doublegate merged 6 commits into
mainfrom
fix/libretro-buildbot-cross-targets
Jul 21, 2026
Merged

fix(ci): make all ten libretro buildbot jobs build (pipeline #91899)#317
doublegate merged 6 commits into
mainfrom
fix/libretro-buildbot-cross-targets

Conversation

@doublegate

@doublegate doublegate commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Summary

The first real run of our .gitlab-ci.yml recipe on libretro's GitLab buildbot (pipeline #91899) passed 1 of 10 jobs. This fixes all three underlying defects and adds a CI guard so the class can't reach a third-party buildbot again.

All three are on our side — the build images are fine. Both reference Rust cores (doukutsu-rs-libretro, holani-retro) build the identical 10-job platform set green against the identical image; neither pins a toolchain or declares a high MSRV, which is precisely the difference.

Zero Rust source is touched, so AccuracyCoin holds 141/141 and every golden vector is unaffected by construction.

Root cause A — missing cross-compile targets (8 jobs)

rust-toolchain.toml pins channel = "1.96.0". The image pre-provisions every libretro cross target on its own default toolchain, but our pin makes rustup install a separate pristine 1.96.0 in the container — carrying only the host rust-std plus the two targets that file declares (thumbv7em, wasm32). Every job whose target wasn't the host triple died:

error[E0463]: can't find crate for `core`
  = note: the `x86_64-pc-windows-gnu` target may not be installed

libretro-build-linux-x64 passed only because its target is the host triple. Same trap rust-toolchain.toml's own comment already documents for the thumbv7em/wasm32 GH Actions jobs, arriving from the other direction.

Each job now declares RUST_TARGET and runs rustup target add ${RUST_TARGET}.

Not solved by appending the eight triples to rust-toolchain.toml's targets = [...] — one line, but it bills every contributor and every GH Actions job for ~8 extra rust-std downloads per toolchain install to serve one downstream CI.

The four Apple jobs compose via !reference, not extends: their templates already define before_script (SDKROOT / STRIP / CC / CXX) and GitLab's extends replaces array keys rather than merging, so extending a shared before_script would have silently dropped the SDK and cross-compiler environment — trading a loud compile error for a quiet mislink.

Root cause B — upstream rust-libretro MinGW ABI bug (Windows)

Masked behind (A) and never reached by any CI. rust-libretro 0.3.2 casts a keycode to i32 under cfg(target_family = "windows"). That cfg is too broad: C enum signedness follows the ABI, not the OS family. Only MSVC gives plain enums int. Verified, one generated bindings file per target:

target retro_key result
x86_64-pc-windows-msvc c_int compiles
x86_64-pc-windows-gnu c_uint E0308
aarch64-apple-ios c_uint cast not taken
armv7-linux-androideabi c_uint cast not taken

MinGW is the only casualty — and it's exactly what the buildbot's Windows job uses. The correct fix is target_env = "msvc" upstream, but max-m/rust-libretro has had no commit since 2023-02 and 0.3.2 is its newest release.

.cargo/config.toml points bindgen's clang at the MSVC triple for that one target. Surgical: diffing the two generated bindings_libretro.rs files gives 28 changed lines, all enum signedness (#[repr(u32)]#[repr(i32)] plus the retro_key/retro_mod newtype fields). No struct layout, signature, or type size moves — MinGW and MSVC share the Windows platform ABI (LLP64, same record layout), and every libretro enum value is non-negative and fits in int.

Root cause C — tvOS MSRV, then panic_abort

The tvOS template overrides script with cargo +nightly build -Zbuild-std …. That +nightly outranks both rust-toolchain.toml and RUSTUP_TOOLCHAIN, so it's the one job not on our pinned 1.96.0 — and the image's nightly is 1.94.0-nightly, below rust-version = "1.96". It failed cargo's MSRV gate before compiling anything. The job now refreshes the nightly channel.

That exposed a second blocker: bare -Zbuild-std builds std but not panic_abort, which [profile.release] panic = "abort" requires → E0463: can't find crate for panic_abort. -Zbuild-std=std,panic_abort isn't available (flag is hardcoded in the shared template), and CARGO_UNSTABLE_BUILD_STD does not help — verified: the CLI -Z flag wins and the crate list stays default. The job exports CARGO_PROFILE_RELEASE_PANIC=unwind, the same CARGO_PROFILE_RELEASE_* mechanism the template itself already uses for CARGO_PROFILE_RELEASE_DEBUG.

Safe: since Rust 1.81 a panic reaching an extern "C" boundary aborts rather than unwinding into C frames, so the core behaves identically on panic. Cost is marginally larger code and unwind tables, on tvOS alone; every other platform keeps panic = "abort".

Prevention — new libretro-cross CI job

There was zero libretro coverage in GitHub Actions, which is why three defects reached a buildbot we can't push to, re-run, or debug interactively. New job checks rustynes-libretro against one triple per buildbot ABI family (MinGW-Windows, Android/NDK, Apple-iOS, Apple-macOS) — the remaining buildbot triples are same-ABI siblings, so same coverage at a quarter of the runner cost.

It runs rustup target add rather than the composite action's targets: input on purpose: that input routes through dtolnay/rust-toolchain, which installs onto the channel it pins (stable) while rust-toolchain.toml switches cargo to 1.96.0 — i.e. it would have reproduced root cause A inside our own CI. check, not build, because no Apple/Windows target links on a Linux runner without its SDK and all three defects were compile-time. Wired into ci-success.

Also: .pre-commit-config.yaml gets a second check-yaml pass with --unsafe scoped to .gitlab-ci.yml, since PyYAML's safe_load can't construct GitLab's !reference tag. Syntax and duplicate keys are still validated — only tag construction is skipped (mirroring the existing mkdocs.yml precedent, but without giving up the check entirely).

Verification

  • All 8 non-tvOS buildbot triples pass cargo check --release -p rustynes-libretro --target <triple>.
  • cargo build (full codegen, which check skips) additionally reaches the linker for aarch64-apple-ios, aarch64-linux-android, and x86_64-pc-windows-gnu — failing only on this host's absent platform SDKs / mingw linker, proving codegen including panic_abort resolution is complete.
  • tvOS reproduced with the template's exact command on a modern nightly: compiles std, all six workspace crates, and rustynes-libretro, then stops at the tvOS SDK the buildbot image supplies.
  • cargo fmt --check, cargo clippy --workspace --all-targets -D warnings, the standalone no_std cross-compile, and markdownlint are all green.

Honest caveat: linking is the one step not reproducible here (no Apple SDKs, no mingw linker locally), so the buildbot remains the authority on that. Every failure in #91899 was compile-time, and those are all reproduced-and-fixed locally.

Addresses #311 — deliberately not a closing keyword. The mirror + buildbot-enable step is on the libretro team's side, and hizzlekizzle closes the issue once he's verified a clean pipeline.

Summary by CodeRabbit

  • Bug Fixes
    • Improved libretro cross-compilation reliability, including Windows MinGW ABI/signing compatibility and more consistent enum handling.
    • Stabilized tvOS builds with updated toolchain/MSRV and safer release panic behavior.
  • CI Improvements
    • Added a GitHub Actions cross-check job covering additional target triples and updated the required CI gate.
    • Updated GitLab CI to install the right Rust targets per job, with refined Apple/tvOS setup.
  • Documentation
    • Expanded operating notes for the libretro build pipeline, plus updated the Unreleased changelog and cross-compilation guidance in the Rust toolchain file.
  • Chores
    • Improved GitLab CI YAML validation in pre-commit checks.

The first real run of the v2.2.1 `.gitlab-ci.yml` recipe on libretro's
GitLab buildbot passed 1 of 10 jobs. Three independent defects, all on
our side — the build images are fine, and both reference Rust cores
(doukutsu-rs-libretro, holani-retro) build the identical 10-job platform
set green against the identical image.

Root cause A — missing cross-compile targets (8 jobs)
-----------------------------------------------------
`rust-toolchain.toml` pins `channel = "1.96.0"`. The build image
pre-provisions every libretro cross target on ITS OWN default toolchain,
but our pin makes rustup download and install a separate, pristine 1.96.0
inside the container, and that toolchain carries only the host `rust-std`
plus the two targets `rust-toolchain.toml` declares (thumbv7em, wasm32).
Every job whose `TARGET_ARCH` was not the host triple therefore had no
`core`/`std` to compile against:

    error[E0463]: can't find crate for `core`
      = note: the `x86_64-pc-windows-gnu` target may not be installed
    ...
    mv: cannot stat 'target/.../rustynes.dll': No such file or directory

`libretro-build-linux-x64` passed only because its target IS the host
triple. This is the same trap `rust-toolchain.toml`'s own comment block
already documents for the thumbv7em/wasm32 GitHub Actions jobs, arriving
from the opposite direction.

Each job now declares a `RUST_TARGET` and runs `rustup target add
${RUST_TARGET}`. `before_script` executes inside the checkout, so rustup
resolves `rust-toolchain.toml` and installs the target onto the 1.96.0
toolchain cargo actually uses. Deliberately NOT solved by appending the
eight triples to `rust-toolchain.toml`'s `targets = [...]`: that fixes it
in one line but bills every contributor and every GitHub Actions job for
~8 extra `rust-std` downloads on each toolchain install, to serve one
downstream CI. The cost belongs where the need is.

The four Apple jobs compose via `!reference` instead of `extends`.
Their upstream templates already define `before_script` (exporting
SDKROOT / STRIP / CC / CXX), and GitLab's `extends` REPLACES array-valued
keys rather than merging them, so extending a shared `before_script` there
would have silently dropped the SDK and cross-compiler environment —
trading a loud compile error for a quiet mislink.

Root cause B — upstream rust-libretro MinGW ABI bug (Windows)
-------------------------------------------------------------
Masked behind root cause A and never previously reached by any CI.
`rust-libretro 0.3.2` carries a bindgen workaround gated on
`cfg(target_family = "windows")` that casts a keycode to `i32`. That cfg
is too broad: C enum signedness is chosen by the target's ABI, not its OS
family. Only the MSVC ABI gives plain enums `int`; under MinGW — which is
exactly what the buildbot's `x86_64-pc-windows-gnu` job uses — bindgen
emits `pub struct retro_key(pub c_uint)`, and the crate fails E0308.

Verified empirically, one bindings file per target:
    x86_64-pc-windows-msvc  ->  retro_key(c_int)    compiles
    x86_64-pc-windows-gnu   ->  retro_key(c_uint)   E0308
    aarch64-apple-ios       ->  retro_key(c_uint)
    armv7-linux-androideabi ->  retro_key(c_uint)
Non-Windows targets never take the cast, so MinGW is the only casualty.

The correct fix is `target_env = "msvc"` upstream, but
github.com/max-m/rust-libretro has had no commit since 2023-02 and 0.3.2
is its newest release. `.cargo/config.toml` therefore points bindgen's
clang at the MSVC triple for that one target, reproducing the ABI the
upstream cast assumes. Surgical, and verified by diffing the two generated
`bindings_libretro.rs` files: 28 changed lines, ALL of them enum
signedness (`#[repr(u32)]` -> `#[repr(i32)]`, plus the `retro_key` /
`retro_mod` newtype fields). No struct layout, function signature, or type
size moves — MinGW and MSVC share the Windows platform ABI (LLP64, same
record layout), and every libretro enum value is non-negative and fits in
`int`, so i32/u32 are wire-identical here. Cargo's `[env]` reaches build
scripts, so bindgen picks it up for the buildbot, CI, and local builds
alike without each having to remember to export it.

Root cause C — tvOS MSRV, then panic_abort (tvOS)
--------------------------------------------------
The tvOS template overrides `script` with
`cargo +nightly build -Zbuild-std --target aarch64-apple-tvos --release`.
That explicit `+nightly` outranks both `rust-toolchain.toml` and
`RUSTUP_TOOLCHAIN`, so this is the one job that does not run on our pinned
1.96.0 — and the image's pre-installed nightly is 1.94.0-nightly, below
the workspace's `rust-version = "1.96"`. It failed cargo's MSRV gate
before compiling a line. The job now refreshes the nightly channel
(uninstall-then-install, so a stale component unpublished for the new
nightly date cannot abort an in-place update; `rust-src` is required by
`-Zbuild-std`).

Refreshing nightly exposed a second, deeper blocker: a bare `-Zbuild-std`
builds `std` but not `panic_abort`, which `[profile.release] panic =
"abort"` requires, so the cdylib fails with `E0463: can't find crate for
panic_abort`. The usual `-Zbuild-std=std,panic_abort` is unavailable (the
flag is hardcoded in the centrally-maintained template), and
`CARGO_UNSTABLE_BUILD_STD` does not help — verified: the CLI `-Z` flag
wins over the config/env form and the crate list stays the default. The
job therefore exports `CARGO_PROFILE_RELEASE_PANIC=unwind`, the same
`CARGO_PROFILE_RELEASE_*` mechanism the template itself already uses for
`CARGO_PROFILE_RELEASE_DEBUG`. Safe: since Rust 1.81 a panic reaching an
`extern "C"` boundary aborts rather than unwinding into C frames, so the
core behaves identically on panic; the cost is marginally larger code and
unwind tables, on tvOS alone. Every other platform keeps `panic = "abort"`.

Prevention — a libretro-cross CI job
-------------------------------------
There was ZERO libretro coverage in GitHub Actions, which is why three
defects reached a third-party buildbot we cannot push to, re-run, or debug
interactively. `.github/workflows/ci.yml` gains `libretro-cross`, checking
`rustynes-libretro` against one triple per buildbot ABI family
(MinGW-Windows, Android/NDK, Apple-iOS, Apple-macOS); the remaining
buildbot triples are same-ABI siblings, so this catches the same class of
breakage at a quarter of the runner cost. It deliberately runs `rustup
target add` rather than the composite action's `targets:` input — that
input routes through `dtolnay/rust-toolchain`, which installs onto the
channel IT pins (stable) while `rust-toolchain.toml` switches cargo to
1.96.0, i.e. it would have reproduced root cause A inside our own CI. The
job is `cargo check`, not `build`: no Apple/Windows target can link on a
Linux runner without its platform SDK, and all three defects were
compile-time. Wired into the `ci-success` aggregate gate.

Verification
------------
All eight non-tvOS buildbot triples now pass `cargo check --release -p
rustynes-libretro --target <triple>` locally. `cargo build` (full codegen,
which `check` skips) additionally reaches the LINKER for aarch64-apple-ios,
aarch64-linux-android, and x86_64-pc-windows-gnu, failing only on this
Linux host's absent platform SDKs / mingw linker — proving codegen,
including `panic_abort` resolution, is complete. The tvOS path was
reproduced with the template's exact command on a modern nightly: it
compiles std, all six workspace crates, and `rustynes-libretro`, then
stops at the tvOS SDK the buildbot image supplies. `cargo fmt --check`,
`cargo clippy --workspace --all-targets -D warnings`, the standalone
no_std cross-compile, and markdownlint are green.

Zero emulation-core changes: no Rust source is touched, so AccuracyCoin
holds 141/141 and every golden vector is unaffected by construction.

Addresses #311 (deliberately not a closing keyword — the mirror and
buildbot-enable step remain on the libretro team's side).
Copilot AI review requested due to automatic review settings July 21, 2026 01:06
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9abfd495-9e06-40f5-af9a-c69c0eae9375

📥 Commits

Reviewing files that changed from the base of the PR and between 19aaac3 and dde2be5.

📒 Files selected for processing (7)
  • .cargo/config.toml
  • .github/workflows/ci.yml
  • .gitlab-ci.yml
  • .pre-commit-config.yaml
  • AGENTS.md
  • CHANGELOG.md
  • rust-toolchain.toml

Disabled knowledge base sources:

  • Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.


📝 Walkthrough

Walkthrough

The changes configure a Windows MinGW bindgen workaround, install Rust targets in libretro buildbot jobs, add GitHub cross-target checks, adjust YAML validation for GitLab syntax, and document the resulting CI and toolchain behavior.

Changes

Libretro CI and toolchain support

Layer / File(s) Summary
Windows bindgen and toolchain guidance
.cargo/config.toml, rust-toolchain.toml
Configures MSVC-targeted clang arguments for Windows GNU targets and documents cross-target handling with the pinned Rust toolchain.
Buildbot target installation and platform setup
.gitlab-ci.yml, .pre-commit-config.yaml
Installs platform-specific Rust targets across buildbot jobs, applies tvOS nightly and panic settings, and validates GitLab YAML using unsafe parser mode.
GitHub cross-target validation
.github/workflows/ci.yml
Adds release-mode checks for Windows GNU and Android libretro targets and makes the final CI gate depend on the new job.
Operational documentation and release notes
AGENTS.md, CHANGELOG.md
Documents libretro buildbot behavior, toolchain constraints, review procedures, feature requirements, and completed CI fixes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GitHubActions
  participant Rustup
  participant Bindgen
  participant Cargo
  GitHubActions->>Rustup: Install matrix target
  GitHubActions->>Bindgen: Configure Android NDK or Windows MSVC clang arguments
  GitHubActions->>Cargo: Run release cargo check for rustynes-libretro
  Cargo-->>GitHubActions: Return target validation result
  GitHubActions->>GitHubActions: Include libretro-cross in ci-success
Loading
🚥 Pre-merge checks | ✅ 9
✅ Passed checks (9 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing CI so all ten libretro buildbot jobs build successfully.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Docs-As-Spec Sync ✅ Passed Diff only changes CHANGELOG.md; no files under crates/rustynes-{cpu,ppu,apu,mappers} or matching docs/.md were touched.
Changelog Entry For User-Visible Changes ✅ Passed CHANGELOG.md has an [Unreleased] → Fixed entry covering the libretro buildbot CI, MinGW bindgen workaround, tvOS/nightly fix, and new cross-check job.
No Unwrap/Expect/Panic On Untrusted Input ✅ Passed Only CHANGELOG.md changed; the patch adds no code and contains no new unwrap(), expect(), or panic! on untrusted input.
Safety Comment On New Unsafe Blocks ✅ Passed PR diff touches only docs/config files; no Rust source, new unsafe {} blocks, unsafe fn, or // SAFETY: additions were introduced.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/libretro-buildbot-cross-targets

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens RustyNES’s libretro buildbot CI integration by addressing the compile-time failures seen on libretro’s GitLab buildbot (pipeline #91899), and by adding an internal GitHub Actions cross-target “early warning” job to prevent similar breakage from reaching third-party CI again.

Changes:

  • Add per-job rustup target add in .gitlab-ci.yml to ensure the pinned toolchain has the required cross targets.
  • Add a Cargo/bindgen workaround for x86_64-pc-windows-gnu to sidestep the upstream rust-libretro MinGW enum-signedness issue.
  • Add a GitHub Actions libretro-cross job plus pre-commit YAML validation adjustments for GitLab’s !reference tags.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
rust-toolchain.toml Documents why buildbot cross targets are installed per-job rather than pinned globally in the toolchain file.
CHANGELOG.md Records the buildbot pipeline fix and the new internal CI guard under Unreleased.
AGENTS.md Captures operational “gotchas” and conventions around libretro buildbot CI and related tooling.
.pre-commit-config.yaml Adds a dedicated check-yaml --unsafe pass for .gitlab-ci.yml while retaining syntax checking.
.gitlab-ci.yml Adds a shared rustup target add step (and Apple !reference composition) + tvOS nightly/panic handling notes.
.github/workflows/ci.yml Introduces the libretro-cross CI job and wires it into the ci-success gate.
.cargo/config.toml Adds a target-scoped bindgen clang target override for MinGW Windows builds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/ci.yml:
- Line 332: Update the actions/checkout step to disable credential persistence
while compiling PR code by setting its persist-credentials option to false. Do
not alter unrelated checkout behavior or enable authentication unless a later
workflow step explicitly requires it.
🪄 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: 44e751c9-05f8-425f-9cba-3a5c30fdfe24

📥 Commits

Reviewing files that changed from the base of the PR and between 19aaac3 and c0e3fc2.

📒 Files selected for processing (7)
  • .cargo/config.toml
  • .github/workflows/ci.yml
  • .gitlab-ci.yml
  • .pre-commit-config.yaml
  • AGENTS.md
  • CHANGELOG.md
  • rust-toolchain.toml

Comment thread .github/workflows/ci.yml
The Apple legs failed: bindgen needs a per-target sysroot to parse
libretro.h, and there is no Apple SDK on a Linux runner, so clang falls
back to host glibc headers and dies on Debian multiarch:

    /usr/include/stdint.h:26:10: fatal error:
    'bits/libc-header-start.h' file not found

(It passed locally only because Arch keeps glibc headers in a flat
/usr/include/bits rather than a multiarch subdir. x86_64-pc-windows-gnu
passed on the runner because it pulls in no glibc at all.)

Feeding clang /usr/include/x86_64-linux-gnu would silence it, but would
then generate Apple bindings from Linux headers — a lookalike rather
than a rehearsal, which is the opposite of this job's purpose. Doing it
honestly needs a macOS runner, which this repo reserves for full
(non-PR) runs per the setup-driven OS matrix on the test job.

Android stays, now with the NDK sysroot wired up the same way cargo ndk
does it on the buildbot ($ANDROID_NDK_LATEST_HOME, matching
android.yml). Windows stays and is the leg that matters most: it is the
one that catches the MinGW enum-ABI class of bug this PR fixes. Both
remaining legs share the crate's entire dependency graph with the
dropped ones, so the marginal coverage lost is small.
Two review findings on PR #317, both adopted:

CodeRabbit (security): `cargo check` executes build scripts and proc
macros from the checkout — untrusted code on a PR — while
`actions/checkout` leaves the workflow token in .git/config by default.
Nothing in this job needs Git auth afterwards, so persist-credentials is
now false. Note the same exposure exists in every other compiling job in
this workflow (test, lint, no_std, wasm, ...); making that repo-wide is a
separate change, deliberately not folded into a buildbot fix.

Copilot (toolchain): flagged that rust-setup defaults to `stable`, so
this guard might not rehearse the pinned 1.96.0 and the surrounding
comments might be misleading. The premise turned out not to hold — the
action runs `rustup default stable`, which rust-toolchain.toml outranks;
the failed run's own log shows rustup reporting "toolchain '1.96.0' is
currently in use (overridden by .../rust-toolchain.toml)". The job was
always on 1.96.0 and the comments were accurate. Pinned anyway: it is
what the lint job does, it avoids installing a stable toolchain nothing
subsequently uses, and it removes the ambiguity that prompted the
comment in the first place.
@doublegate

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.


Your plan includes PR reviews subject to rate limits. More reviews will be available in 35 seconds.

The workaround landed for x86_64-pc-windows-gnu only, because that is the
triple the GitLab buildbot builds. But this crate's Makefile — the legacy
libretro-super path — also builds i686-pc-windows-gnu for 32-bit Windows,
and BINDGEN_EXTRA_CLANG_ARGS is target-suffixed, so the 64-bit line did
nothing for it. Confirmed by building it: identical E0308 from the same
rust-libretro cfg(target_family = "windows") cast, since 32-bit MinGW
picks the same unsigned enum representation as 64-bit. Now overridden to
the matching i686-pc-windows-msvc triple, and verified to generate
retro_key(c_int) and compile.

Also documents a local-only trap found while verifying this:
rust-libretro-sys's build script does not emit
cargo:rerun-if-env-changed=BINDGEN_EXTRA_CLANG_ARGS*, so editing these
lines appears to have no effect until
target/<triple>/release/build/rust-libretro-sys-* is deleted. CI and the
buildbot always build from a clean tree, so only local testing is
affected — but it is exactly the kind of thing that reads as "the fix
doesn't work".
Repository owner deleted a comment from coderabbitai Bot Jul 21, 2026
@doublegate

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@doublegate doublegate self-assigned this Jul 21, 2026
@doublegate doublegate added the bug Something isn't working label Jul 21, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/ci.yml:
- Line 393: Update the libretro Cargo check matrix around the rustynes-libretro
step to add separately compatible explicit feature-set entries, invoking each
with --no-default-features and the appropriate --features value. Remove reliance
on Cargo defaults, do not use --all-features, and keep script-wasm and native
scripting feature combinations separate.

In `@AGENTS.md`:
- Line 192: The documentation claims must match the GitHub Actions matrix: in
AGENTS.md at lines 192-192, replace “one triple per buildbot ABI family” with
the two Linux-modelable families or explicitly state that Apple is excluded; in
CHANGELOG.md at lines 113-115, remove Apple-iOS and Apple-macOS from the
libretro-cross coverage claim.
🪄 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: f49d7e6c-4028-4e8d-bf84-346701e677ba

📥 Commits

Reviewing files that changed from the base of the PR and between 19aaac3 and afdd327.

📒 Files selected for processing (7)
  • .cargo/config.toml
  • .github/workflows/ci.yml
  • .gitlab-ci.yml
  • .pre-commit-config.yaml
  • AGENTS.md
  • CHANGELOG.md
  • rust-toolchain.toml

Comment thread .github/workflows/ci.yml
Comment thread AGENTS.md Outdated
Both still described the job as covering "one triple per buildbot ABI
family (MinGW-Windows, Android, Apple-iOS, Apple-macOS)", which was
true when first written but stopped being true in e3a9cfc, where the
Apple legs were dropped because bindgen cannot get a real Apple sysroot
on a Linux runner. Caught by CodeRabbit; a straightforward doc-vs-code
drift I introduced by narrowing the matrix without revisiting the prose.

Both now state the two families actually covered and record why Apple is
excluded, so the exclusion reads as deliberate rather than as an
oversight someone should "fix" by adding the legs back.
@doublegate
doublegate merged commit e55e9f2 into main Jul 21, 2026
24 checks passed
@doublegate
doublegate deleted the fix/libretro-buildbot-cross-targets branch July 21, 2026 01:59
doublegate added a commit that referenced this pull request Jul 21, 2026
…#319)

* security(ci): drop persisted Git credentials from compiling checkouts

Closes #318.

actions/checkout defaults to persist-credentials: true, which writes the
workflow GITHUB_TOKEN into .git/config as an
http.https://github.com/.extraheader entry. Any code the job subsequently
executes from the checkout can read it: Cargo build scripts, proc macros,
test binaries, Gradle build scripts, scripts/*.sh, the MkDocs build. On a
pull request that tree is by definition unreviewed code, and every CI job
in this repo compiles and runs it. Raised by CodeRabbit on #317, where the
fix was applied to the one new job; this is the repo-wide sweep.

Applied to 18 of the 19 checkouts under .github/.

Audited rather than applied blanket. Three findings shaped the result:

1. .github/actions/rust-setup performs no checkout of its own (it says so
   at L5-6 and is used by 12 call sites), so hardening at each call site is
   complete coverage — there is no hidden checkout to miss.

2. No job actually needed the persisted credential. There is exactly ONE
   Git network operation in the whole .github/ tree. Nothing pushes commits,
   tags, or branches; there are no submodules; GitHub Pages uses the OIDC
   flow (configure-pages/upload-pages-artifact/deploy-pages) rather than a
   gh-pages branch push, and its deploy job has no checkout at all. The
   usual false positives were each checked and dismissed: `gh release
   create` and softprops/action-gh-release authenticate via GH_TOKEN against
   the REST API, and `fastlane match` clones a DIFFERENT repository using its
   own MATCH_GIT_* credentials. persist-credentials only affects git network
   operations relying on the stored credential — not the gh CLI, not the API.

3. The highest-exposure site is not a ci.yml job. web.yml declares
   pages: write + id-token: write at the WORKFLOW level, so that block also
   covers its `build` job — which is pull-request-reachable and runs trunk,
   cargo doc, pip install, and mkdocs build. That is the most valuable token
   in the repo sitting in the job with the widest untrusted-code surface.

The single deliberate exception is release-auto.yml's `prepare`, which keeps
its credentials and now carries an inline comment explaining why. It holds
the tree's only real Git network operation (git ls-remote --tags origin), it
is unreachable from untrusted input (workflow_run, further gated to
event == 'push'), and it executes no repository code at all — it reads two
text files. Hardening it would buy nothing while risking a release path that
is only exercised at a version cut. The comment also records the safe
migration order for anyone later wanting uniformity: convert the ls-remote to
`gh api .../git/ref/tags/$tag` FIRST, then drop the credentials. That swap
additionally fixes a latent bug, since the current `>/dev/null 2>&1` makes a
transient failure indistinguishable from "tag does not exist" and would
publish a duplicate release.

ci.yml's `changes` job is the one place the change touches the git layer at
all, since dorny/paths-filter reads local git on push events. It needs no
authentication (fetch-depth: 0 means the base commit is already local, and
the pull_request path uses the GitHub API), so this is expected to be inert;
an inline comment records the reasoning and the benign failure mode.

Deliberately NOT included: zizmor's unpinned-uses finding. SHA-pinning every
action is a different risk class and a maintenance commitment that needs the
github-actions Dependabot ecosystem enabled first, or it trades a
supply-chain risk for a stale-action risk. One sub-item is worth doing soon
on its own: .github/actions/rust-setup pins dtolnay/rust-toolchain@master —
a branch ref that advances on every upstream commit, unlike the @vn tags used
everywhere else — and that composite feeds 12 of the 19 checkouts including
release.yml (contents: write) and web.yml (pages: write). Tracked in #318.

Purely additive CI configuration: no source, build output, or emulation
behavior changes.

* docs(ci): tighten the credential-exception wording (review feedback)

Three accuracy corrections from Copilot and CodeRabbit on #319, all
adopted; none change behaviour.

1. "reads two text files" / "Cargo.toml + CHANGELOG.md" undercounted:
   the job also reads the optional .github/release-notes/vX.Y.Z.md
   override (L119). Both the pre-existing comment and mine now describe
   the set without pinning an exact count, so the security rationale
   cannot go stale the next time a file is added.

2. "the only real Git network operation in the whole .github/ tree" was
   too broad. fastlane match in ios.yml also clones over the network — it
   just uses its own MATCH_GIT_* secrets against a different remote, so it
   is unaffected by persist-credentials. Reworded to the claim that
   actually justifies the exception: the only operation needing THIS
   checkout's origin credential. The CHANGELOG had the mirrored problem,
   asserting no job needed the credential and then naming an exception;
   it now says every *hardened* job was confirmed not to need it.

3. The CHANGELOG claimed "every CI job compiles and runs" the checked-out
   tree. The audit and deny jobs do neither — they install prebuilt
   binaries and parse Cargo.lock — and overstating a security claim in
   release notes is exactly what should not happen.

Also corrected a claim in my own comment: a masked ls-remote failure was
described as publishing "a duplicate release". Per gh's documented
behaviour it would instead fail outright when a release already exists for
the tag, or publish against the pre-existing tag when one does not — noisy
or wrong, but not silently duplicated.

Declined in this PR (tracked in #318): converting the ls-remote to a
fail-closed gh api call. It is a real latent bug, but it changes the
release path, cannot be verified without a version cut, and does not
belong in a credential-hardening change. The comment records the safe
migration order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants