Skip to content

Conversation

@Taknok
Copy link

@Taknok Taknok commented Apr 19, 2025

Also include a CI with binary attestation and auto release.

@Taknok Taknok changed the title Bump to build-tools 35 Bump to build-tools 35.0.2 Apr 19, 2025
Copy link
Member

@oSumAtrIX oSumAtrIX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. However I have a couple of strong gripes that need to be addressed.

  • Why are we bumping to 35.0.2 specifically? Is there no way to target the latest Android has?
  • The README uses "I". You should communicate to us via GitHub, not the checked in readme.
  • The README, workflows and code style and license does not follow the previous one. Changing them is not part of the PRs aim.
  • The PR now makes this repo base on some other repo with different goals. The repos purpose is to build AAPT2 not other build tools.
  • Part of what belongs to the build system is put in CI, or part what belongs to GIT as the source manager is put in the build system. Separation of concern is important
  • The current readme should be kept as is or synced with our other repos readmes and their style and speech of figure.

@Taknok
Copy link
Author

Taknok commented Apr 21, 2025

Hi,
Thank for the very detailed review.
As a general approach, this repo is based on the work of lzhiyong at https://github.com/lzhiyong/android-sdk-tools. I did the less possible modification from the original repo in order to lesser the merge conflict that may comes in the futur. Here is the diff between my modification from lzhiyong https://github.com/Taknok/aapt2/compare/lzhiyong-master..main

Here are the diff made by ibotpeaches for the aapt2 used for apktools : aosp-mirror/platform_frameworks_base@main...iBotPeaches:platform_frameworks_base:apktool_14.0.0

Why are we bumping to 35.0.2 specifically? Is there no way to target the latest Android has?

Because build-tools were stop in 2023 to v34 https://developer.android.com/tools/releases/build-tools
Aapt2 modifications are tracking in platform-tools now, latest tag for platform-tools is the v35.0.2 https://android.googlesource.com/platform/manifest/+refs

The README uses "I". You should communicate to us via GitHub, not the checked in readme.

I agree, I only add the 1st line in the Readme as I did not wanted to modify it to avoid rebase/merge from future modifications in the lzhiyong repo. But if it is wanted to modify it, it is a totally understandable choice. Tell me if you want me to recreate/rephrase it.

The README, workflows and code style and license does not follow the previous one. Changing them is not part of the PRs aim.

I was in a quite tricky situation, as lzhiyong put his code under the apache v2 and you code is under the GPLv3. Here the GPLv3 can be applied to the repo and all created files/code, but the code from lzhiyong should remain under apache v2

The PR now makes this repo base on some other repo with different goals. The repos purpose is to build AAPT2 not other build tools.

I do not totally agree. I used the work of lzhiyong that aim to provide build-tools and platform-tools binaries for android but I striped all the repo to keep only what is used for aapt and aapt2. I add the modification of IbotPeaches in order to keep the best compatibility of aapt2 with the apktools libs.

Part of what belongs to the build system is put in CI, or part what belongs to GIT as the source manager is put in the build system. Separation of concern is important

I did not design the build system, and (as said previously I did not want to modify too much the repo to lesser merge/rebase conflict). We can choose to go back to submodule with a cleaner build.py or build.sh as before. I let you decide.

The current readme should be kept as is or synced with our other repos readmes and their style and speech of figure.

Cf response to point 2

As a general approach, it seems that you prefer to increase the diff with the lzhiyong repo in order to clean and standardize across revanced repo even if future rebase/merge will be a bit harder ?

@Taknok
Copy link
Author

Taknok commented Apr 25, 2025

What i propose:

  1. Source repo are submodules pinned to 35.0.2
  2. Patch is in a separate bash file as patch should be done once
  3. Build is in another bash and it targets one arch passed as argument because build could be done many times
  4. CI as closed as possible of revanced (ndk installation, protoc build etc differs)

Are those points ok ?

@oSumAtrIX
Copy link
Member

I do not totally agree. I used the work of lzhiyong that aim to provide build-tools and platform-tools binaries for android but I striped all the repo to keep only what is used for aapt and aapt2. I add the modification of IbotPeaches in order to keep the best compatibility of aapt2 with the apktools libs.

I suppose your PR has the same goal (build aapt2), but not izhiyongs repo. However, as you have modified their repo substantially, it may be easier to base of our repo instead of theirs, as our repo already has the same goal as your PR, while theirs follows another.

As a general approach, it seems that you prefer to increase the diff with the lzhiyong repo in order to clean and standardize across revanced repo even if future rebase/merge will be a bit harder ?

