Remove aapt1

[iBotPeaches]

The following aapt1 tests fail when forced to aapt2. They need to be investigated if they were testing aapt1 specific behavior or need changes/removal with aapt2.

• brut.androlib.aapt1.BuildAndDecodeTest
• brut.androlib.aapt1.DebugTagRetainedTest > DebugIsTruePriorToBeingFalseTest
• brut.androlib.aapt1.ExternalEntityTest > doctypeTest
• brut.androlib.aapt1.ProviderAttributeTest > isProviderStringReplacementWorking
• brut.androlib.aapt1.SharedLibraryTest > isSharedResourceDecodingAndRebuildingWorking

brut.androlib.aapt1.BuildAndDecodeTest > classMethod FAILED
    brut.androlib.exceptions.AndrolibException at BuildAndDecodeTest.java:52
        Caused by: brut.common.BrutException at BuildAndDecodeTest.java:52

brut.androlib.aapt1.DebugTagRetainedTest > DebugIsTruePriorToBeingFalseTest FAILED
    junit.framework.AssertionFailedError at DebugTagRetainedTest.java:76

brut.androlib.aapt1.ExternalEntityTest > doctypeTest FAILED
    org.junit.ComparisonFailure at ExternalEntityTest.java:62

brut.androlib.aapt1.ProviderAttributeTest > isProviderStringReplacementWorking FAILED
    junit.framework.AssertionFailedError at ProviderAttributeTest.java:72

brut.androlib.aapt1.SharedLibraryTest > isSharedResourceDecodingAndRebuildingWorking FAILED
    brut.androlib.exceptions.AndrolibException at SharedLibraryTest.java:83
        Caused by: brut.common.BrutException at SharedLibraryTest.java:83

[iBotPeaches]

cc @IgorEisberg in case you want to do this.

[IgorEisberg]

cc @IgorEisberg in case you want to do this.

Not sure why GitHub doesn't notify me of mentions, I found this comment by chance just now.
I don't really see the dilemma here, everything under aapt1 was testing behavior that was expected from aapt1 only, those tests are expected to fail under aapt2 because it's a lot stricter. Many of the tests assert things that are forbidden in aapt2.
aapt1 was an easily exploitable mess. The whole aapt1 test folder needs to go.

[IgorEisberg]

By the way, your aapt2 doesn't handle MIUI-specific qualifiers (godzillaui/smallui/mediumui/largeui/hugeui) correctly, e.g. godzillaui gets merged with default resources on compile, then decompiling fails because they conflict. I'd suggest removing anything MIUI-specific from your framework/base if you don't intend on supporting that. I'm using my own aapt2 binary where it works correctly.

[iBotPeaches]

Not sure why GitHub doesn't notify me of mentions, I found this comment by chance just now. I don't really see the dilemma here, everything under aapt1 was testing behavior that was expected from aapt1 only, those tests are expected to fail under aapt2 because it's a lot stricter.

This isn't accurate - I of course didn't know aapt2 would exist so a large chunk of those aapt1 tests are basic Apktool tests that just happen to be using aapt1. So I believe porting all tests, reviewing the failure ones to remove is the correct path so we maintain a higher level of test coverage.

By the way, your aapt2 doesn't handle MIUI-specific qualifiers (godzillaui/smallui/mediumui/largeui/hugeui) correctly, e.g. godzillaui gets merged with default resources on compile, then decompiling fails because they conflict. I'd suggest removing anything MIUI-specific from your framework/base if you don't intend on supporting that.

That is a good call - I don't think I ported any miui changes to aapt2. Looking at my diff of aapt2 changes that confirms it.

I'll take a stab at this after Android 16 is out and I cut a mini release for that support.

[IgorEisberg]

Stumbled upon an issue with an APK used for brut.androlib.aapt1.SharedLibraryTest:
client.apk in brut.apktool/apktool-lib/src/test/resources/aapt1/shared_libraries is being decompiled as a shared library (sharedLibrary: true). aapt1 forgave that, but it can't be rebuilt using aapt2 because shared libraries cannot use package ID 0x7f. I think only library.apk is intended to be a shared library as it's built with package ID 0x02.
Looks like a bug with apktool decoding in the only place that sets 'sharedLibrary: true`:
https://github.com/iBotPeaches/Apktool/blob/master/brut.apktool/apktool-lib/src/main/java/brut/androlib/res/decoder/ARSCDecoder.java#L184

Got the other tests passing, except for the main brut.androlib.aapt1.BuildAndDecodeTest which fully works on testapp used exclusively for aapt1 and has some stuff not allowed in aapt2. testapp for aapt2 is completely different.

[iBotPeaches]

I see. I guess we could grow aapt2 test app with the all the generic testing in aapt1 (which was basic qualifier tests and all the extra - unknown/lib/etc stuff)

[IgorEisberg]

