ADFA-2778: ensure binaries in build-tools are executable after installation#916
ADFA-2778: ensure binaries in build-tools are executable after installation#916itsaky-adfa merged 4 commits intostagefrom
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
📝 WalkthroughRelease Notes - ADFA-2778: Ensure Binaries in Build-Tools Are Executable After InstallationOverviewThis PR enhances the build-tools installation process to ensure that multiple binary executables have the correct permissions after installation, improving reliability of the build process. Key Changes
|
| Cohort / File(s) | Summary |
|---|---|
Build-tools Configuration common/src/main/java/com/itsaky/androidide/utils/Environment.java |
Add BUILD_TOOLS_VERSION = "35.0.0" and BUILD_TOOLS_DIR field; compute tool paths from BUILD_TOOLS_DIR and update AAPT2 reference to use it. |
Assets installer / NDK & binaries app/src/main/java/com/itsaky/androidide/assets/BaseAssetsInstaller.kt |
Replace single aapt2 toggle with loop setting executables for aapt, aapt2, aidl, dexdump, split-select, zipalign. Refactor installNdk to accept archiveFile and outputDir, add experiments-gate early return, add archive existence check and debug log, retain tar extraction, PATH update, logging, and archive cleanup; adjust postInstall to iterate binaries and call new installNdk signature. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
- ndk installation #880: Modifies BaseAssetsInstaller NDK installation flow and Environment NDK/build-tools fields—strong code-level overlap.
- ADFA-2179 ndk template #882: Adjusts BaseAssetsInstaller.installNdk including experiments gating—directly related to this PR's NDK refactor.
Suggested reviewers
- davidschachterADFA
Poem
🐇 I hopped through builds at break of dawn,
Marked aapt and friends so workflows run on,
Pulled out the NDK, untarred with a grin,
Cleaned up the archive, tossed it in bin,
Now tools are ready — time for a sprint!
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and specifically describes the main change: ensuring build-tools binaries are executable after installation, which aligns with the core modifications to BaseAssetsInstaller.kt and Environment.java. |
| Description check | ✅ Passed | The description references ADFA-2778, which is related to the changeset's objective of ensuring binaries in build-tools are executable after installation. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
fix/ADFA-2778
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/assets/BaseAssetsInstaller.kt`:
- Around line 17-32: The loop in BaseAssetsInstaller.postInstall currently calls
Path.setExecutable(...) and ignores its boolean result so failures are silent;
replace those calls with the existing helper Environment.setExecutable(...)
which logs failures. Update the loop that iterates over the build-tools names
("aapt","aapt2","aidl","dexdump","split-select","zipalign") to call
Environment.setExecutable(Environment.BUILD_TOOLS_DIR.resolve(bin)) for each bin
(keeping the same bin list) so missing files or permission errors are reported.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/assets/BaseAssetsInstaller.kt (1)
58-66: Avoidbash -cto prevent shell parsing/quoting issues.Build the
ProcessBuilderwith explicit arguments instead of a shell command string; this is safer and handles paths with spaces correctly.♻️ Suggested refactor
- val processBuilder = - ProcessBuilder( - "${TermuxConstants.TERMUX_BIN_PREFIX_DIR_PATH}/bash", - "-c", - "tar -xJf ${archiveFile.absolutePath} -C ${outputDir.absolutePath} --no-same-owner", - ).redirectErrorStream(true) + val processBuilder = + ProcessBuilder( + "tar", + "-xJf", + archiveFile.absolutePath, + "-C", + outputDir.absolutePath, + "--no-same-owner", + ).redirectErrorStream(true)
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/assets/BaseAssetsInstaller.kt`:
- Line 65: In BaseAssetsInstaller.postInstall, wrap the call to
processBuilder.start() in a try-catch for IOException so the method logs the
exception and returns false instead of letting the exception propagate; either
move the try-catch inside the existing measureTimeMillis block (so you still
capture elapsed time) or wrap the entire measureTimeMillis call and record
elapsed accordingly, then on catch call the existing logger (or processLogger)
with the error details and return false to match other failure paths.
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/assets/BaseAssetsInstaller.kt (2)
82-96: Successful extraction returnsfalseif archive deletion fails.When
exitCode == 0, the return value on line 92 isdeleted, so a deletion failure causes the method to report overall failure despite the NDK being successfully extracted. This conflates extraction outcome with cleanup outcome. Consider always returningtrueafter successful extraction and just logging the deletion failure.♻️ Proposed change
return if (exitCode == 0) { logger.debug("Extraction of ${archiveFile.absolutePath} successful took ${elapsed}ms : $result") if (archiveFile.exists()) { val deleted = archiveFile.delete() if (deleted) { logger.debug("${archiveFile.absolutePath} deleted successfully.") } else { - logger.debug("Failed to delete ${archiveFile.absolutePath}.") + logger.warn("Failed to delete ${archiveFile.absolutePath}.") } - deleted - } else { - logger.debug("Archive file not found for deletion.") - false } + true } else {
77-77: Consider adding a timeout toreader.join().After
destroyForcibly(), the reader thread should eventually terminate, but a bounded join (e.g.,reader.join(10_000)) would prevent an indefinite hang in edge cases.
2 participants
See ADFA-2778 for more details.