You understand the general change I would like to see in this PR. Basically, this repo was made from "scratch" with every line intended as is for the purpose of building AAPT2. I am not familiar with many lines from lzhiyong and reviewing that seems quite difficult, because I come across lines I either question or need changes a lot. I am wondering, how easy it is to work up to v35 from our repo. What prevents this? From my understanding, we need to bump the submodules to whatever v35 uses. Some compilation errors will surely occur that would be needed to look into, but then again lzhiyong's repo would come in clutch for reference in this regard. Once we can compile upstream AAPT2 v35 with our build system, we can look into the patches that make it work on Android and for ReVanced's purposes, which should be ibotpeaches patches (and ours such as the one that fix AAPT2 for arm32).

What i propose:

  1. Source repo are submodules pinned to 35.0.2
  2. Patch is in a separate bash file as patch should be done once
  3. Build is in another bash and it targets one arch passed as argument because build could be done many times
  4. CI as closed as possible of revanced (ndk installation, protoc build etc differs)

Are those points ok ?

Yep! Sounds perfect. Regarding 4, please let me know what changes you have in mind. I would like to understand why changes are needed in the first place.

@Taknok Taknok requested a review from oSumAtrIX May 1, 2025 21:01
@Taknok
Copy link
Author

Taknok commented Jun 19, 2025

Hi,
Just a gentle reminder about this PR that I opened a couple of month ago :)

semantic-release-bot and others added 7 commits August 18, 2025 13:56
# 1.0.0 (2025-08-18)

### Bug Fixes

* `basic_string` inline templates patch ([#9](#9)) ([b907424](b907424))
* Add basic_string inline templates to fix args segmentation fault ([1ad472f](1ad472f))
* Add ending newlines to fix corrupt aapt2 and androidfw patches ([93324ca](93324ca))
* **build:** add missing patch ([0f54c42](0f54c42))
* Specify minimum alignment for `ResStringPool_span` to fix SIGBUS on ARMv7 ([8285e73](8285e73))
* Typo on unzip option ([1adcce5](1adcce5))

### Features

* improve `aapt2.cmake` [skip ci] ([a2d190f](a2d190f))
* license file [skip ci] ([fa1b651](fa1b651))
* readme file [skip ci] ([2896964](2896964))
Copy link
Author

@Taknok Taknok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes were made or commented

@Taknok Taknok requested a review from oSumAtrIX August 20, 2025 15:18
@oSumAtrIX
Copy link
Member

Some reviews are still nto marked as resolved, please check them

@Taknok
Copy link
Author

Taknok commented Aug 21, 2025

protoc is now installed from the apt repository. I’ve marked the outdated reviews as resolved and replied to your comment.

@Taknok
Copy link
Author

Taknok commented Sep 30, 2025

Is there anything that I should do ? All review are closed except:

  1. The valicube feedback
  2. The approach of the build process (full aosp source with Soong or minimal cmake)

@Taknok
Copy link
Author

Taknok commented Oct 20, 2025

Please, could you indicate me what is blocking the merge of this PR ?

@Ushie
Copy link
Member

Ushie commented Oct 20, 2025

Hey, sorry for the wait

The validcube feedback is dismissed

Has "The approach of the build process (full aosp source with Soong or minimal cmake)" been decided? If so, I believe this is ready to go in terms of the reviews

@Taknok
Copy link
Author

Taknok commented Oct 20, 2025

No problem!

Ok.

The approach hasn’t been decided yet, as there hasn’t been any response. Here are the arguments I previously presented (also shared on Discord):

  1. Soong dependency loading:
    Soong starts by loading all Android.bp files referenced through includes before even checking the target binary. This means that if a required Android.bp—even one used only for a test unrelated to our binary—is missing, Soong will stop.
    I tried this approach by cloning only the base AOSP build-tools branch and compiling, then iterating as Soong complained about missing dependencies until it succeeded. In the end, only a handful of projects (fewer than five) could be excluded.
    Therefore, this approach would require creating many patches to modify Android.bp files and tweak the build system, which seems overly complicated and beyond the scope of this project.

  2. Hardware requirements:
    Building with Soong from the full AOSP source requires substantial hardware resources (e.g., large memory usage for Soong’s include processing). iBotPeaches builds on his personal computer, but using Soong in GitHub Actions isn’t feasible due to resource limits—even under the open-source organization plan.
    In contrast, using CMake allows us to build through GitHub Actions with attestation and automation, which is much more maintainable and transparent

@Taknok
Copy link
Author

Taknok commented Nov 30, 2025

Please ?

@oSumAtrIX
Copy link
Member

Using Soong is not really possible in actions, so its okay (or do we not? Our org has the teams plan. Is that enough?)

I havent forgotten this PR. I will review in timely manner.

@oSumAtrIX oSumAtrIX requested review from oSumAtrIX and removed request for oSumAtrIX November 30, 2025 18:19
@Taknok

This comment was marked as resolved.

@coderZalmy

This comment was marked as resolved.

@oSumAtrIX

This comment was marked as resolved.

@coderZalmy
Copy link

Reviewed as in the commits need to be verified?

@oSumAtrIX
Copy link
Member

The changes here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants