fix(packaging): ship libperry_ui_android.a to Windows installs so perry/ui android apps link (#4823)#5502
Merged
Conversation
…ry/ui android apps link (#4823) A Windows user (discussion #4823) with the NDK/SDK and Rust android targets installed could not build a `perry/ui` app for android: `perry --target android` cleared the runtime-only link and then failed with "perry/ui imported but libperry_ui_android.a not found". Root cause was a packaging gap, not user error. Two holes, two fixes (the options the maintainer approved): 1. release-packages.yml: the Windows build leg cross-built only the android runtime+stdlib (#872), never the UI backend. It now also cross-builds perry-ui-android (--profile dist; best-effort with an explicit $LASTEXITCODE guard so a finicky aws-lc-rs cross-build cannot sink the release) and stages libperry_ui_android.a into the zip. 2. stage-npm.sh dropped the aarch64-linux-android/release/ subdir entirely, so npm packages shipped no android libs at all. A new stage_android_cross_libs helper stages all three archives into the Windows npm package under bin/aarch64-linux-android/release/ (compressed to .a.zst), sourcing each from the Windows artifact first and falling back to the authoritative perry-cross-aarch64-linux-android bundle for the UI lib. Enables npm-installed android builds for the first time and backfills the UI lib if the Windows cross-build was skipped. Also improves the link-time error message (build_and_run.rs) to point binary-install users at the prebuilt cross bundle, not a cargo build they cannot run.
📝 WalkthroughWalkthroughVersion ChangesAndroid UI Library Windows Packaging Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release-packages.yml (1)
422-431: 💤 Low valueConsider logging a warning if
clang++is not found.Unlike the C linker (
$linker), the C++ linker ($linkerxx) is probed but not validated with an error exit. This is acceptable since it's only needed for the best-effort UI build, but emitting aWrite-Warningwhen$linkerxxis missing would aid debugging when the UI build fails due to missing C++ toolchain.💡 Optional diagnostic improvement
$linkerxx = Join-Path $toolchain "aarch64-linux-android$($env:ANDROID_API_LEVEL)-clang++.exe" } + if (-not (Test-Path $linkerxx)) { + Write-Warning "Android NDK clang++ not found at $linkerxx — perry-ui-android build may fail." + } $env:CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER = $linker🤖 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/release-packages.yml around lines 422 - 431, The C++ linker path variable $linkerxx is probed by checking for both the .cmd and .exe versions but unlike the C linker, there is no validation or warning when neither file is found. Add a Write-Warning statement after the $linkerxx assignment block that checks if the resolved path does not exist and logs a warning message indicating that the C++ linker clang++ could not be found, which will help with debugging if the UI build subsequently fails due to missing C++ toolchain components.CHANGELOG.md (1)
7-10: 💤 Low valueAdd language specifier to fenced code block.
Line 7 opens a code fence without a language identifier. Per markdownlint (MD040), add a language specifier (e.g.,
bash,sh, ortext) to make the intent clear and improve syntax highlighting.✨ Proposed fix
-``` +```sh Error: perry/ui imported but libperry_ui_android.a not found. Build with: RUSTC_BOOTSTRAP=1 RUSTFLAGS="-Z tls-model=global-dynamic" cargo build --release -p perry-ui-android --target aarch64-linux-android -``` +```🤖 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 `@CHANGELOG.md` around lines 7 - 10, The fenced code block in CHANGELOG.md starting at line 7 is missing a language specifier on the opening fence. Add the language identifier `sh` immediately after the opening triple backticks (```sh) to comply with markdownlint rule MD040 and enable proper syntax highlighting for the shell command block.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In @.github/workflows/release-packages.yml:
- Around line 422-431: The C++ linker path variable $linkerxx is probed by
checking for both the .cmd and .exe versions but unlike the C linker, there is
no validation or warning when neither file is found. Add a Write-Warning
statement after the $linkerxx assignment block that checks if the resolved path
does not exist and logs a warning message indicating that the C++ linker clang++
could not be found, which will help with debugging if the UI build subsequently
fails due to missing C++ toolchain components.
In `@CHANGELOG.md`:
- Around line 7-10: The fenced code block in CHANGELOG.md starting at line 7 is
missing a language specifier on the opening fence. Add the language identifier
`sh` immediately after the opening triple backticks (```sh) to comply with
markdownlint rule MD040 and enable proper syntax highlighting for the shell
command block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5aab06c4-9a9e-4c46-b9a0-99daed157fea
📒 Files selected for processing (6)
.github/workflows/release-packages.ymlCHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry/src/commands/compile/link/build_and_run.rsscripts/stage-npm.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes GitHub discussion #4823 — a Windows user with the NDK/SDK and Rust android targets installed could not build a
perry/uiapp for android.perry --target androidcleared the runtime-only link step and then failed:Root cause is a packaging gap, not user error. Two distinct holes:
runtime+stdlib(libperry_runtime.afor Android target not bundled in WinGet package #872), neverlibperry_ui_android.a. The runtime-only link succeeded, but any app importingperry/uicouldn't resolve the UI backend. The UI lib was built — but only inside the standaloneperry-cross-aarch64-linux-androidbundle (Ship cross-target staticlib bundles in release CI so builders don't recompile runtime/stdlib/ui per target #1083), which a binary-install user has no reason to know about.stage-npm.shflattened libs intolib/and dropped theaarch64-linux-android/release/subdir entirely, so npm packages shipped no android cross-libs at all; android-from-npm was wholly unsupported.Changes (the two options approved on the discussion)
release-packages.yml— the Windowsbuild:leg now also cross-buildsperry-ui-android(--profile dist; UI crates are already staticlib, so no-staticwrapper). It's best-effort: a native non-zero exit is caught via an explicit$LASTEXITCODEguard (pwsh native failures aren't terminating errors) so a finicky aws-lc-rs cross-build can't sink the release; the mandatory runtime/stdlib builds are made explicitly fatal so they can't be masked. Staging oflibperry_ui_android.ainto the zip isTest-Path-guarded.stage-npm.sh— newstage_android_cross_libshelper stages all three android archives into the Windows npm package underbin/aarch64-linux-android/release/(the layoutlibrary_search.rsprobes, already inside thefilesallowlist), compressed to.a.zstlike the rest. Each lib is sourced from the Windows build artifact first, falling back to theperry-cross-aarch64-linux-androidbundle the npm-publish job already downloads — so the UI lib is backfilled from the authoritative ubuntu build even if the best-effort Windows cross-build was skipped. This also enables npm-installed android builds for the first time.Also improves the link-time error message (
build_and_run.rs) so binary-install users (no source tree) are pointed at dropping the prebuilt lib fromperry-cross-aarch64-linux-android.tar.gz, not acargo buildthey can't run.Testing
stage_android_cross_libsvalidated end-to-end against a synthetic artifact layout (UI lib intentionally absent from the Windows artifact): runtime/stdlib resolve from the Windows artifact, UI lib backfills from the cross-bundle. Provenance confirmed byte-for-byte.bash -nclean; conflict-free merge onto currentmain(v0.5.1197 → bump to v0.5.1198).Note / open call
The zip path depends on the Windows-side
perry-ui-androidcross-build succeeding (aws-lc-rs cross-from-Windows is the risk). npm always gets the authoritative ubuntu lib via the fallback; the zip has no equivalent ubuntu fallback unless we add a post-merge job. Judged that over-engineering given the same leg already cross-builds runtime+stdlib fine — happy to add a zip-side fallback if preferred.Summary by CodeRabbit
Release Notes v0.5.1198
Bug Fixes
libperry_ui_android.a), enabling Android cross-compilation without additional manual setup.Documentation