Add per-ABI split APKs to the Android release pipeline - #6139
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Android release build pipeline to additionally produce and archive per-ABI split APKs (arm64-v8a and armeabi-v7a) for sideload distribution, while keeping the default AAB/universal-APK flow unchanged unless explicitly opted in via a Gradle property.
Changes:
- Add a Gradle
splits.abiconfiguration gated behind-PabiSplits, while preservingndk.abiFiltersfor normal builds. - Extend the release deploy script to run
assembleRelease -PabiSplitsand archive the resulting per-ABI APKs alongside existing artifacts. - Add a CHANGELOG entry describing the new per-ABI APK artifacts and expected size reduction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
scripts/deploy.ts |
Runs an additional Gradle assemble step with -PabiSplits and archives per-ABI APK outputs. |
android/app/build.gradle |
Adds an opt-in ABI split configuration and conditionally applies ndk.abiFilters to avoid AGP conflicts. |
CHANGELOG.md |
Documents the addition of per-ABI Android release APK artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (1)
scripts/deploy.ts:539
- The split-APK archiving hard-codes both the ABI list and the expected output filenames. This can drift from the Gradle
splits { abi { include ... } }configuration and will fail if the ABI list changes or if additional ABIs are added. Consider discovering the generated split APKs in the output directory and parsing the ABI from the filename instead of maintaining a second ABI list here.
const splitApkDir = join(guiPlatformDir, 'app/build/outputs/apk/release')
buildObj.abiApkFiles = {}
for (const abi of ['arm64-v8a', 'armeabi-v7a']) {
const archivedApk = join(archiveDir, `${outfile}-${abi}.apk`)
fs.copyFileSync(join(splitApkDir, `app-${abi}-release.apk`), archivedApk)
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (1)
scripts/deploy.ts:634
- The per-ABI Zealot upload uses
call(curl ...), andcalllogs the full command line. That will printzealotApiTokenand the ABI channel key into build logs (credential leakage). Also,curlshould use--failso HTTP 4xx/5xx causes a non-zero exit code instead of silently succeeding.
mylog(`\n\nUploading ${abi} APK to Zealot: ${zealotUrl}`)
call(
`curl -X POST "${zealotUrl}/api/apps/upload?token=${zealotApiToken}&channel_key=${abiChannelKey}&branch=${branch}&git_commit=${gitCommit}" -F "file=@${abiApkFiles[abi]}"`
)
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (3)
scripts/deploy.ts:543
- The comment claims each split APK is “roughly half” the size of the universal APK, but the PR description measurements show closer to ~20–25% smaller (~30 MiB). Keeping this accurate helps avoid confusion when validating artifacts.
// per-ABI APKs for distribution outside Google Play (Play already
// serves per-ABI installs from the AAB). Each one is roughly half the
// size of the universal APK. The gradle daemon reuses the compile work
// from the bundle task above, so this only pays for packaging and
// signing. Maestro builds skip this entirely:
android/app/build.gradle:115
- This comment says each split APK is “roughly half” the size of the universal APK, but the PR description indicates ~20–25% smaller (~30 MiB). Consider updating the wording to match measured sizes so future readers don’t expect a 50% reduction.
// Edge addition: sideloadable per-ABI APKs for distribution outside
// Google Play (Play already serves per-ABI installs from the AAB).
// Each one is roughly half the size of the universal APK. Off by
// default so normal builds are unchanged; the deploy script opts in:
scripts/deploy.ts:557
abivalues come from deploy-config and are used to construct file paths. If an unexpected value is present (typo, or something like '../...'), this will currently fail with an unhelpful ENOENT (or potentially write outside the archive dir). Validate the ABI string and produce a clear error if the expected split APK isn’t found.
for (const { abi } of splitArchitectures) {
const archivedApk = join(archiveDir, `${outfile}-${abi}.apk`)
fs.copyFileSync(join(splitApkDir, `app-${abi}-release.apk`), archivedApk)
buildObj.abiApkFiles[abi] = archivedApk
}
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (4)
scripts/deploy.ts:649
- This upload uses
call(...), which logs the full command string. Because the Zealot API token is embedded in the URL, it will be printed to logs and can leak via build output retention. Prefer executing the command without logging secrets (or log a redacted form).
mylog(`\n\nUploading ${abi} APK to Zealot: ${zealotUrl}`)
call(
`curl -X POST "${zealotUrl}/api/apps/upload?token=${zealotApiToken}&channel_key=${abiChannelKey}&branch=${branch}&git_commit=${gitCommit}" -F "file=@${apkFile}"`
)
scripts/deploy.ts:541
- The comment says each split APK is "roughly half" the size of the universal APK, but the PR description measurements show ~20–25% smaller. Keeping this accurate helps avoid setting incorrect expectations for artifact sizes.
// serves per-ABI installs from the AAB). Each one is roughly half the
// size of the universal APK. The gradle daemon reuses the compile work
scripts/deploy.ts:557
abiis taken directly from deploy-config and interpolated into a filename. Without validation, an unexpected value can produce confusing ENOENT failures (or even path traversal via..). Validate supported ABIs and fail with a clear error if the expected APK file is missing before copying/archiving.
for (const { abi } of splitArchitectures) {
const archivedApk = join(archiveDir, `${outfile}-${abi}.apk`)
fs.copyFileSync(join(splitApkDir, `app-${abi}-release.apk`), archivedApk)
buildObj.abiApkFiles[abi] = archivedApk
android/app/build.gradle:110
- This comment says each split APK is "roughly half" the size of the universal APK, but the PR description measurements are ~20–25% smaller. Updating this keeps the build documentation accurate.
// Each one is roughly half the size of the universal APK. Off by
9ccc70d to
26abec4
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (1)
scripts/deploy.ts:557
- If deploy-config includes an unexpected ABI (typo / unsupported) or Gradle output naming changes,
fs.copyFileSyncwill throw anENOENTwithout a clear message about which ABI/config entry caused the failure. Adding an existence check with an explicit error makes misconfiguration and CI failures much easier to diagnose.
for (const { abi } of splitArchitectures) {
const archivedApk = join(archiveDir, `${outfile}-${abi}.apk`)
fs.copyFileSync(join(splitApkDir, `app-${abi}-release.apk`), archivedApk)
buildObj.abiApkFiles[abi] = archivedApk
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
cda933a to
a6defb0
Compare
a6defb0 to
2903346
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (3)
scripts/deploy.ts:542
- The comment says each split APK is “roughly half the size” of the universal APK, but the PR description’s measured release artifacts are ~20–25% smaller. Keeping the comment aligned avoids misleading future maintainers.
// Branches configured with splitArchitectures also archive sideloadable
// per-ABI APKs for distribution outside Google Play (Play already
// serves per-ABI installs from the AAB). Each one is roughly half the
// size of the universal APK. The gradle daemon reuses the compile work
// from the bundle task above, so this only pays for packaging and
android/app/build.gradle:112
- This comment says each per-ABI APK is “roughly half the size” of the universal APK, but the PR’s measured sizes are closer to ~20–25% smaller. Consider updating the wording to match reality so it doesn’t overpromise.
// Edge addition: sideloadable per-ABI APKs for distribution outside
// Google Play (Play already serves per-ABI installs from the AAB).
// Each one is roughly half the size of the universal APK. Off by
// default so normal builds are unchanged; the deploy script opts in:
// ./gradlew assembleRelease -PabiSplits
scripts/deploy.ts:562
splitArchitecturesis validated against an allowlist, but duplicate ABI entries will silently overwrite the archived file and can trigger repeated uploads for the same ABI. Since the intent is one channel per ABI, it’s safer to fail fast on duplicates in the config.
const supportedAbis = ['arm64-v8a', 'armeabi-v7a']
for (const { abi } of splitArchitectures) {
if (!supportedAbis.includes(abi)) {
throw new Error(`Unsupported abi "${abi}" in splitArchitectures`)
}
2903346 to
2c5a27f
Compare
3aca25a to
0207f0f
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (2)
scripts/deploy.ts:562
splitArchitecturesis validated for supported ABI values, but duplicate ABI entries aren’t rejected. Duplicates will overwriteabiApkFiles[abi]/ the archived destination path and can cause repeated uploads to Zealot for the same ABI, which is likely unintended. Consider rejecting duplicates up front with a clear error.
for (const { abi } of splitArchitectures) {
if (!supportedAbis.includes(abi)) {
throw new Error(`Unsupported abi "${abi}" in splitArchitectures`)
}
}
scripts/gitVersionFile.ts:95
- The PR description says Android versionCode is the build number * 10 (with ABI offsets), but the implementation here advances the build counter by 3 specifically to avoid collisions from adding small ABI offsets to the build number. Also,
scripts/updateVersion.tssetsversionCodetoversionFile.builddirectly. Please reconcile the PR description with the implemented scheme (either update the description, or adjust the build/versionCode logic to match what’s described).
// Advance by 3, not 1, so each build owns a block of three
// versionCodes: the Android split APKs add per-ABI offsets to the
// build number (universal +0, armeabi-v7a +1, arm64-v8a +2, see
// app/build.gradle), and Google Play rejects any versionCode it
// has ever seen, so consecutive builds must never overlap blocks.
0207f0f to
c5633a5
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (1)
scripts/deploy.ts:27
- The
SplitArchitecturedoc comment says the Zealot channel “receives it”, butzealotChannelKeyis optional (entries can be archived without upload). This comment should reflect that the channel is optional to avoid misleading future config authors.
/**
* One per-ABI split APK build: the ABI to package, and the Zealot
* channel that receives it.
*/
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (1)
scripts/deploy.ts:560
splitArchitecturesis validated for supported ABI values, but duplicates aren’t rejected. A duplicate entry will overwrite the same archived file path and can trigger redundant Zealot uploads for the same ABI/channel, which is confusing and makes the config more error-prone.
const supportedAbis = ['arm64-v8a', 'armeabi-v7a']
for (const { abi } of splitArchitectures) {
if (!supportedAbis.includes(abi)) {
throw new Error(`Unsupported abi "${abi}" in splitArchitectures`)
}
j0ntz
left a comment
There was a problem hiding this comment.
Automated code review (xhigh)
This PR adds per-ABI split APK builds (gradle splits plus per-ABI versionCode offsets), a +3 build-number stride, and a redacted-command helper for Zealot uploads.
The highest-value findings: a secret-leak gap in callRedacted (the unredacted command still reaches execSync, so a failed upload prints the token in the CI stack trace), the per-ABI versionCode offset making getBuildNumber() diverge from the release build number that info-card min/max/exactBuildNum checks compare against, and two changes that were meant to be opt-in but apply globally: dex.useLegacyPackaging in the top-level packagingOptions block hits every variant, and the +3 stride cuts same-day build capacity for every branch and platform including iOS.
The rest are lower-severity ordering and duplication cleanups: ABI validation runs after the multi-minute gradle build despite a comment claiming it fails fast, and the ABI allowlist, execSync options, and branch/commit encoding are each duplicated with no shared source of truth.
9 findings below, ranked, each independently verified by a separate agent (8 CONFIRMED, 1 PLAUSIBLE). 0 refuted.
| * Like `call`, but logs the command with the given secrets masked, so | ||
| * tokens and keys do not land in the CI build log. | ||
| */ | ||
| function callRedacted(cmdstring: string, secrets: string[]): void { |
There was a problem hiding this comment.
Secrets still reach the CI log on failure (CONFIRMED)
callRedacted only masks the secret in the console.log line. The raw cmdstring, with the plaintext token and channel key, is still passed straight into childProcess.execSync, so it does not actually stop secrets from reaching CI logs.
When the Zealot upload curl fails (bad token, network blip, or the 3600000ms timeout), execSync throws a Node Error whose message is Command failed: <cmdstring> with the unredacted URL embedded. That exception is not caught anywhere in buildCommonPost, so it propagates to the top level and Node prints the full stack trace, including the plaintext zealotApiToken and zealotChannelKey, to the CI console.
This defeats the purpose the helper was introduced for, per its own doc comment: "so tokens and keys do not land in the CI build log".
There was a problem hiding this comment.
Fixed in 078bc7b. callRedacted now runs the command inside a try/catch and
re-throws with the same masking applied to the error message, so the
Command failed: <cmd> text that execSync embeds no longer carries the token or
channel key into the CI stack trace. Verified against a deliberately failing
curl: the raw execSync error prints token=<the real token>, the wrapped one
prints token=<redacted>, and the secret appears nowhere in the stack.
| variant.outputs.each { output -> | ||
| def abi = output.getFilter(OutputFile.ABI) | ||
| if (abi != null) { | ||
| output.versionCodeOverride = |
There was a problem hiding this comment.
Per-ABI versionCode offset breaks build-number comparisons (CONFIRMED)
The per-ABI versionCode offset (+1 armeabi-v7a, +2 arm64-v8a) makes getBuildNumber() diverge from the actual release build number for users on sideloaded split APKs, breaking every call site that assumes buildNumber matches the tracked release build.
Concretely: an ops engineer configures an info/promo card with exactBuildNum (or minBuildNum/maxBuildNum) equal to the release's real build number from release-version.json/CHANGELOG/Zealot. src/util/infoUtils.ts:88 does if (exactBuildNum != null && exactBuildNum !== buildNumber) continue, comparing that configured value against react-native-device-info's getBuildNumber().
Universal-APK users match and see the card as intended, but any user who sideloaded the arm64-v8a or armeabi-v7a split APK reports buildNumber+2 or +1, so the exact-match check silently fails and the card (which could be a critical warning or forced-update notice) never renders for them, with no error surfaced anywhere.
There was a problem hiding this comment.
will not fix - the play store doesn't allow separate uploads to share a build number. Card will need to be created using build number min/max ranges or multiple cards targeting different exact build numbers.
|
|
||
| // Compress dex, matching how the universal APK that bundletool | ||
| // derives from the AAB is packaged. AGP stores dex uncompressed | ||
| // by default at minSdk 28+, which adds ~28 MB to a sideloaded APK: |
There was a problem hiding this comment.
dex.useLegacyPackaging applies to every variant, not just ABI-split builds (CONFIRMED)
This is placed in the top-level packagingOptions block, which applies to every build variant, not just the opt-in ABI-split release builds the surrounding comments describe.
Unlike the splits { abi { enable abiSplitsEnabled } } block a few lines above (deliberately gated so "normal builds are unchanged" when -PabiSplits is not passed), packagingOptions.dex is unconditional. Every local debug build, every branch's normal release build, and every maestro test APK now gets legacy (compressed) dex packaging even though nobody asked for the ABI-split feature on those builds, silently changing install-time dex extraction behavior and APK layout repo-wide instead of only for the master branch's opt-in split APKs.
There was a problem hiding this comment.
The trade off is taking a few extra milliseconds to install the app the first time or add a bunch of complexity to the build.gradle. I opted for simpler build.gradle
| if (typeof previousBuild !== 'number') | ||
| throw new Error(`Invalid previous buildNum ${previousBuild}`) | ||
| build = Math.max(previousBuild + 1, newBuildNum) | ||
| // Advance by 3, not 1, so each build owns a block of three |
There was a problem hiding this comment.
The +3 stride applies to every branch and platform, and can roll the day field (CONFIRMED)
The build-number stride was raised from +1 to +3 for every branch's counter, not just the one branch (master) that actually builds ABI splits. That reduces every branch's same-day build capacity and lets the day-encoded build number roll into the next day's numeric range on high-cadence branches.
pickBuildNumber() encodes the day in a fixed 2-digit field (day*100+counter). On a branch like develop that can get more than 33 CI builds in one day, incrementing by 3 each time pushes the counter past 99, so day*100+100 numerically equals (day+1)*100+0. A build actually cut on day N gets a build number that reads as day N+1's build 0 anywhere it is decoded or displayed (release notes, Zealot upload notes, CHANGELOG), even though that branch never uses the per-ABI versionCode offsets this stride exists to protect.
Same root cause also at scripts/gitVersionFile.ts:101.
There was a problem hiding this comment.
Keeping the +3 stride, with the reasoning now written into the code (891ab6c)
instead of left implicit.
The stride can't be traded for a separate numeric range for the splits
(build * 10 + offset with +1 here): the next build's universal APK would then
carry a lower versionCode than the previous build's split APKs, which Play treats
as a downgrade for that APK, and the universal has to keep reporting the plain
build number because getBuildNumber() returns the versionCode and infoUtils
string-compares it against min/max/exactBuildNum. A block of >= 3 codes per build
is the smallest scheme that satisfies both constraints.
On the day-field roll: the bleed isn't new, +3 only moves the threshold from 99
same-day builds on a branch to 33. When it does happen the numbers stay unique
and strictly increasing (Math.max(previousBuild + 3, newBuildNum)), so nothing
that depends on ordering or uniqueness breaks — only the date reading of a
display string is off, and only past 33 builds in one day on one branch. iOS
skipping by 3 is cosmetic for the same reason. Both costs are now spelled out in
the comment.
|
|
||
| // The gradle splits block can only emit these ABIs, so anything else | ||
| // in the config is a typo. Checking up front turns that into a clear | ||
| // error instead of a low-signal ENOENT from the copy below, and keeps |
There was a problem hiding this comment.
ABI validation runs after the gradle build, not before it (CONFIRMED)
The comment claims an up-front check, but the validation for splitArchitectures runs after the build.
With a typo'd ABI in a branch's deploy-config.json (e.g. "arm64" instead of "arm64-v8a"), buildAndroid() calls ./gradlew assembleRelease -PabiSplits first, a multi-minute release build, and only after it completes does the supportedAbis.includes(abi) loop run and throw Unsupported abi "arm64" in splitArchitectures. The deploy still fails, but only after burning the full build time this check exists to avoid, so a Jenkins release pipeline wastes CI minutes on every abi-typo deploy attempt instead of erroring immediately.
Same root cause also at scripts/deploy.ts:548.
There was a problem hiding this comment.
Fixed in 078bc7b: the supportedAbis loop now runs before
./gradlew assembleRelease -PabiSplits, so an abi typo fails immediately instead
of after a multi-minute assemble. The comment was right about the intent and
wrong about the placement; it now describes what the code actually does.
| def abi = output.getFilter(OutputFile.ABI) | ||
| if (abi != null) { | ||
| output.versionCodeOverride = | ||
| variant.versionCode + abiVersionOffsets.get(abi, 0) |
There was a problem hiding this comment.
Map.get(key, default) inserts the default into the map (PLAUSIBLE)
abiVersionOffsets.get(abi, 0) uses Groovy's Map.get(key, defaultValue) GDK method, whose implementation is if (!map.containsKey(key)) map.put(key, defaultValue); return map.get(key). It mutates the map on a miss, unlike Java's getOrDefault.
This is dormant today because the only non-null ABI filters (armeabi-v7a, arm64-v8a) already exist as keys. But if a future edit adds a third ABI to splits.abi.include (e.g. x86_64) without adding a matching abiVersionOffsets entry, this call silently writes a 0 offset for it, giving that split APK the same versionCode as the universal APK. Play Console's multi-APK upload (or the Zealot upload of both files) would then reject the release for a duplicate versionCode, with no compile-time or obvious runtime signal pointing here.
There was a problem hiding this comment.
Fixed in 891ab6c. Switched to the subscript form (abiVersionOffsets[abi]),
which is a plain get with no insert, and — more to the point — an ABI with no
offset now throws a GradleException instead of silently defaulting to 0 and
duplicating the universal APK's versionCode. Verified by temporarily dropping
arm64-v8a from the map: ./gradlew :app:tasks -PabiSplits fails with
No versionCode offset defined for ABI 'arm64-v8a'. With the map intact, both
configuration modes still configure cleanly.
| // in the config is a typo. Checking up front turns that into a clear | ||
| // error instead of a low-signal ENOENT from the copy below, and keeps | ||
| // unvalidated config values out of the path construction: | ||
| const supportedAbis = ['arm64-v8a', 'armeabi-v7a'] |
There was a problem hiding this comment.
ABI allowlist duplicates the gradle include list (CONFIRMED, cleanup)
android/app/build.gradle's splits.abi.include 'armeabi-v7a', 'arm64-v8a' and this const supportedAbis = ['arm64-v8a', 'armeabi-v7a'] encode the same set independently, with no shared source of truth.
If a future change adds a third ABI (e.g. x86_64) to the gradle include list but this allowlist is not updated in the same PR, every branch that adds that ABI to splitArchitectures gets rejected with "Unsupported abi" even though gradle would happily build it, and diagnosing it means tracing the mismatch across two unrelated files.
There was a problem hiding this comment.
There's no clean shared source of truth between a Groovy build file and a TS
deploy script, and deriving the list at runtime (globbing the output dir) would
give up the up-front validation the ordering finding above asks for. Left as two
lists, with a cross-reference comment added on both sides (2a10014, 078bc7b)
so adding an ABI points at the include list, the abiVersionOffsets map, and
supportedAbis together.
| * Like `call`, but logs the command with the given secrets masked, so | ||
| * tokens and keys do not land in the CI build log. | ||
| */ | ||
| function callRedacted(cmdstring: string, secrets: string[]): void { |
There was a problem hiding this comment.
callRedacted duplicates the entire execSync options block from call (CONFIRMED, cleanup)
call (line 814) and callRedacted both hardcode the same five-key options object (encoding: 'utf8', timeout: 3600000, stdio: 'inherit', cwd: _currentPath, killSignal: 'SIGKILL').
If the timeout or another execution option ever needs to change (say, raising it for a slower CI runner), only one of the two is likely to get updated, silently reintroducing the old behavior at whichever call site was missed. Extracting a shared execCommand(cmdstring) helper called by both, with callRedacted adding only its masked console.log, would remove the duplication.
There was a problem hiding this comment.
Fixed in 078bc7b: call() and callRedacted() now both go through a shared
execCommand() that owns the options object. callRedacted adds only the masked
log line and the masked error.
| // downloads. Only branches whose config block lists splitArchitectures | ||
| // build these at all, and maestro builds never do: | ||
| if ( | ||
| zealotApiToken != null && |
There was a problem hiding this comment.
branch and gitCommit are re-encoded, duplicating lines 620-621 (CONFIRMED, cleanup)
const branch = encodeURIComponent(buildObj.repoBranch) and const gitCommit = encodeURIComponent(buildObj.guiHash) recompute the exact expressions already bound at lines 620-621 in the same function.
It costs nothing at runtime, but a future change to how branch/commit are encoded (e.g. adding a fallback for an empty guiHash) has to be made in two places, and it is easy to update one and miss the other, leaving the two upload paths silently inconsistent.
There was a problem hiding this comment.
Fixed in 078bc7b: branch and gitCommit are computed once above both upload
blocks. token stays per-block, since it depends on the zealotApiToken
null-narrowing in each condition.
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (2)
android/app/build.gradle:119
abiSplitsEnabledtreats the Gradle property value as a string, so if the property is ever set programmatically as a boolean (e.g.abiSplits = false),project.property('abiSplits') != 'false'will still evaluate truthy and unexpectedly enable ABI splits. Converting the property to a string (or usingfindProperty) makesfalse/"false" behave consistently and matches the comment that-PabiSplits=falsedisables the feature.
def abiSplitsEnabled = project.hasProperty('abiSplits') &&
project.property('abiSplits') != 'false'
scripts/deploy.ts:834
callRedactedrethrows failures as a newError, which drops the original stack trace and useful execSync fields (like exit status/signal). Since this helper is intended for operational use in CI, it’s better to keep the original error object while still redacting secrets from its message.
try {
execCommand(cmdstring)
} catch (error) {
const message = error instanceof Error ? error.message : String(error)
throw new Error(redact(message))
}
078bc7b to
142c205
Compare
A build with -PabiSplits emits three APKs: the universal (same size and lib set as a default build, arm ABIs only) plus one sideloadable APK per ABI, each roughly 30 MiB smaller than the universal. The property is read as a boolean flag, so -PabiSplits=false disables the feature from the command line, gradle.properties, or CI. Builds without the property are completely unchanged. ndk.abiFilters stays active in both modes, which keeps Intel ABIs out of every artifact. AGP 8.8 allows abiFilters to coexist with splits.abi, so no filter juggling is needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Branches opt in through an optional splitArchitectures list in their android deploy-config block, mapping each ABI to the Zealot channel that receives it. Configured branches run assembleRelease -PabiSplits after the existing bundle step (the gradle daemon reuses the compile work, so this only pays for packaging and signing) and archive the per-ABI APKs next to the AAB and universal APK. ABI names are checked against the ABIs the gradle splits block can actually emit, so a config typo fails with a clear error instead of a stray file path. Both Zealot uploads, the existing universal one and the new per-ABI ones, log their curl commands through a redacting helper and URL encode the credentials, keeping the token and channel keys out of the CI build log. One channel per ABI keeps each channel's latest build on the right architecture. The universal APK keeps serving the main channel, the rsync location, and direct downloads. Google Play is unaffected: it already serves per-ABI installs from the AAB. Maestro builds and unconfigured branches skip the feature entirely. Entries without a zealotChannelKey are archived but not uploaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Google Play multi-APK uploads require a unique versionCode per APK, which the split APKs would otherwise share with the universal one. The split outputs now add a per-ABI offset: armeabi-v7a +1, arm64-v8a +2. Play serves the highest compatible code, so 64-bit devices get the arm64 APK, and the universal APK keeps the plain build number as the lowest-priority fallback. The offsets stay this small on purpose. getBuildNumber() returns the versionCode on Android, and the info server filters promo and info cards by string-comparing minBuildNum/maxBuildNum/exactBuildNum against it, so widening the number would have desynced Android from iOS and stopped every existing eight-digit exactBuildNum rule from matching. The universal APK that nearly every user installs keeps reporting the plain build number, unchanged. To keep the offsets from ever colliding with a neighboring build's codes, the build counter now advances by 3 per build, giving every build its own block of three versionCodes. Google Play rejects any versionCode it has ever seen, so this matters as soon as two Play releases are cut close together. Same-day capacity drops from 99 builds to 33, and iOS build numbers, which share the counter, simply skip by 3. Verified locally with aapt2: a split build produces base, base+1, and base+2, and a default build keeps the plain base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AGP stores dex uncompressed by default at minSdk 28+, but the universal APK that the deploy script derives from the AAB with bundletool compresses it. The split APKs were therefore carrying 27.7 MB of uncompressed dex that the artifact they are meant to replace does not carry, which hid almost all of the ABI saving: the first real cheese build produced a 115 MB universal against a 111.8 MB arm64 APK, a 3 MB win instead of the expected 30 MB. Compressing dex in APK packaging lines the two up. Measured on the same tree: universal 115.0 MB (unchanged, now matches the bundletool one) arm64-v8a 84.1 MB (was 111.8 MB) -30.9 MB vs universal armeabi-v7a 81.3 MB (was 109.0 MB) -33.6 MB vs universal The AAB is byte for byte identical either way, so Google Play delivery is unaffected. Direct downloads already received compressed dex, since they get the bundletool universal, so this changes no artifact any user currently installs; it only stops the new split APKs from being packaged differently from their sibling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
makeProject logged the entire merged buildObj, printing every secret in deploy-config.json (the Zealot API token and channel keys, the keystore password, and the upload service credentials) into the CI build log on every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
142c205 to
78adcb7
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- deploy-config.sample.json
Suppressed comments (2)
scripts/gitVersionFile.ts:95
- In this comment,
app/build.gradleis ambiguous from the scripts/ context (the actual file path in the repo isandroid/app/build.gradle). Using the full path avoids confusion when someone searches for the referenced logic.
// versionCodes: the Android split APKs add per-ABI offsets to the
// build number (universal +0, armeabi-v7a +1, arm64-v8a +2, see
// app/build.gradle), and Google Play rejects any versionCode it
// has ever seen, so consecutive builds must never overlap blocks.
scripts/deploy.ts:834
callRedactedrethrows a brand-newError, which discards useful diagnostic fields from the originalexecSyncerror (exit status, signal, and stack). It’s safer to redact the existing error message and rethrow the original error so CI failures stay debuggable while still masking secrets.
try {
execCommand(cmdstring)
} catch (error) {
const message = error instanceof Error ? error.message : String(error)
throw new Error(redact(message))
}
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Requirements
If you have made any visual changes to the GUI. Make sure you have:
No visual changes: build pipeline only.
Description
Adds per-ABI Android APKs to release builds, distributed through dedicated Zealot channels and driven entirely by deploy-config, with a versionCode scheme that supports uploading them to Google Play as a multi-APK release (Edge uploads self-signed APKs rather than an AAB, since Play App Signing would require sharing the signing key with Google). Direct downloads keep the universal APK.
Measured on a real Jenkins build of this branch (test-gouda cheese build):
The split APKs compress dex the same way the bundletool-derived universal does (AGP stores dex uncompressed at minSdk 28+), which is where most of the saving comes from being preserved; the AAB is hash-identical with and without that packaging change, so Google Play delivery is untouched.
How it works:
app/build.gradlegains asplits.abiblock gated behind-PabiSplits, withndk.abiFiltersuntouched and always active. A split build emits three APKs: the universal (same size and lib set as a default build, arm ABIs only, verified) plus one per ABI, so the universal stays available across the board. Default invocations (bundleRelease,npm run android:release*, dev builds) are completely unchanged.Branches opt in through an optional
splitArchitectureslist in their android deploy-config block, mapping each ABI to the Zealot channel that receives it:Branch scoping comes from the config file, so the deploy script itself stays branch-agnostic. Branches without the field, and all maestro builds, skip the feature entirely. Entries without a
zealotChannelKeyare archived but not uploaded.For configured branches,
deploy.tsrunsassembleRelease -PabiSplitsafter the existing bundle step (the gradle daemon reuses the compile work, so this only pays for packaging and signing) and archives<outfile>-arm64-v8a.apkand<outfile>-armeabi-v7a.apknext to the AAB and universal APK, with the same signing config, so cross-updates between any of these artifacts keep working.One Zealot channel per ABI means each channel's latest build is always the right architecture. The universal APK keeps uploading to the existing channel and the rsync location for direct downloads.
Split APKs add per-ABI versionCode offsets (armeabi-v7a +1, arm64-v8a +2) for Google Play multi-APK uploads, which require a unique versionCode per APK. Play serves the highest compatible code, so 64-bit devices get the arm64 APK while the universal keeps the plain build number as the lowest-priority fallback. The build counter advances by 3 per build (gitVersionFile.ts), giving every build a disjoint block of three codes so offsets never collide at any release cadence. Codes stay eight-digit, so the info server's string-compared minBuildNum/maxBuildNum/exactBuildNum rules keep matching, and iOS keeps the raw build number.
deploy-config.sample.jsondocuments the new field and the previously undocumented Zealot fields.Verified: both gradle configuration modes pass, each split APK contains exactly one
lib/<abi>/tree (24 .so files each), the split-mode universal matches the default build's size and contains only the two arm ABI trees, versionCodes come out as base/base+1/base+2 on a split build and plain base on a default build (aapt2-checked),updateVersion.tsis untouched, so the universal APK and the AAB keep reporting the plain build number, and lint plus the test suite pass.Ops steps to activate: create one Zealot channel per ABI in the Zealot admin, then add the
splitArchitectureslist to the master block ofdeploy-config.jsonon the build machine.Note
Medium Risk
Touches release signing, versionCode allocation, and CI deploy/upload paths; mistakes could break Play uploads or leak credentials in logs, though behavior is opt-in and default builds are unchanged.
Overview
Adds optional per-ABI Android release APKs (arm64-v8a and armeabi-v7a) for sideloading outside Google Play, while the universal APK still goes to the main Zealot channel and direct downloads. Branches opt in via
splitArchitecturesin deploy-config, each ABI mapped to its own Zealot channel.Gradle (
android/app/build.gradle): ABI splits are gated by-PabiSplits(off by default). Split builds emit universal plus per-ABI APKs, with per-ABIversionCodeoffsets (+1 / +2) for Play multi-APK rules. Dex legacy packaging is enabled to shrink sideload APKs without changing the AAB.Deploy (
scripts/deploy.ts): After the existing bundle/universal step, configured branches runassembleRelease -PabiSplits, archive split APKs, and upload each to its channel. Zealot uploads usecallRedactedso tokens/keys are masked in CI logs.Build numbers (
scripts/gitVersionFile.ts): The counter advances by 3 per build so universal and splitversionCodes never collide across releases; the universal APK still reports the plain build number for info-server rules.CHANGELOG and
deploy-config.sample.jsondocument the new option.Reviewed by Cursor Bugbot for commit 78adcb7. Bugbot is set up for automated code reviews on this repo. Configure here.