fix(android): restores custom apk naming#410
Conversation
|
Thanks! |
|
Landed on main via cherry-pick + conflict resolution.
Notes: main had already removed the previous broken ApkVariantOutput approach; this reapplies APK output naming using VariantOutputImpl. |
Comprehensive Code Review (Retrospective)Status: ✅ Approved (already merged with improvements) This PR successfully fixed the critical Android build error from PR #402. Here's my analysis across 7 dimensions: 🔒 Security: PASS
🔄 Concurrency: PASS
🧪 Testing: WEAK
|
mkalkere
left a comment
There was a problem hiding this comment.
Detailed inline code review comments
| @@ -1,3 +1,5 @@ | |||
| import com.android.build.api.variant.impl.VariantOutputImpl | |||
There was a problem hiding this comment.
This import is from the .impl package, indicating it's not part of Android Gradle Plugin's public API. While this is currently the only way to set custom APK output filenames, it carries risks:
Risks:
- May break in future AGP versions without warning
- No API stability guarantees from Google
- Requires monitoring AGP release notes
Why necessary:
The public VariantOutput interface doesn't expose outputFileName as a mutable property.
Recommendation:
// NOTE: Uses internal VariantOutputImpl API due to lack of public API
// for setting outputFileName. Tested with AGP 8.7.x.
// TODO: Migrate to public API when AGP provides one
import com.android.build.api.variant.impl.VariantOutputImpl| onVariants { variant -> | ||
| variant.outputs | ||
| .filterIsInstance<VariantOutputImpl>() | ||
| .forEach { output -> |
There was a problem hiding this comment.
✅ Excellent Type Safety
This is a significant improvement over PR #402's approach which used unsafe casting.
.filterIsInstance<VariantOutputImpl>() // ✅ Safe filteringBenefits:
- Gracefully skips non-matching outputs
- More defensive than
as?cast - Fails fast if API changes
Performance: O(n) where n = output count (~2-4), negligible
| .filterIsInstance<VariantOutputImpl>() | ||
| .forEach { output -> | ||
| val versionName = output.versionName.get() | ||
| val buildType = variant.buildType |
There was a problem hiding this comment.
Original PR #410:
val versionName = output.versionName.get() // ❌ Can throwMerged to Main:
val versionName = output.versionName.orNull ?: "0" // ✅ SafeExcellent catch by @steipete! Consider logging when fallback triggers:
val versionName = output.versionName.orNull ?: run {
logger.warn("versionName null, defaulting to '0'")
"0"
}| val buildType = variant.buildType | ||
|
|
||
| val outputFileName = "clawdbot-${versionName}-${buildType}.apk" | ||
| output.outputFileName = outputFileName |
There was a problem hiding this comment.
✅ Clear APK Naming Convention
Format: openclaw-{version}-{buildType}.apk
Examples:
openclaw-2026.2.25-debug.apkopenclaw-2026.2.25-release.apk
Testability improvement:
fun apkFileName(version: String, type: String) =
"openclaw-${version}-${type}.apk"Makes naming logic unit-testable.
Summary
Edit: Custom apk naming is gone from main. This PR reintroduces this change.
Fixes the Android build configuration error introduced by 7f6b989 from #402.
The
ApkVariantOutputcast was failing. This PR usesVariantOutputImplto properly accessversionNameand setoutputFileNamedirectly.APK output filenames:
clawdbot-2026.1.7-debug.apkclawdbot-2026.1.7-release.apkTest plan