Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 119 additions & 0 deletions .github/workflows/android.yml
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,129 @@ jobs:
done
exit "$fail"

# The RELEASE build path, and it is deliberately UNSIGNED.
#
# Signing material is the project owner's to provision and this workflow does not hold it, so
# a *signed* release build is out of reach here. An unsigned one is not: `assembleRelease`
# still runs R8, resource shrinking and the release manifest merge, which is where
# release-only breakage actually lives -- a missing keep rule strips a class the UniFFI
# bindings reach reflectively, and the debug build never notices because it does not minify.
#
# `isMinifyEnabled` is `false` in `app/build.gradle.kts` today, so this currently proves the
# release variant assembles at all. It is wired now rather than when minification is turned
# on, because the moment it is turned on this step is what catches the fallout.
- name: Assemble the release APK (unsigned)
working-directory: android
env:
RUSTFLAGS: "-C link-arg=-Wl,-z,max-page-size=16384"
run: ./gradlew --no-daemon assembleRelease

# The same 16 KB gate as the debug APK's, on the release variant. Not redundant: the release
# variant has its own packaging and its own shrinking, so alignment has to be proven on the
# artifact that would actually ship, not inferred from the one that would not.
- name: Assert 16 KB page alignment inside the RELEASE APK
run: |
set -euo pipefail
apk=$(find android/app/build/outputs/apk/release -name '*.apk' | head -1)
test -n "$apk" || { echo "no release APK was produced"; exit 1; }
work=$(mktemp -d)
unzip -q "$apk" 'lib/*' -d "$work"
fail=0
for so in "$work"/lib/arm64-v8a/*.so "$work"/lib/x86_64/*.so; do
[ -e "$so" ] || continue
align=$(readelf -lW "$so" | awk '$1 == "LOAD" { print $NF; exit }')
case "$align" in
0x4000|0x10000) echo "OK $(basename "$so") $align" ;;
*) echo "FAIL $(basename "$so") $align (need 16 KB or larger)"; fail=1 ;;
esac
done
exit "$fail"
Comment on lines +279 to +319
Comment thread
coderabbitai[bot] marked this conversation as resolved.

- name: Upload the APK
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7
with:
name: rustysnes-debug-apk
path: android/app/build/outputs/apk/debug/*.apk
if-no-files-found: error
retention-days: 14

# The UniFFI RUNTIME smoke test, in its own job on purpose.
#
# `build` above proves the bindings COMPILE -- `MainActivity` calls `MobileCore` directly, so
# bindgen output that drifted from the Rust API fails the Kotlin compile there. What no build can
# prove is that `System.loadLibrary` finds the `.so` for the device's ABI, that JNA's mapping
# matches the symbols in it, and that a call marshals across and returns. This project has already
# shipped one native Android crash that a build could not have caught.
#
# A separate job, not another step in `build`: an emulator is the flakiest thing in this workflow,
# and a flaky step inside `build` would put the 16 KB alignment gates -- which are not flaky, and
# which gate a real Play requirement -- behind an AVD boot.
#
# The emulator runs x86_64, but that is NOT the set of Rust targets this job needs. Gradle's
# `cargoNdkBuild` builds every ABI in `app/build.gradle.kts`'s `cargoAbis` map -- arm64-v8a AND
# x86_64 -- before the instrumented test can install anything, so both targets must be installed
# or the task fails on `can't find crate for core`. Installing only x86_64 because "the emulator
# runs the host ABI" confuses what the emulator RUNS with what the build COMPILES; that is
# exactly how this job failed on its first run.
smoke:
runs-on: ubuntu-latest
env:
CARGO_NET_RETRY: "10"
CARGO_TERM_COLOR: always
steps:
Comment on lines +347 to +352

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check ci-success aggregation and permissions blocks for the android workflow.
set -euo pipefail

echo "=== android.yml: top-of-file permissions and job list ==="
sed -n '1,40p' .github/workflows/android.yml

echo
echo "=== every job defined in android.yml ==="
rg -nP '^  [a-zA-Z0-9_-]+:\s*$' .github/workflows/android.yml

echo
echo "=== any ci-success aggregator across all workflows ==="
fd -e yml -e yaml . .github/workflows --exec rg -n -A 20 'ci-success' {} \;

echo
echo "=== permissions blocks per workflow ==="
fd -e yml -e yaml . .github/workflows --exec sh -c 'echo "--- $1"; rg -n -A 3 "permissions:" "$1" || echo "(none)"' _ {} \;

Repository: doublegate/RustySNES

Length of output: 2084


🏁 Script executed:

#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re

path = Path(".github/workflows/android.yml")
text = path.read_text()

top_permissions = re.search(
    r"^permissions:\s*\n(?P<body>(?:^[ \t]+.*\n?)*)",
    text,
    re.MULTILINE,
)
print("workflow_permissions:")
print(top_permissions.group("body").rstrip() if top_permissions else "(none)")

jobs = {}
job_matches = list(re.finditer(r"^  ([A-Za-z0-9_-]+):\s*$", text, re.MULTILINE))
for i, match in enumerate(job_matches):
    name = match.group(1)
    start = match.end()
    end = job_matches[i + 1].start() if i + 1 < len(job_matches) else len(text)
    block = text[start:end]
    if name in {"smoke", "ci-success"}:
        needs = re.search(r"^    needs:\s*(.+)$", block, re.MULTILINE)
        job_name = re.search(r"^    name:\s*(.+)$", block, re.MULTILINE)
        job_permissions = re.search(r"^    permissions:\s*$([\s\S]*?)(?=^    [A-Za-z_-]+:|\Z)", block, re.MULTILINE)
        jobs[name] = {
            "needs": needs.group(1).strip() if needs else "(none)",
            "name": job_name.group(1).strip() if job_name else "(none)",
            "permissions": job_permissions.group(0).strip() if job_permissions else "(none)",
        }

print("selected_jobs:")
for name, data in jobs.items():
    print(name, data)

ci = jobs.get("ci-success", {})
needs_text = ci.get("needs", "")
print("smoke_in_ci_success_needs:", bool(re.search(r"\bsmoke\b", needs_text)))
PY

Repository: doublegate/RustySNES

Length of output: 314


Add smoke to ci-success.needs. The workflow grants contents: read, but ci-success omits smoke; a failing smoke test can merge without failing the required check. Add a name to smoke for readable check output.

🧰 Tools
🪛 zizmor (1.28.0)

[info] 318-318: workflow or action definition without a name (anonymous-definition): this job

(anonymous-definition)

🤖 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 @.github/workflows/android.yml around lines 318 - 323, Update the smoke job
in the Android workflow to include a descriptive name, then add smoke to the
ci-success job’s needs list so smoke failures block the aggregate required
check.

Sources: Path instructions, Linters/SAST tools

- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
persist-credentials: false

- uses: ./.github/actions/rust-setup

- name: Add the Android targets Gradle's cargoNdkBuild needs
run: rustup target add x86_64-linux-android aarch64-linux-android

# The same NDK and the same discovery as the `build` job, deliberately: `sdkmanager` is NOT
# on PATH on `ubuntu-latest` even though `$ANDROID_HOME` is set, which that job found the
# hard way on its first run. Two jobs building the same libraries with different NDKs would
# also make a divergence between them impossible to attribute.
- name: Install the NDK from the runner's Android SDK
run: |
set -euo pipefail
: "${ANDROID_HOME:?the runner image is expected to provide an Android SDK}"
sdk="$ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager"
if [ ! -x "$sdk" ]; then
echo "::error::no sdkmanager at $sdk"
ls -la "$ANDROID_HOME/cmdline-tools" || true
exit 1
fi
"$sdk" --install "ndk;27.2.12479018"
echo "ANDROID_NDK_HOME=$ANDROID_HOME/ndk/27.2.12479018" >> "$GITHUB_ENV"

- name: Install cargo-ndk
run: cargo install cargo-ndk --locked --version ^3
Comment on lines +362 to +380

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the NDK setup into a composite action to enforce the invariant the comment states.

Lines 337-351 duplicate lines 70-86 verbatim, including the NDK version 27.2.12479018. The comment at lines 333-336 states that the two jobs must build with the same NDK and that a divergence would be impossible to attribute. A copied literal is the mechanism most likely to produce exactly that divergence: a version bump applied to one job and not the other passes review and CI.

The repository already uses this pattern with ./.github/actions/rust-setup. Move the NDK install and the cargo-ndk install into a single composite action and call it from both jobs. The version then exists in one place.

Proposed structure

New file .github/actions/android-ndk-setup/action.yml:

name: Android NDK setup
description: Installs the pinned NDK from the runner's Android SDK and cargo-ndk.
runs:
  using: composite
  steps:
    - name: Install the NDK from the runner's Android SDK
      shell: bash
      run: |
        set -euo pipefail
        # `sdkmanager` is NOT on PATH on `ubuntu-latest` even though `$ANDROID_HOME` is set.
        : "${ANDROID_HOME:?the runner image is expected to provide an Android SDK}"
        sdk="$ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager"
        if [ ! -x "$sdk" ]; then
          echo "::error::no sdkmanager at $sdk"
          ls -la "$ANDROID_HOME/cmdline-tools" || true
          exit 1
        fi
        "$sdk" --install "ndk;27.2.12479018"
        echo "ANDROID_NDK_HOME=$ANDROID_HOME/ndk/27.2.12479018" >> "$GITHUB_ENV"

    - name: Install cargo-ndk
      shell: bash
      run: cargo install cargo-ndk --locked --version ^3

Then in .github/workflows/android.yml, replace both copies:

-      # The same NDK and the same discovery as the `build` job, deliberately: `sdkmanager` is NOT
-      # on PATH on `ubuntu-latest` even though `$ANDROID_HOME` is set, which that job found the
-      # hard way on its first run. Two jobs building the same libraries with different NDKs would
-      # also make a divergence between them impossible to attribute.
-      - name: Install the NDK from the runner's Android SDK
-        run: |
-          set -euo pipefail
-          : "${ANDROID_HOME:?the runner image is expected to provide an Android SDK}"
-          sdk="$ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager"
-          if [ ! -x "$sdk" ]; then
-            echo "::error::no sdkmanager at $sdk"
-            ls -la "$ANDROID_HOME/cmdline-tools" || true
-            exit 1
-          fi
-          "$sdk" --install "ndk;27.2.12479018"
-          echo "ANDROID_NDK_HOME=$ANDROID_HOME/ndk/27.2.12479018" >> "$GITHUB_ENV"
-
-      - name: Install cargo-ndk
-        run: cargo install cargo-ndk --locked --version ^3
+      # Shared with the `build` job so both jobs cannot drift onto different NDKs.
+      - uses: ./.github/actions/android-ndk-setup
🤖 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 @.github/workflows/android.yml around lines 333 - 351, Extract the duplicated
NDK and cargo-ndk installation steps from both Android workflow jobs into a new
composite action at .github/actions/android-ndk-setup/action.yml, keeping the
pinned NDK version and setup behavior centralized there. Replace both inline
copies in android.yml with uses of this composite action, preserving the
existing job behavior and ensuring both jobs share one version source.


- uses: actions/setup-java@c5195efecf7bdfc987ee8bae7a71cb8b11521c00 # v5
with:
distribution: temurin
java-version: "17"

# KVM has to be enabled explicitly on GitHub's Linux runners, or the AVD falls back to
# software rendering and the boot times out rather than failing with a clear reason.
- name: Enable KVM
run: |
echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules
sudo udevadm control --reload-rules
sudo udevadm trigger --name-match=kvm

# `RUSTFLAGS` for the same reason the `build` job sets it on its Gradle step: Gradle's
# `cargoNdkBuild` re-runs `cargo ndk` in its own process and inherits this environment, not
# the flags of any earlier step.
- name: Run the instrumented UniFFI smoke test
uses: reactivecircus/android-emulator-runner@a421e43855164a8197daf9d8d40fe71c6996bb0d # v2.38.0
env:
RUSTFLAGS: "-C link-arg=-Wl,-z,max-page-size=16384"
with:
api-level: 34
arch: x86_64
target: google_apis
disable-animations: true
working-directory: android
script: ./gradlew --no-daemon connectedDebugAndroidTest
Comment thread
coderabbitai[bot] marked this conversation as resolved.
36 changes: 36 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,42 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- **`v1.30.0` mobile store-readiness: the App Store §4.7 self-audit, a release build path, and a
UniFFI runtime smoke test — plus a correction to four stale claims in the readiness doc.**

**The §4.7 self-audit** (`docs/app-store-4-7-self-audit.md`) is the item `docs/mobile-readiness.md`
recorded as outstanding, and it is the only one of the store-facing items that is *not*
maintainer-blocked. It passes on all five criteria, and the strongest evidence is capability rather
than intent: **Android declares no permissions at all — not even `INTERNET`** — and iOS has no
networking code, so neither shell *can* obtain game software. Every user-visible string in both
shells was enumerated; the complete set is `RustySNES`, `Open ROM`, `Save State`, `Load State`. Two
re-audit triggers are recorded: the peripheral UI when it lands (Super Scope / Mouse / Multitap
names are a fresh trademark decision, and the audit does not pre-approve them) and
`rustysnes-monetization` if it is ever activated.

**An unsigned `assembleRelease` path**, with its own 16 KB alignment gate on the release APK.
Signing material is the maintainer's to provision, so a *signed* release build stays out of reach —
but an unsigned one still runs R8, resource shrinking and the release manifest merge, which is
where release-only breakage lives. `isMinifyEnabled` is `false` today, so this currently proves the
release variant assembles; it is wired now because the moment minification is enabled, this is what
catches the fallout.

**An instrumented UniFFI smoke test** (`android/app/src/androidTest`), in its own CI job.
`assembleDebug` already proves the bindings *compile* — `MainActivity` calls `MobileCore` directly.
What no build can prove is that `System.loadLibrary` finds the `.so` for the device's ABI, that
JNA's mapping matches its symbols, and that a call marshals across and returns. This project has
already shipped one native Android crash a build could not have caught. It is a separate job
because an emulator is the flakiest thing in that workflow, and a flaky step inside `build` would
put the 16 KB gates — which are not flaky and do gate a real Play requirement — behind an AVD boot.

**Four entries in the readiness doc's deferred list had gone stale** and are now marked DONE rather
than deleted, because a readiness document that silently drops items cannot be audited backwards:
`android.yml` exists and gates alignment twice, the `./gradlew` wrapper is committed, `ios.yml`
boots a simulator and requires the app to survive the launch, and the §4.7 audit is done. What
remains genuinely outstanding is stated as such — distribution signing, TestFlight, and Play's Data
Safety form, all maintainer-blocked. **Mobile Phase 6 stays NOT GREENLIT**; passing this audit
removes a prerequisite from that gate's checklist, it does not move the gate.

- **`A6.15` — every 65C816 opcode is defined, and only `STP` hangs. Coverage 361 of 443.** The row
executes each of the 241 straight-line opcodes in a WRAM sandbox and counts three outcomes against
the length **Table 5-4 of the WDC W65C816S datasheet** documents: returned where it should,
Expand Down
13 changes: 13 additions & 0 deletions android/app/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ android {
targetSdk = 34
versionCode = 1
versionName = "1.18.0"
testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"
}

buildTypes {
Expand Down Expand Up @@ -47,6 +48,12 @@ android {
getByName("main") {
jniLibs.srcDirs("src/main/jniLibs")
}
// The instrumented UniFFI smoke test needs the same generated bindings the app uses --
// `androidTest` compiles as its own variant and does not inherit `main`'s generated
// sources automatically.
getByName("androidTest") {
kotlin.srcDirs("src/androidTest/kotlin")
}
}
}

Expand Down Expand Up @@ -130,6 +137,12 @@ tasks.named("preBuild") {
}

dependencies {
// The instrumented UniFFI smoke test (`src/androidTest`). It proves the generated bindings
// LOAD and CALL on a device, which a build cannot: `assembleDebug` already proves they
// compile, because `MainActivity` calls `MobileCore` directly.
androidTestImplementation("androidx.test.ext:junit:1.2.1")
androidTestImplementation("androidx.test:runner:1.6.2")

implementation("androidx.core:core-ktx:1.15.0")
implementation("androidx.activity:activity-compose:1.9.3")
implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.8.7")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package com.doublegate.rustysnes

import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import uniffi.rustysnes_mobile.MobileCore
import uniffi.rustysnes_mobile.MobileRegion

/**
* The UniFFI smoke test: proves the generated Kotlin bindings actually **load and call** the native
* library on a real Android runtime.
*
* `assembleDebug` already proves the bindings *compile* against the shell — `MainActivity` calls
* `MobileCore` directly, so a bindgen output that drifted from the Rust API fails the Kotlin
* compile. What a build cannot prove is that `System.loadLibrary` finds the `.so` for the device's
* ABI, that JNA's mapping matches the symbols in it, and that a call marshals across and returns.
* Those are runtime facts, and this project has already shipped one native Android crash that a
* build could not have caught.
*
* Deliberately ROM-free. The app takes ROMs only from the user's document picker
* (`docs/app-store-4-7-self-audit.md`), so there is no ROM to open here and no need for one: every
* assertion below is about the *bridge*, not about emulation, which the workspace's own test suite
* covers far better than an emulator can.
*/
@RunWith(AndroidJUnit4::class)
class MobileCoreSmokeTest {
/** Constructing the core loads the library and crosses the FFI boundary once. */
@Test
fun the_native_library_loads_and_a_core_can_be_constructed() {
val core = MobileCore(MobileRegion.NTSC)
assertFalse("a freshly constructed core must report no ROM loaded", core.romLoaded())
}

/**
* A frame with no ROM loaded still has to return a correctly sized framebuffer. This is the
* assertion that would catch a marshalling error: a wrong length, or a returned buffer that
* does not survive the crossing, shows up here and nowhere in a build.
*/
@Test
fun a_frame_runs_and_returns_a_framebuffer_of_the_declared_size() {
val core = MobileCore(MobileRegion.NTSC)
core.runFrame()

val size = core.frameSize()
assertTrue("frame width must be positive, got ${size.width}", size.width > 0u)
assertTrue("frame height must be positive, got ${size.height}", size.height > 0u)

val fb = core.framebuffer()
assertEquals(
"the framebuffer length must be width * height * 4 (RGBA8)",
(size.width * size.height * 4u).toInt(),
fb.size,
)
}

/**
* `drainAudio` is documented as non-destructive — it returns the current frame's buffered
* samples rather than popping a FIFO — so calling it twice for one frame returns the same
* count. Pinning that here is what stops the contract drifting under a shell that calls it once
* per `runFrame` and would not notice.
*/
@Test
fun drain_audio_is_non_destructive_within_a_frame() {
val core = MobileCore(MobileRegion.NTSC)
core.runFrame()
val first = core.drainAudio().size
val second = core.drainAudio().size
assertEquals("drainAudio must not consume the buffer", first, second)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/** Reset and power-cycle are the two lifecycle calls the shell makes; both must cross safely. */
@Test
fun the_lifecycle_calls_cross_the_boundary() {
val core = MobileCore(MobileRegion.NTSC)
core.reset()
core.powerCycle()
assertFalse("no ROM was ever loaded", core.romLoaded())
}
}
Loading
Loading