Open
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
📝 Walkthrough
WalkthroughRefactors Gradle build scripts: rewrites D8 task invocation and asset packaging to use explicit paths, streaming AAR repackaging, and checksum-backed CDN/fallback downloads; reflows Gradle DSL formatting and adds LeakCanary dependency. No public API signature changes. Changes
Sequence DiagramsequenceDiagram
participant GradleTask as Gradle Task\n(createAssetsZip)
participant FileSystem as File System
participant CDN as CDN/HTTP
participant D8 as D8 Tool
participant ZipOutput as ZIP Output
GradleTask->>FileSystem: normalize buildDirectory & create temp dirs
GradleTask->>CDN: download required AAR/JAR assets (with checksum)
CDN-->>GradleTask: asset streams / fallbacks
GradleTask->>FileSystem: explode AARs & collect classpath jars
GradleTask->>D8: invoke with explicit --output and per-dependency inputs
D8-->>GradleTask: produce classes.dex
GradleTask->>ZipOutput: stream original AAR contents
GradleTask->>ZipOutput: insert/replace classes.dex in nested AAR location
ZipOutput-->>FileSystem: write final assets-*.zip
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/build.gradle.kts`:
- Around line 542-558: The inner ZipOutputStream created as
ZipOutputStream(zipOut).use { aarZipOut -> ... } will call close() which closes
the underlying zipOut and corrupts the outer ZIP; change the inner handling so
it does not close the underlying stream — e.g., stop using .use on the inner
ZipOutputStream and after writing entries call aarZipOut.finish() (not close())
before returning control, ensuring you still close the outer ZipOutputStream
only once; update the block that writes entries from originalLlamaAarFile and
the classes.dex write (references: ZipOutputStream, aarZipOut,
originalLlamaAarFile, dexOutputFile) accordingly.
🧹 Nitpick comments (3)
app/build.gradle.kts (3)
1148-1177: Redundantreturnon line 1165.The outer
if/elseon line 1149 already returns its value. The innerreturn try { … }on line 1165 is redundant — it works, but adds confusion. Removereturnand let thetryexpression be theelsebranch value.Proposed fix
} else { val checksumUrl = asset.url + ".md5" val conn = URL(checksumUrl).openConnection() as HttpURLConnection conn.requestMethod = "GET" conn.setRequestProperty("User-Agent", "Mozilla/5.0") conn.instanceFollowRedirects = true conn.connectTimeout = 10_000 conn.readTimeout = 10_000 - return try { + try { val status = conn.responseCode if (status == HttpURLConnection.HTTP_OK) { conn.inputStream.bufferedReader().use { it.readText().trim() } } else { throw GradleException("Failed to fetch checksum from $checksumUrl (HTTP $status: ${conn.responseMessage})") } } finally { conn.disconnect() } }
1075-1082: SCP command has no timeout — could hang indefinitely in CI.If the SCP server is unreachable or slow, this
execcall will block the CI pipeline with no way to time out gracefully. Consider adding a timeout viascp -o ConnectTimeout=30or wrapping the exec with a Gradle timeout.Proposed fix
project.exec { commandLine( "scp", + "-o", "ConnectTimeout=30", "-r", "$scpServer:public_html/dev-assets/$variant/", tmpDir.absolutePath, ) }
1179-1230: Download writes directly to target path — partial file risk on failure.If the HTTP download in
assetsFileDownloadfails mid-stream (network error, timeout), the target file is left in a partial/corrupt state. While the checksum validation will catch this and throw, the corrupt file persists on disk and could cause confusion.Consider downloading to a temp file first, then atomically moving it to the target path after checksum validation passes.
Comment on lines
+542
to
+558
| ZipOutputStream(zipOut).use { aarZipOut -> | ||
| // Copy all files from the original AAR *except* classes.jar | ||
| ZipInputStream(originalLlamaAarFile.inputStream()).use { originalAarStream -> | ||
| var entry = originalAarStream.nextEntry | ||
| while (entry != null) { | ||
| if (entry.name != "classes.jar") { | ||
| aarZipOut.putNextEntry(ZipEntry(entry.name)) | ||
| originalAarStream.copyTo(aarZipOut) | ||
| aarZipOut.closeEntry() | ||
| } | ||
| entry = originalAarStream.nextEntry | ||
| } | ||
| } | ||
| aarZipOut.putNextEntry(ZipEntry("classes.dex")) | ||
| dexOutputFile.inputStream().use { dexInput -> dexInput.copyTo(aarZipOut) } | ||
| aarZipOut.closeEntry() | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: nested ZipOutputStream.use will close the outer zip stream prematurely.
ZipOutputStream(zipOut).use { aarZipOut -> ... } — when use calls aarZipOut.close(), it propagates through DeflaterOutputStream.close() → out.close(), which closes zipOut. This means the outer ZipOutputStream can never write its central directory, resulting in a corrupt ZIP file.
Use finish() instead of close() for the inner stream to write the ZIP trailer without closing the underlying stream.
Proposed fix
- ZipOutputStream(zipOut).use { aarZipOut ->
+ val aarZipOut = ZipOutputStream(zipOut)
+ try {
// Copy all files from the original AAR *except* classes.jar
ZipInputStream(originalLlamaAarFile.inputStream()).use { originalAarStream ->
var entry = originalAarStream.nextEntry
while (entry != null) {
if (entry.name != "classes.jar") {
aarZipOut.putNextEntry(ZipEntry(entry.name))
originalAarStream.copyTo(aarZipOut)
aarZipOut.closeEntry()
}
entry = originalAarStream.nextEntry
}
}
aarZipOut.putNextEntry(ZipEntry("classes.dex"))
dexOutputFile.inputStream().use { dexInput -> dexInput.copyTo(aarZipOut) }
aarZipOut.closeEntry()
+ } finally {
+ aarZipOut.finish() // writes ZIP central directory without closing zipOut
}
+ zipOut.closeEntry() // close the "dynamic_libs/llama.aar" entry📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ZipOutputStream(zipOut).use { aarZipOut -> | |
| // Copy all files from the original AAR *except* classes.jar | |
| ZipInputStream(originalLlamaAarFile.inputStream()).use { originalAarStream -> | |
| var entry = originalAarStream.nextEntry | |
| while (entry != null) { | |
| if (entry.name != "classes.jar") { | |
| aarZipOut.putNextEntry(ZipEntry(entry.name)) | |
| originalAarStream.copyTo(aarZipOut) | |
| aarZipOut.closeEntry() | |
| } | |
| entry = originalAarStream.nextEntry | |
| } | |
| } | |
| aarZipOut.putNextEntry(ZipEntry("classes.dex")) | |
| dexOutputFile.inputStream().use { dexInput -> dexInput.copyTo(aarZipOut) } | |
| aarZipOut.closeEntry() | |
| } | |
| val aarZipOut = ZipOutputStream(zipOut) | |
| try { | |
| // Copy all files from the original AAR *except* classes.jar | |
| ZipInputStream(originalLlamaAarFile.inputStream()).use { originalAarStream -> | |
| var entry = originalAarStream.nextEntry | |
| while (entry != null) { | |
| if (entry.name != "classes.jar") { | |
| aarZipOut.putNextEntry(ZipEntry(entry.name)) | |
| originalAarStream.copyTo(aarZipOut) | |
| aarZipOut.closeEntry() | |
| } | |
| entry = originalAarStream.nextEntry | |
| } | |
| } | |
| aarZipOut.putNextEntry(ZipEntry("classes.dex")) | |
| dexOutputFile.inputStream().use { dexInput -> dexInput.copyTo(aarZipOut) } | |
| aarZipOut.closeEntry() | |
| } finally { | |
| aarZipOut.finish() // writes ZIP central directory without closing zipOut | |
| } | |
| zipOut.closeEntry() // close the "dynamic_libs/llama.aar" entry |
🤖 Prompt for AI Agents
In `@app/build.gradle.kts` around lines 542 - 558, The inner ZipOutputStream
created as ZipOutputStream(zipOut).use { aarZipOut -> ... } will call close()
which closes the underlying zipOut and corrupts the outer ZIP; change the inner
handling so it does not close the underlying stream — e.g., stop using .use on
the inner ZipOutputStream and after writing entries call aarZipOut.finish() (not
close()) before returning control, ensuring you still close the outer
ZipOutputStream only once; update the block that writes entries from
originalLlamaAarFile and the classes.dex write (references: ZipOutputStream,
aarZipOut, originalLlamaAarFile, dexOutputFile) accordingly.
dara-abijo-adfa
approved these changes
Feb 17, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/build.gradle.kts`:
- Line 483: The current call
d8Command.add(android.defaultConfig.minSdk.toString()) can pass the literal
"null" because android.defaultConfig.minSdk is Int?; update this to validate and
handle null explicitly: either requireNotNull(android.defaultConfig.minSdk) with
a clear error message to fail fast, or use the Elvis operator
(android.defaultConfig.minSdk ?: throw IllegalStateException("minSdk not set"))
and then add the integer's string value to d8Command; ensure you reference the
existing d8Command.add(...) call and replace the .toString() usage with the
null-checked value.
- Around line 1207-1213: The code inconsistently uses rootProject.projectDir
when calling assetsFileDownload but computes the checksum on target (which is
derived from the projectDir parameter), risking mismatched files; update the
assetsFileDownload call to use the same projectDir-derived File (i.e.,
File(projectDir, asset.localPath)) or otherwise ensure the checksum and download
both operate on the same File instance (refer to assetsFileDownload, projectDir,
rootProject.projectDir, and target/MessageDigest usage) so the downloaded file
and subsequent MD5 calculation reference the identical path.
---
Duplicate comments:
In `@app/build.gradle.kts`:
- Around line 542-559: The nested ZipOutputStream(zipOut).use { aarZipOut -> ...
} wrapper closes the underlying zipOut and corrupts the outer ZIP; replace the
.use block for the inner ZipOutputStream with manual resource management: create
aarZipOut = ZipOutputStream(zipOut) (do not use .use), run the existing copy
loop and entries, then call aarZipOut.finish() and aarZipOut.flush() in a
finally block without calling close() so zipOut remains open; reference the
ZipOutputStream instance named aarZipOut and the outer stream zipOut when making
this change.
| d8Command.add(d8Executable.absolutePath) | ||
| d8Command.add("--release") | ||
| d8Command.add("--min-api") | ||
| d8Command.add(android.defaultConfig.minSdk.toString()) // Add minSdk for better desugaring |
Contributor
There was a problem hiding this comment.
minSdk could be null, producing "null" as the --min-api value.
android.defaultConfig.minSdk returns Int?. If unset, .toString() yields the string "null", causing a confusing D8 failure. Consider adding a null check or using minSdk!! to fail fast with a clear message.
Proposed fix
- d8Command.add(android.defaultConfig.minSdk.toString())
+ d8Command.add((android.defaultConfig.minSdk ?: error("minSdk is not set")).toString())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| d8Command.add(android.defaultConfig.minSdk.toString()) // Add minSdk for better desugaring | |
| d8Command.add((android.defaultConfig.minSdk ?: error("minSdk is not set")).toString()) // Add minSdk for better desugaring |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/build.gradle.kts` at line 483, The current call
d8Command.add(android.defaultConfig.minSdk.toString()) can pass the literal
"null" because android.defaultConfig.minSdk is Int?; update this to validate and
handle null explicitly: either requireNotNull(android.defaultConfig.minSdk) with
a clear error message to fail fast, or use the Elvis operator
(android.defaultConfig.minSdk ?: throw IllegalStateException("minSdk not set"))
and then add the integer's string value to d8Command; ensure you reference the
existing d8Command.add(...) call and replace the .toString() usage with the
null-checked value.
Comment on lines
+1207
to
+1213
| if (needsDownload) { | ||
| project.logger.lifecycle("Downloading ${asset.url} → ${asset.localPath}") | ||
|
|
||
| assetsFileDownload(asset, File(rootProject.projectDir, asset.localPath)) | ||
| // Recompute checksum after download | ||
| val digest = MessageDigest.getInstance("MD5") | ||
| target.inputStream().use { input -> |
Contributor
There was a problem hiding this comment.
Inconsistent use of projectDir parameter vs hardcoded rootProject.projectDir.
Line 1210 passes File(rootProject.projectDir, asset.localPath) as the download target, but the checksum at line 1213 is computed on target which is File(projectDir, asset.localPath). If projectDir ever differs from rootProject.projectDir, the checksum will be computed on a stale or missing file.
Use the projectDir parameter consistently:
Proposed fix
- assetsFileDownload(asset, File(rootProject.projectDir, asset.localPath))
+ assetsFileDownload(asset, target)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/build.gradle.kts` around lines 1207 - 1213, The code inconsistently uses
rootProject.projectDir when calling assetsFileDownload but computes the checksum
on target (which is derived from the projectDir parameter), risking mismatched
files; update the assetsFileDownload call to use the same projectDir-derived
File (i.e., File(projectDir, asset.localPath)) or otherwise ensure the checksum
and download both operate on the same File instance (refer to
assetsFileDownload, projectDir, rootProject.projectDir, and target/MessageDigest
usage) so the downloaded file and subsequent MD5 calculation reference the
identical path.
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
2 participants
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.
See ADFA-2756 for more details.