unom/punktfunk
6
0
Fork 1
Code Issues 3 Pull Requests 1 Actions 1 Packages Projects Releases 46 Wiki Activity

ci(android): lint the Android target, which nothing had ever done

Browse Source 
`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.
This commit is contained in:
enricobuehler
2026-08-08 00:01:39 +02:00
parent 70e6b80200
commit 74270109dd
6 changed files with 113 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitea/workflows/android.yml
+16
View File
@@ -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
clients/android/kit/build.gradle.kts
+61 -19
View File
@@ -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<Exec>(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<Exec>(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
clients/android/native/src/audio.rs
+9 -5
View File
@@ -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<Vec<f32>>, Receiver<Vec<f32>>)>;
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<Vec<f32>>,
Receiver<Vec<f32>>,
)> {
let try_open = |sharing: AudioSharingMode| -> OpenedPlayback {
let (tx, rx) = sync_channel::<Vec<f32>>(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()
clients/android/native/src/decode/vsync.rs
+15 -2
View File
@@ -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
clients/android/native/src/mic.rs
+10 -7
View File
@@ -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<Vec<f32>>, SyncSender<Vec<f32>>)>;
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<Vec<f32>>,
SyncSender<Vec<f32>>,
)> {
let try_open = |sharing: AudioSharingMode, voice: bool| -> OpenedCapture {
let (tx, rx) = sync_channel::<Vec<f32>>(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
clients/android/native/src/pad_audio.rs
+2 -2
View File
@@ -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) {