I'm not sure what's going on with ARSCDecoder, even having an empty APK with a basic manifest, including library ID 1 and 2 and having this in strings.xml causes client.apk to be decoded as a shared library.

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <string name="changes">@com.google.android.test.shared_library:string/shared_string</string>
</resources>

[iBotPeaches]

Odd. I thought that code only fired when we had a 0 pkgId, since it was derived at runtime.

[IgorEisberg]

Something mad is going on when a shared library (library.apk) is marked as sharedLibrary: true.
I just decoded libraries from MIUI/HyperOS (framework-res.apk, framework-ext-res.apk, miuisystem.apk) and they are all decoded with sharedLibrary: false, which explains why I never had any build issues.
As for library.apk, it's decoded with sharedLibrary: true and trying to recompile it with aapt2 also gives some mad behavior:

I: Building resources with aapt2...
W: error: can't assign ID 0x02050000 to resource com.google.android.test.shared_library:array/animals because package already has ID 2.
W: error: failed assigning IDs.

The only way to recompile is by removing public.xml or changing to sharedLibrary: false.

[IgorEisberg]

And I just confirmed that sharedLibrary: true being set when decoding client.apk is caused by library.apk having it set to true.
If it's false in library.apk, then it's false when decoding client.apk as well.

[IgorEisberg]

I think we need to re-evaluate what a "shared lib" is (vs. a framework APK) because setting the package ID to the next free ID (0x02) is inappropriate considering that aapt2 does not allow resources with non-zero IDs in a shared lib.
https://android.googlesource.com/platform/frameworks/base/+/master/tools/aapt2/cmd/Link.cpp#2515

As an test, I kept sharedLibrary: true and replaced all ID prefixes in public.xml from 0x02 to 0x00. This way, library.apk built fine. The value for forcedPackageId is meaningless as --package-id is meant for framework APKs, and not passed to aapt2 for shared libraries due to !mApkInfo.sharedLibrary check, so changing that makes no difference.

[iBotPeaches]

I think we need to re-evaluate what a "shared lib" is (vs. a framework APK) because setting the package ID to the next free ID (0x02) is inappropriate considering that aapt2 does not allow resources with non-zero IDs in a shared lib. https://android.googlesource.com/platform/frameworks/base/+/master/tools/aapt2/cmd/Link.cpp#2515

As an test, I kept sharedLibrary: true and replaced all ID prefixes in public.xml from 0x02 to 0x00. This way, library.apk built fine. The value for forcedPackageId is meaningless as --package-id is meant for framework APKs, and not passed to aapt2 for shared libraries due to !mApkInfo.sharedLibrary check, so changing that makes no difference.

Good find. I don't remember randomly picking 0x02. I'll double check aapt1 code for where I learned that. Sounds like the key is to move to 0x00 for shared libs

[IgorEisberg]

I think we need to re-evaluate what a "shared lib" is (vs. a framework APK) because setting the package ID to the next free ID (0x02) is inappropriate considering that aapt2 does not allow resources with non-zero IDs in a shared lib. https://android.googlesource.com/platform/frameworks/base/+/master/tools/aapt2/cmd/Link.cpp#2515
As an test, I kept sharedLibrary: true and replaced all ID prefixes in public.xml from 0x02 to 0x00. This way, library.apk built fine. The value for forcedPackageId is meaningless as --package-id is meant for framework APKs, and not passed to aapt2 for shared libraries due to !mApkInfo.sharedLibrary check, so changing that makes no difference.

Good find. I don't remember randomly picking 0x02. I'll double check aapt1 code for where I learned that. Sounds like the key is to move to 0x00 for shared libs

I think that some hacks used to make aapt1 work seeped into aapt2 support, where such hacks are not gonna fly.
If we manage to get this working right, the sharedLibrary flag becomes obsolete as forcedPackageId: 0 implies a PackageType::kSharedLib already, while only PackageType::kApp is allowed to use a package ID.
https://android.googlesource.com/platform/frameworks/base/+/master/tools/aapt2/cmd/Link.cpp#2525
Basically:

if (mApkInfo.packageInfo.forcedPackageId != null) {
    if (mApkInfo.packageInfo.forcedPackageId.equals("0")) {
        cmd.add("--shared-lib");
    } else if (!mApkInfo.packageInfo.forcedPackageId.equals("1")) {
        cmd.add("--allow-reserved-package-id");
        cmd.add("--package-id");
        cmd.add(mApkInfo.packageInfo.forcedPackageId);
    }
}

[IgorEisberg]

In fact client.apk uses library.apk like it's a framework APK (refers to resources from library.apk by package ID 0x02) so I'm not sure that client.apk was even built properly if it hardcodes the package ID like that if it's supposed to be dynamically assigned.
The only shared library I can find in the wild is android.ext.shared/com.google.android.ext.shared and it has no resources besides app_name (0x00010000). It's only referred to by PackageManagerService but not any app that I can find, so why does Apktool tries to support that and what was the real reference APK that required such support?