From 74270109dd2dc28aa45d315163fad99cb289e560 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 8 Aug 2026 00:01:39 +0200 Subject: [PATCH] ci(android): lint the Android target, which nothing had ever done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ci.yml` runs `cargo clippy --workspace` on the HOST, where `clients/android/native` and every `#[cfg(target_os = "android")]` module elsewhere compile out, and `android.yml` only ever built. So the Android target was never linted at all — not once. Five lints were sitting in clients/android/native when this was noticed, in code no gate had ever read. The gate is a Gradle task rather than a YAML step because cargo-ndk needs a specific discovery environment (NDK sysroot, SDK cmake 3.22.1 for libopus, `LIBOPUS_STATIC`, Ninja) and duplicating it into the workflow would let the lint drift from the build — a lint that ran against a different toolchain is a lint about a different program. `registerCargoNdkClippy` reuses the build task's environment verbatim via the extracted `cargoNdkEnvironment`, so local and CI runs are the same invocation. It lints BOTH pointer widths, and that is load-bearing rather than thorough: arm64-v8a is 64-bit and armeabi-v7a is 32-bit, so a cast that is redundant on one can be required on the other. Linting only the primary ABI would license "fixes" that break the 32-bit build — the shipping ABI for the many 32-bit Google TV / Android TV boxes this client targets. x86_64 is skipped: it is emulator-only and shares its width with arm64, so it costs lint time for no signal the other two do not already carry. The five resident lints: * `audio.rs` / `mic.rs` `type_complexity` — the open-attempt closures now return named `OpenedPlayback` / `OpenedCapture` aliases. The two tuples are mirror images of each other (playback sends, capture receives), which the aliases now say out loud. * `vsync.rs` ×2 `unnecessary_cast` — **not** taken. `timespec`'s fields are 32-bit on armv7 and 64-bit on arm64, so the casts are REQUIRED on one shipping ABI and redundant on the other; following the suggestion would break the 32-bit build. `i64::from`/`.into()` do not escape it either, they trade `unnecessary_cast` for `useless_conversion` on the 64-bit side. Answered with a documented `#[allow]` at the expression instead of in whichever build breaks first. * `pad_audio.rs` `needless_range_loop` — iterator form, preserving the `channels < 2` no-op the range had. Verified: `:kit:cargoNdkClippy` green on both ABIs, host-lane clippy for the crate still clean, `cargo fmt --all --check` clean. The gate was proven non-vacuous by planting `1i32 as i32` in an android-only module and confirming it fails the task, then reverting. --- .gitea/workflows/android.yml | 16 +++++ clients/android/kit/build.gradle.kts | 80 +++++++++++++++++----- clients/android/native/src/audio.rs | 14 ++-- clients/android/native/src/decode/vsync.rs | 17 ++++- clients/android/native/src/mic.rs | 17 +++-- clients/android/native/src/pad_audio.rs | 4 +- 6 files changed, 113 insertions(+), 35 deletions(-) diff --git a/.gitea/workflows/android.yml b/.gitea/workflows/android.yml index 43fe3cb9..9707c2c0 100644 --- a/.gitea/workflows/android.yml +++ b/.gitea/workflows/android.yml @@ -160,6 +160,22 @@ jobs: key: gradle-${{ hashFiles('clients/android/**/*.gradle.kts', 'clients/android/gradle/wrapper/gradle-wrapper.properties') }} restore-keys: gradle- + # Clippy for the ANDROID target. Like the kit tests below, this was running NOWHERE: ci.yml + # lints `--workspace` on the host, where `clients/android/native` and every + # `#[cfg(target_os = "android")]` module elsewhere compile out, and this workflow only ever + # built. Discovered in 2026-08 with five lints already resident — code no gate had ever read. + # + # Placed BEFORE assembleDebug deliberately: a lint failure should cost the ~10 s the lint + # takes, not the full three-ABI build first. It shares sccache and the target dir with the + # build that follows, so the compile is not paid twice. + # + # The task lints arm64-v8a AND armeabi-v7a, and reuses the build task's exact cargo-ndk + # environment — see the long note on `registerCargoNdkClippy` in kit/build.gradle.kts for why + # both pointer widths are load-bearing and why the environment must not be duplicated here. + - name: Clippy (Android target, deny warnings) + working-directory: clients/android + run: ./gradlew :kit:cargoNdkClippy --stacktrace + # The kit's JVM unit tests — the pure parsers, migrations and feedback policies. They were # running nowhere: this workflow only assembled, and android-screenshots.yml runs the :app # module's tests, so nothing enforced :kit's. Cheap (a couple of seconds against an already diff --git a/clients/android/kit/build.gradle.kts b/clients/android/kit/build.gradle.kts index 7d0807b4..0ab0cfb0 100644 --- a/clients/android/kit/build.gradle.kts +++ b/clients/android/kit/build.gradle.kts @@ -67,30 +67,37 @@ fun androidSdkDir(): String { return "${System.getProperty("user.home")}/Library/Android/sdk" } +// Every cargo-ndk invocation needs the same discovery environment, and they must not drift apart: +// a lint that ran against a different toolchain/sysroot than the build is a lint about a different +// program. Applied by both `registerCargoNdk` (build) and `registerCargoNdkClippy` (lint). +fun Exec.cargoNdkEnvironment() { + val sdk = androidSdkDir() + // A GUI Android Studio launch does not source the login shell, so make cargo, the NDK, and + // cmake (libopus builds via the cmake crate) discoverable explicitly — same as a bare CLI. + val cmakeBin = "$sdk/cmake/3.22.1/bin" + environment( + "PATH", + cargoBin + File.pathSeparator + cmakeBin + File.pathSeparator + System.getenv("PATH"), + ) + environment("ANDROID_HOME", sdk) + environment("ANDROID_NDK_HOME", "$sdk/ndk/$ndkVer") + // CMake's built-in Android support (used by the cmake crate for libopus) finds the NDK via + // these, and uses Ninja (bundled next to the SDK cmake) since there's no `make`. + environment("ANDROID_NDK_ROOT", "$sdk/ndk/$ndkVer") + environment("ANDROID_NDK", "$sdk/ndk/$ndkVer") + environment("CMAKE_GENERATOR", "Ninja") + // audiopus_sys picks static-vs-dynamic by HOST not target — force the bundled static libopus + // (pure C) so the android .so links it instead of looking for the host's libopus.so. + environment("LIBOPUS_STATIC", "1") + environment("LIBOPUS_NO_PKG", "1") +} + fun registerCargoNdk(taskName: String, release: Boolean) = tasks.register(taskName) { group = "rust" description = "cargo-ndk build of punktfunk-client-android (${if (release) "release" else "debug"})" workingDir = repoRoot - val sdk = androidSdkDir() - // A GUI Android Studio launch does not source the login shell, so make cargo, the NDK, and - // cmake (libopus builds via the cmake crate) discoverable explicitly — same as a bare CLI. - val cmakeBin = "$sdk/cmake/3.22.1/bin" - environment( - "PATH", - cargoBin + File.pathSeparator + cmakeBin + File.pathSeparator + System.getenv("PATH"), - ) - environment("ANDROID_HOME", sdk) - environment("ANDROID_NDK_HOME", "$sdk/ndk/$ndkVer") - // CMake's built-in Android support (used by the cmake crate for libopus) finds the NDK via - // these, and uses Ninja (bundled next to the SDK cmake) since there's no `make`. - environment("ANDROID_NDK_ROOT", "$sdk/ndk/$ndkVer") - environment("ANDROID_NDK", "$sdk/ndk/$ndkVer") - environment("CMAKE_GENERATOR", "Ninja") - // audiopus_sys picks static-vs-dynamic by HOST not target — force the bundled static libopus - // (pure C) so the android .so links it instead of looking for the host's libopus.so. - environment("LIBOPUS_STATIC", "1") - environment("LIBOPUS_NO_PKG", "1") + cargoNdkEnvironment() // Resolve cargo by ABSOLUTE path: Gradle's Exec resolves command[0] via the JVM's // inherited PATH, NOT the environment("PATH", …) set above (that only reaches the spawned // child). A GUI Android Studio launch (and any daemon it started) has no ~/.cargo/bin on @@ -113,6 +120,41 @@ fun registerCargoNdk(taskName: String, release: Boolean) = commandLine(cmd) } +// ------------------------------------------------------------------------------------------------ +// Lint the ANDROID target. `punktfunk-client-android` and every `#[cfg(target_os = "android")]` +// module elsewhere in the workspace were, until this task existed, **completely unlinted**: ci.yml +// runs `cargo clippy --workspace` on the HOST, where all of that code is compiled out, and this +// workflow only ever ran `build`. The gap was found in 2026-08 with five lints sitting in +// clients/android/native (two of them `unnecessary_cast`, which is exactly the class that decides +// whether a cast is redundant BY POINTER WIDTH). +// +// Both widths are linted, and that is the load-bearing part: arm64-v8a is 64-bit and armeabi-v7a is +// 32-bit, so a cast that is redundant on one can be required on the other. Linting only the primary +// ABI would license "fixes" that break the 32-bit build — the shipping ABI for the many 32-bit +// Google TV / Android TV boxes this client targets. x86_64 is deliberately omitted: it is +// emulator-only and shares its pointer width with arm64, so it costs a third of the job's lint time +// for no signal these two do not already carry. +// +// `--all-targets` for the same reason ci.yml spells it out: without it the `#[cfg(test)]` modules +// are never compiled, and un-compiled test code drifts silently. +fun registerCargoNdkClippy(taskName: String) = + tasks.register(taskName) { + group = "verification" + description = "clippy (deny warnings) for punktfunk-client-android on both Android widths" + workingDir = repoRoot + cargoNdkEnvironment() + commandLine( + // Absolute cargo path for the same reason as the build task above. + "$cargoBin/cargo", "ndk", + "-t", "arm64-v8a", "-t", "armeabi-v7a", + "--platform", "28", + "clippy", "-p", "punktfunk-client-android", "--all-targets", + "--", "-D", "warnings", + ) + } + +val cargoNdkClippy = registerCargoNdkClippy("cargoNdkClippy") + // Post-link floor check: every undefined symbol in the built .so must exist in the API-28 stubs, // else System.loadLibrary fails on devices at the minSdk floor (see the script header for the // 0.9.0 incident this guards against). Runs right after its cargo-ndk task; the APK build depends diff --git a/clients/android/native/src/audio.rs b/clients/android/native/src/audio.rs index 7cf53722..a1135800 100644 --- a/clients/android/native/src/audio.rs +++ b/clients/android/native/src/audio.rs @@ -44,6 +44,14 @@ use std::sync::mpsc::{sync_channel, Receiver, SyncSender, TrySendError}; use std::sync::Arc; use std::time::Duration; +/// What one playback open attempt yields: the stream, plus both halves of the PCM hand-off — the +/// sender the decode thread fills and the receiver that returns drained buffers for refill. +/// +/// Named rather than written inline because the closure's return type trips +/// `clippy::type_complexity`, which the Android target is now linted for (`:kit:cargoNdkClippy`) +/// after years of nothing checking it. +type OpenedPlayback = ndk::audio::Result<(AudioStream, SyncSender>, Receiver>)>; + const SAMPLE_RATE: i32 = 48_000; /// Decoded-chunk hand-off depth: 64 × 5 ms = 320 ms slack (matches the core's AUDIO_QUEUE). const RING_CHUNKS: usize = 64; @@ -175,11 +183,7 @@ impl AudioPlayback { // One open attempt at a given sharing mode. Everything the realtime callback captures // (channels, ring, prime state) is rebuilt per attempt — `open_stream` consumes the builder // AND the callback, so nothing survives a failed try to reuse. - let try_open = |sharing: AudioSharingMode| -> ndk::audio::Result<( - AudioStream, - SyncSender>, - Receiver>, - )> { + let try_open = |sharing: AudioSharingMode| -> OpenedPlayback { let (tx, rx) = sync_channel::>(RING_CHUNKS); // Recycle free-list: drained PCM buffers go BACK to the decode thread to be refilled, so // the realtime callback never frees heap (Android's Scudo allocator has unbounded free() diff --git a/clients/android/native/src/decode/vsync.rs b/clients/android/native/src/decode/vsync.rs index c5361ea3..a0ccfc55 100644 --- a/clients/android/native/src/decode/vsync.rs +++ b/clients/android/native/src/decode/vsync.rs @@ -33,8 +33,21 @@ pub(super) fn now_monotonic_ns() -> i64 { }; // SAFETY: `clock_gettime` with a valid out-pointer is an always-safe syscall. unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC, &mut ts) }; - // Explicit widening: timespec's fields are 32-bit on armv7 (time_t/c_long). - ts.tv_sec as i64 * 1_000_000_000 + ts.tv_nsec as i64 + // Explicit widening: `timespec`'s fields are 32-bit on armv7 (`time_t`/`c_long`) and 64-bit on + // arm64, so these casts are REQUIRED on one shipping ABI and redundant on the other. + // + // `:kit:cargoNdkClippy` lints both widths, so it sees the redundant half and flags it; taking + // its advice would break the 32-bit build, which is the ABI for the many 32-bit Google TV / + // Android TV boxes this client targets. `i64::from`/`.into()` do not escape it either — they + // just trade `unnecessary_cast` for `useless_conversion` on the 64-bit side. So the cast stays + // and the lint is answered here rather than in whichever build breaks first. + #[allow( + clippy::unnecessary_cast, + reason = "required on 32-bit ABIs; redundant only on 64-bit" + )] + { + ts.tv_sec as i64 * 1_000_000_000 + ts.tv_nsec as i64 + } } /// One upcoming frame timeline (API 33+ payload): when SurfaceFlinger expects to present the diff --git a/clients/android/native/src/mic.rs b/clients/android/native/src/mic.rs index 4db552b9..e07d2c74 100644 --- a/clients/android/native/src/mic.rs +++ b/clients/android/native/src/mic.rs @@ -26,6 +26,15 @@ use std::sync::mpsc::{sync_channel, Receiver, RecvTimeoutError, SyncSender, TryS use std::sync::Arc; use std::time::{Duration, SystemTime, UNIX_EPOCH}; +/// What one capture open attempt yields: the stream, plus both halves of the PCM hand-off — the +/// receiver the encode worker drains and the sender that returns emptied buffers for reuse. Note +/// the pair is the mirror image of [`crate::audio::OpenedPlayback`]'s: here the callback produces +/// and the worker consumes. +/// +/// Named rather than written inline for the same reason as that one — `clippy::type_complexity`, +/// now that the Android target is actually linted (`:kit:cargoNdkClippy`). +type OpenedCapture = ndk::audio::Result<(AudioStream, Receiver>, SyncSender>)>; + const CHANNELS: usize = 1; const SAMPLE_RATE: i32 = 48_000; /// 10 ms per channel @ 48 kHz — half the desktop clients' 20 ms frame, trading a little Opus @@ -84,13 +93,7 @@ impl MicCapture { // One open attempt at a given sharing mode (same pattern as [`crate::audio`]: `open_stream` // consumes the builder AND the callback, so each try rebuilds the channels it captures). - let try_open = |sharing: AudioSharingMode, - voice: bool| - -> ndk::audio::Result<( - AudioStream, - Receiver>, - SyncSender>, - )> { + let try_open = |sharing: AudioSharingMode, voice: bool| -> OpenedCapture { let (tx, rx) = sync_channel::>(RING_CHUNKS); // Recycle free-list, mirroring the playback path: the realtime capture callback must // not touch the allocator (Android's Scudo has unbounded malloc/free tail latency — an diff --git a/clients/android/native/src/pad_audio.rs b/clients/android/native/src/pad_audio.rs index deaa7a36..48d3f1aa 100644 --- a/clients/android/native/src/pad_audio.rs +++ b/clients/android/native/src/pad_audio.rs @@ -408,8 +408,8 @@ pub(crate) unsafe fn self_test(fd: i32, seconds: i32, hz: i32) -> i32 { frame.fill(0); // Channels 2 and 3 are the voice coils; the speaker pair stays silent so a pass is // unambiguously FELT rather than merely audible. - for c in 2..channels { - frame[c] = sample; + for slot in frame.iter_mut().take(channels).skip(2) { + *slot = sample; } } if let Err(e) = playback.write_interleaved(&chunk) {