From 0e9c7842f71e4f26fd077bd84a6f3757739c21a4 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Tue, 1 Sep 2026 10:30:02 -0400 Subject: [PATCH] Find the SDK without an environment, and say what broke A service manager hands its process a scrubbed environment -- measured in the Gentoo guest, an OpenRC user service gets 19 variables and neither ANDROID_HOME nor ANDROID_NDK_HOME among them -- so the tool lookups have to stand on their own. Two ways they did not: - $ANDROID_HOME was taken as given. This machine exports a system-wide /opt/android-sdk whose build-tools/36.0.0 holds nothing but package.xml, so the download failed with "failed to spawn .../zipalign", naming the tool rather than the root that was wrong. Each lookup now takes the first candidate that actually contains what it needs, and names every place it looked. - The NDK search took the newest child of ~/Android and filtered it afterwards, which can only reject that one directory. It worked by the luck of `Sdk` sorting before `android-ndk-r27c`; a lowercase neighbour would have hidden an NDK that was sitting right there. newest_child_where filters before taking the max. And the failure had nowhere to go: ApiError::Internal answered with an empty body, so a missing NDK reached the phone as "Server returned HTTP 500" while the message naming the path stayed in a log on a machine the person holding the phone cannot see. It now answers with the same {err:#} chain it logs. Every route is behind the bearer token of a device somebody enrolled themselves, so there is no third party to withhold it from. Diagnosis from the tdep-survey session, which measured the OpenRC environment and the /opt/android-sdk contents. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 34 +++++++- server/src/routes.rs | 46 +++++++++- server/src/sdk.rs | 195 +++++++++++++++++++++++++++++++++++++------ 3 files changed, 244 insertions(+), 31 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4eb9cd1..800e46b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -240,6 +240,16 @@ mutable at runtime from the phone. somebody asked. Stop and Uninstall do go through the script, and do strand the phone; the app confirms them rather than hiding them. +- **A 500 answers with its message.** `ApiError::Internal` used to be a + bare status with an empty body, on the grounds that an internal cause + is not safe to hand back -- but every route here is behind the bearer + token of a device somebody enrolled themselves, so there is no third + party to withhold it from, and what these actually say is which tool on + the build machine could not be found or would not run. Withheld, a + missing NDK reached the phone as "Server returned HTTP 500", with the + explanation in a log on a machine the person holding it cannot see; the + app has always shown a failure body when there is one, so the whole fix + was on this side. The log keeps the same `{err:#}` chain. - **A remote git failure is shortened and explained in one place** (`git::remote_failure`). Git's stderr runs to a paragraph of generic advice, so the card gets the first line and the log gets all of it; and @@ -830,4 +840,26 @@ session reads. What follows is what that means here. what *is* attached instead of guessing which of the three situations it is in. - The server needs `aapt2` (SDK build-tools) to add an app, and - `llvm-strip` (NDK) only for apps that need stripping. + `llvm-strip` (NDK) only for apps that need stripping. **It has to find + both without an environment**, because the way this server usually + starts is from a service manager, and one hands its process a scrubbed + environment: measured in the Gentoo guest, an OpenRC user service gets + 19 variables with neither `$ANDROID_HOME` nor `$ANDROID_NDK_HOME` among + them, and a systemd user unit inherits an equally bare one unless + somebody imported theirs. Neither branch of `service-default` passes any + through, so this is not a Gentoo quirk and "export it in the unit" is + not the fix. `HOME` does survive, which is what the home-relative + candidates in `sdk.rs` are for. The trap it produces is a build that + *succeeds* and a download that 500s -- a project's own build script + sources its `android-env.sh` and repairs the environment inside its own + process, while the strip runs in-process here and sources nothing. + So `sdk_roots` lists candidates and every lookup takes the first that + actually contains the tools it needs: an `$ANDROID_HOME` that exists is + not one with the tools in it (this machine's system-wide + `/opt/android-sdk` has a `build-tools/36.0.0` holding nothing but + `package.xml`), and taking it blamed `zipalign` for a root that should + never have been chosen. Filter *before* taking the newest, too -- + `newest_child_where` exists because filtering the single newest child + can only reject that one directory, so `Sdk` sitting beside + `android-ndk-r27c` in `~/Android` hid the NDK from a message that said + "install one". diff --git a/server/src/routes.rs b/server/src/routes.rs index 38f993b..8621873 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -158,10 +158,23 @@ impl IntoResponse for ApiError { Self::BadRequest(_) => StatusCode::BAD_REQUEST, Self::RangeNotSatisfiable => StatusCode::RANGE_NOT_SATISFIABLE, Self::Internal(err) => { - // The only variant whose real cause isn't safe to hand back - // verbatim, and the only one worth a log line. + // The one variant worth a log line, because it is the one + // nobody wrote for a reader. It still answers with its + // message: this used to be a bare 500 with an empty body, + // on the grounds that an internal cause is not safe to + // hand back -- but every route here is behind the bearer + // token of a device somebody enrolled themselves, so + // there is no third party to be careful of, and what + // these actually say is which tool on the build machine + // could not be found or would not run. Withholding that + // left the phone reporting "HTTP 500" for a missing NDK, + // with the only explanation in a log on a machine the + // person holding it cannot see. + // + // The whole `{err:#}` chain rather than the top context, + // because the tail is the part that names the path. tracing::error!("{err:#}"); - return StatusCode::INTERNAL_SERVER_ERROR.into_response(); + return (StatusCode::INTERNAL_SERVER_ERROR, format!("{err:#}")).into_response(); } }; (status, self.to_string()).into_response() @@ -1576,3 +1589,30 @@ async fn serve_apk( .body(Body::from_stream(stream)) .expect("response with a streaming body is valid")) } + +#[cfg(test)] +mod tests { + use super::*; + + /// A tool that could not be found on the build machine is a + /// configuration problem there, and the person who can fix it is + /// holding the phone rather than reading this server's log. The + /// message travelled nowhere for a while -- a bare 500 with an empty + /// body -- and the app, which shows a failure's body whenever there + /// is one, could only say "Server returned HTTP 500". + #[tokio::test] + async fn an_internal_failure_still_tells_the_phone_what_broke() { + let err = anyhow::anyhow!("llvm-strip not found -- looked under /home/x/Android") + .context("stripping app-dioxus"); + let response = ApiError::Internal(err).into_response(); + + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("body"); + let body = String::from_utf8(body.to_vec()).expect("utf-8"); + // The whole chain: the tail is the half that names the path. + assert!(body.contains("stripping app-dioxus"), "{body}"); + assert!(body.contains("/home/x/Android"), "{body}"); + } +} diff --git a/server/src/sdk.rs b/server/src/sdk.rs index 99dde2b..53de333 100644 --- a/server/src/sdk.rs +++ b/server/src/sdk.rs @@ -23,42 +23,105 @@ fn home_dir() -> PathBuf { std::env::home_dir().unwrap_or_default() } -/// Honors `$ANDROID_HOME`/`$ANDROID_SDK_ROOT` before falling back to -/// Android Studio's default location on Linux, so this works on a machine -/// that keeps its SDK somewhere else without needing a flag. -fn sdk_root() -> PathBuf { - std::env::var_os("ANDROID_HOME") - .or_else(|| std::env::var_os("ANDROID_SDK_ROOT")) +/// Everywhere an SDK might be, best guess first: what the environment +/// says, then Android Studio's default location on Linux. +/// +/// Candidates rather than one answer, because **an `$ANDROID_HOME` that +/// exists is not the same thing as one with the tools in it**, and taking +/// it as the answer is a failure that names the wrong thing. This machine +/// exports a system-wide `ANDROID_HOME=/opt/android-sdk` holding a +/// `build-tools/36.0.0` with no `aapt2`, `zipalign` or `apksigner` under +/// it at all; a server that inherited that env -- which a service started +/// by a service manager rather than from a shell is the likely way to -- +/// answered a download with `failed to spawn +/// /opt/android-sdk/build-tools/36.0.0/zipalign`, blaming the tool for a +/// root that should never have been chosen. So each lookup below takes +/// the first candidate that actually holds what it needs, and names every +/// place it looked when none does. +/// +/// Listing places nobody named is only safe *because* of that check, and +/// it is what makes this work from a service: a service manager hands its +/// process a scrubbed environment -- measured in the Gentoo guest, an +/// OpenRC user service gets 19 variables and neither `$ANDROID_HOME` nor +/// `$ANDROID_NDK_HOME` among them, and a systemd user unit inherits an +/// equally bare one unless somebody has imported theirs. `HOME` does +/// survive, which is why the home-relative candidates are the ones that +/// carry this. +fn sdk_roots() -> Vec { + ["ANDROID_HOME", "ANDROID_SDK_ROOT"] + .into_iter() + .filter_map(std::env::var_os) .map(PathBuf::from) - .unwrap_or_else(|| home_dir().join("Android/Sdk")) + .chain([ + home_dir().join("Android/Sdk"), + PathBuf::from("/opt/android-sdk"), + ]) + .collect() } -/// Highest-versioned entry directly under `dir`, by plain name sort -- -/// good enough for SDK/NDK release directories, which sort correctly as -/// strings within a single major version scheme. -fn newest_child(dir: &Path) -> Option { +/// Highest-versioned entry directly under `dir` that `keep` accepts, by +/// plain name sort -- good enough for SDK/NDK release directories, which +/// sort correctly as strings within a single major version scheme. +/// +/// The filter is applied *before* the max is taken, which is the whole +/// reason it is a parameter rather than something the caller does to the +/// answer: filtering afterwards can only reject the one directory that +/// happened to sort last, so a single unrelated neighbour hides an +/// install that is sitting right there. `~/Android` holds `Sdk` beside +/// `android-ndk-r27c` and worked only by the luck of `S` sorting before +/// `a`; a lowercase `sdk`, an `avd`, or a second toolchain would have +/// made the NDK undiscoverable while the error said "install one". +fn newest_child_where(dir: &Path, keep: impl Fn(&Path) -> bool) -> Option { let mut children: Vec = std::fs::read_dir(dir) .ok()? .filter_map(|entry| entry.ok()) .map(|entry| entry.path()) - .filter(|path| path.is_dir()) + .filter(|path| path.is_dir() && keep(path)) .collect(); children.sort(); children.pop() } -/// The newest installed `build-tools//` directory, which is where -/// `aapt2`, `zipalign` and `apksigner` live. +/// What this server runs out of a build-tools directory, and therefore +/// what makes one usable. Named once here rather than at the three call +/// sites, so a directory cannot be chosen for a tool nobody checked was +/// in it. +const BUILD_TOOLS: [&str; 3] = ["aapt2", "zipalign", "apksigner"]; + +/// The newest `/` directory under any of `bases` that has every +/// one of [`BUILD_TOOLS`] in it, best base first. +/// +/// Its own function so the rule can be exercised with directories rather +/// than through an environment and a process-wide cache. +fn newest_usable_build_tools(bases: &[PathBuf]) -> Option { + bases.iter().find_map(|base| { + newest_child_where(base, |dir| { + BUILD_TOOLS.iter().all(|tool| dir.join(tool).is_file()) + }) + }) +} + +/// The newest installed `build-tools//` directory that has all of +/// [`BUILD_TOOLS`] in it, from the first SDK candidate that offers one. pub fn build_tools_dir() -> Result<&'static Path> { static DIR: OnceLock> = OnceLock::new(); match DIR.get_or_init(|| { - let base = sdk_root().join("build-tools"); - newest_child(&base).ok_or_else(|| { + let bases: Vec = sdk_roots() + .iter() + .map(|sdk| sdk.join("build-tools")) + .collect(); + newest_usable_build_tools(&bases).ok_or_else(|| { format!( - "no Android SDK build-tools found under {} -- install one \ - (`android sdk install build-tools/37.0.0`, or source \ - ../app/android-env.sh, which does it for you)", - base.display(), + "no Android SDK build-tools with {} in them found -- looked under {}. \ + Install one (`android sdk install build-tools/37.0.0`, or source \ + ../app/android-env.sh, which does it for you), or point $ANDROID_HOME \ + at an SDK that has one.", + BUILD_TOOLS.join(", "), + bases + .iter() + .map(|base| base.display().to_string()) + .collect::>() + .join(", "), ) }) }) { @@ -67,12 +130,10 @@ pub fn build_tools_dir() -> Result<&'static Path> { } } +/// No existence check of its own: [`build_tools_dir`] only ever answers +/// with a directory it has already found every one of [`BUILD_TOOLS`] in. pub fn aapt2_path() -> Result { - let path = build_tools_dir()?.join("aapt2"); - if !path.is_file() { - bail!("aapt2 not found at {}", path.display()); - } - Ok(path) + Ok(build_tools_dir()?.join("aapt2")) } /// `llvm-strip` ships with the NDK, not the SDK, so this looks in the two @@ -85,8 +146,13 @@ pub fn llvm_strip_path() -> Result { let roots: Vec = std::env::var_os("ANDROID_NDK_HOME") .map(PathBuf::from) .into_iter() - .chain(newest_child(&sdk_root().join("ndk"))) - .chain(newest_child(&home_dir().join("Android")).filter(|path| { + .chain( + sdk_roots() + .iter() + .filter_map(|sdk| newest_child_where(&sdk.join("ndk"), |_| true)) + .collect::>(), + ) + .chain(newest_child_where(&home_dir().join("Android"), |path| { path.file_name() .is_some_and(|name| name.to_string_lossy().starts_with("android-ndk-")) })) @@ -149,3 +215,78 @@ pub fn run_checked(cmd: &mut std::process::Command) -> Result<()> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn dir(path: PathBuf) -> PathBuf { + std::fs::create_dir_all(&path).unwrap(); + path + } + + fn build_tools(base: &Path, version: &str, tools: &[&str]) { + let dir = dir(base.join(version)); + for tool in tools { + std::fs::write(dir.join(tool), "#!/bin/sh\n").unwrap(); + } + } + + /// The `~/Android` case: an SDK sitting beside the NDK is not the NDK, + /// and it must not be able to hide it by sorting last. Filtering the + /// single newest child instead of the children rejects that one + /// directory and answers "nothing here" with the NDK in plain view. + #[test] + fn an_unrelated_neighbour_does_not_hide_the_newest_match() { + let home = tempfile::tempdir().unwrap(); + dir(home.path().join("android-ndk-r27c")); + dir(home.path().join("sdk")); + dir(home.path().join("zz-scratch")); + + let found = newest_child_where(home.path(), |path| { + path.file_name() + .is_some_and(|name| name.to_string_lossy().starts_with("android-ndk-")) + }); + assert_eq!(found, Some(home.path().join("android-ndk-r27c"))); + } + + /// What `$ANDROID_HOME=/opt/android-sdk` looks like on this machine: + /// the directory is there and has a plausibly-named build-tools + /// version in it, with none of the tools inside. Taking it produces a + /// download that fails with "failed to spawn .../zipalign", naming + /// the tool rather than the root that was wrong. + #[test] + fn a_build_tools_directory_without_the_tools_is_not_used() { + let root = tempfile::tempdir().unwrap(); + let empty = dir(root.path().join("opt/android-sdk/build-tools")); + build_tools(&empty, "36.0.0", &["package.xml"]); + let real = dir(root.path().join("home/Android/Sdk/build-tools")); + build_tools(&real, "37.0.0", &BUILD_TOOLS); + + let bases = vec![empty, real.clone()]; + assert_eq!( + newest_usable_build_tools(&bases), + Some(real.join("37.0.0")), + "the first candidate that exists is not the first that works" + ); + } + + /// And a half-populated one is no better than an empty one: every + /// tool this server runs has to be in the directory it picks, or the + /// failure lands on whichever one it happens to spawn first. + #[test] + fn every_tool_has_to_be_present_not_just_the_first() { + let base = tempfile::tempdir().unwrap(); + build_tools(base.path(), "36.0.0", &["aapt2"]); + assert_eq!( + newest_usable_build_tools(&[base.path().to_path_buf()]), + None + ); + + build_tools(base.path(), "36.0.0", &BUILD_TOOLS); + assert_eq!( + newest_usable_build_tools(&[base.path().to_path_buf()]), + Some(base.path().join("36.0.0")), + ); + } +}