Every second build fails due to compiler not finding things changed with DexReplace

Lanchon commented 4 years ago

originally filed by: @andrewleech moved to new issue by Lanchon

often every second build fails due to the compiler not finding a bunch of things I've changed with DexReplace. I hit build again and it always works though. Have you seen this? Thinking about it now, it might be from cases

the build issue I get, it's reliable for me with this project, this commit: https://gitlab.com/alelec/fossil_smartwatches_alelec_android/tree/9c8fecfea815dbf33c68e13b5c78498bce3773b8 It should just checkout and build, I'm currently on Android Studio 3.5.1 (though I've had this issue for a long time now) Basically build, it works. build again, it's still fine (it doesn't re-compile anything). If I make one change (even just change formatting on a file, no functional change) the build fails, with the first failure being

> Task :compileDebugJavaWithJavac
C:\Users\corona\src\fossil_app\src\main\java\com\portfolio\platform\MigrationManager.java:761: error: constructor Device in class Device cannot be applied to given types;
                Device device = new Device(
                                ^
  required: String,String,String,String,int,Integer,boolean
  found: String,String,String,String,int,int,boolean,int,<null>

In this case, jadx decompiles a number of functions like this with a couple more args than the builtin decompilers comes up with. So I've got the jadx copy sitting there completely DexIgnored and that satisfies the compiler (every second time) and function calls I make to the long version seem to work fine in practice. But every other time, the compiler only sees the jar version and complains the signature's wrong. Not the end of the world, I just compile again, but a bit odd nonetheless.

andrewleech commented 4 years ago

Cheers, yeah I probably should have opened a new issue from the start!

Lanchon commented 4 years ago

one workaround that might help you for now is using a 'clean assembleDebug' or similar as a build target, as shown here:

andrewleech commented 4 years ago

Ah yeah, that should work reliably - though for a larger app it's probably also slower than hitting compile twice!

Lanchon commented 4 years ago

clean building is pretty fast if you enable gradle caching. MAKE SURE YOU DO!!! i don't think that's in the docs... it should! the v2 gradle plugins fully take advantage of caching.

Lanchon commented 4 years ago

hey! i'm missing the orig/*apk. lanchon at google's email.

Lanchon commented 4 years ago

and an unrelated comment.

you are using:

Android Gradle plugin 3.4.2... OK
build tools 28... OK
but platform 29... hmmm

platform 29 might require newer versions of these tools. consider downgrading to 28 if your app permits.

andrewleech commented 4 years ago

The apk should be there, but it is checked in with LFS https://gitlab.com/alelec/fossil_smartwatches_alelec_android/blob/9c8fecfea815dbf33c68e13b5c78498bce3773b8/orig/Fossil_Smartwatches_v4.1.3.apk

If you install git lfs it should work, or you could download it directly from that link.

The original app appears to be compiled with build tools 29 so I assumed I needed to add well, haven't tried going back to 28.

Lanchon commented 4 years ago

The apk should be there, but it is checked in with LFS

but i need the apk you feed dxp-gradle. is this the file that you already put though apktool and your class renaming script?

andrewleech commented 4 years ago

Ah, yes it is the repacked/renamed one, this is what I feed into dxp-gradle. I preprocess it (with apktool and smali_patch.py script) then point gradle to it, I never needs to know about the renaming. The apk is still valid and runs fine after such renaming.

When I try your new auto-renaming I'll go back to the original unmodified apk, I can update this ticket when I have it up of you'd prefer.

Lanchon commented 4 years ago

no, it's ok for investigating this issue.

meanwhile, your use of rootProject.files('orig/dex-tools-2.1-SNAPSHOT') instead of a zip file generates this error for me:

> Task :dedexAppClasses FAILED
Error: Could not find or load main class com.googlecode.dex2jar.tools.Dex2jarCmd

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':dedexAppClasses'.
> Process 'command '/home/rod/android/android-studio/android-studio/jre/bin/java'' finished with non-zero exit value 1

which is not surprising, as this dependency is processed by a zipTree(...) which shouldn't work on a straight directory. (or maybe i coded it to autodetect?)

im suprised this works for you. what platform are you working on?

Lanchon commented 4 years ago

FYI, im building using 'dexpatcher-repo.dexpatcher.dex2jar:dex-tools:2.1-20171001-lanchon@zip'. any issues with that?

i still can't build. now i'm getting lots of these:

> Task :compileDebugJavaWithJavac FAILED
error: package com.misfit.frameworks... does not exist
error: package com.portfolio... does not exist
error: cannot find symbol

what's wrong?

andrewleech commented 4 years ago

Yep that error is from the older dex-tools, I just switched back to your copy and sync'd it, the first error is

> Task :dedexAppClasses
dex2jar C:\Users\corona\src\fossil_app\orig\Fossil_Smartwatches_v4.1.3.apk -> C:\Users\corona\src\fossil_app\build\intermediates\dexpatcher\dedexed-classes\app-classes.jar
com.googlecode.d2j.DexException: fail convert code for Lcom/google/common/reflect/TypeToken$f;.a(Ljava/lang/Object;Ljava/util/Map;)I
    at com.googlecode.d2j.dex.ExDex2Asm.convertCode(ExDex2Asm.java:44)
    at com.googlecode.d2j.dex.Dex2jar$2.convertCode(Dex2jar.java:132)
    at com.googlecode.d2j.dex.Dex2Asm.convertMethod(Dex2Asm.java:582)
    at com.googlecode.d2j.dex.Dex2Asm.convertClass(Dex2Asm.java:441)
    at com.googlecode.d2j.dex.Dex2Asm.convertDex(Dex2Asm.java:457)
    at com.googlecode.d2j.dex.Dex2jar.doTranslate(Dex2jar.java:126)
    at com.googlecode.d2j.dex.Dex2jar.to(Dex2jar.java:275)
    at com.googlecode.dex2jar.tools.Dex2jarCmd.doCommandLine(Dex2jarCmd.java:107)
    at com.googlecode.dex2jar.tools.BaseCmd.doMain(BaseCmd.java:290)
    at com.googlecode.dex2jar.tools.Dex2jarCmd.main(Dex2jarCmd.java:33)
Caused by: java.lang.RuntimeException
    at com.googlecode.dex2jar.ir.ts.TypeTransformer$TypeAnalyze.mergeProviderType(TypeTransformer.java:613)
    at com.googlecode.dex2jar.ir.ts.TypeTransformer$TypeAnalyze.mergeTypeToSubRef(TypeTransformer.java:457)
    at com.googlecode.dex2jar.ir.ts.TypeTransformer$TypeAnalyze.copyTypes(TypeTransformer.java:560)
    at com.googlecode.dex2jar.ir.ts.TypeTransformer$TypeAnalyze.fixTypes(TypeTransformer.java:392)
    at com.googlecode.dex2jar.ir.ts.TypeTransformer$TypeAnalyze.analyze(TypeTransformer.java:369)
    at com.googlecode.dex2jar.ir.ts.TypeTransformer.transform(TypeTransformer.java:45)
    at com.googlecode.d2j.dex.Dex2jar$2.optimize(Dex2jar.java:165)
    at com.googlecode.d2j.dex.Dex2Asm.convertCode(Dex2Asm.java:449)
    at com.googlecode.d2j.dex.ExDex2Asm.convertCode(ExDex2Asm.java:41)
    ... 9 more

IIRC there's a newer android feature that trips it up, but works with the newer snapshot version included in the repo.

I remember it taking a bit of trial and error (late at night) to get the gradle reference to the newer dex-tools working, but what's there builds fine in a clean checkout in CI so seems to work fine for me. Again, all the dex-tools files are checked in with LFS so may not have cloned correctly? https://gitlab.com/alelec/fossil_smartwatches_alelec_android/-/jobs/367504097

Lanchon commented 4 years ago

Again, all the dex-tools files are checked in with LFS so may not have cloned correctly?

ok so that's the issue.

Lanchon commented 4 years ago

i was able to repro.

i have a strong suspicion that this is caused by gradle's java compile avoidance:

importSymbols=true causes all types such as AppType to be added to javac classpath.
AppType is also defined in your java sources.
when javac is invoked with both a java source and a classpath of the same class, javac ignores the binary and compiles the source, and uses the source to compile all other sources at the same time.
when you change AppType, somehow gradle is not feeding your source to the compilation of other client classes of AppType, possibly because it finds the binary via importSymbols.
the client classes can't compile because they depend on the AppType changes.
but AppType itself compiled, so its compiled class file is used for the next compilation. it masks the binary in importSymbols, and now the client classes compile.

i might be able to build a mockup of this without the dxp plugin and verify. and report it to gradle.

Lanchon commented 4 years ago

observation: you should avoid as much as possible copying code from the source app.

1) because it is copyrighted 2) because it can change under your feet, requiring you to update the patch.

for instance, look at AppType:

@DexReplace
public class AppType extends DynaEnum {
    public static AppType BADOO = new AppType(-1, "com.badoo.mobile");
    public static AppType CLOCK = new AppType(R.string.app_clock, "");
    public static AppType ESPN = new AppType(-1, "com.espn.score_center");
    public static AppType FACEBOOK = new AppType(-1, "com.facebook.katana");
    public static AppType FACEBOOK_MESSENGER = new AppType(-1, "com.facebook.orca");

why not this instead?

@DexEdit
public class AppType extends DynaEnum {
    @DexIgnore public static AppType USED_VALUE_1;  // only bring here the ones YOUR code depends on
    @DexIgnore public static AppType USED_VALUE_2;
    @DexAdd public static AppType MY_NEW_VALUE = new AppType(...);

    @DexAdd
    public static AppType get(String appName) {
        return AppType.get(AppType.class, appName);
    }

XXX.class can always give you trouble (compiler synthesizes fields), so you can do something like this for that:

@DexEdit
public class AppType extends DynaEnum {
    static class CLASS {
        static final Class AppType = AppType.class;
    }

    @DexAdd
    public static AppType get(String appName) {
        return AppType.get(CLASS.AppType, appName);
    }

andrewleech commented 4 years ago

Interesting... and thanks for the pointers.

In this case, with AppType, the original in the jar was a regular java enum. I brought in DynaEnum from a blog online and converted AppType to entend from it instead of enum as I wanted to make it runtime editable. I didn't think such a conversion would be possible as just a DexEdit, or more importantly the existing fields were enum entries, not static AppType fields.

But yeah I do tend to bring in more and more, even when I don't continue to use it (usually after a bit of debug tracing). Habits from my navdy project where I know the source apk's are never going to change again since the company closed.

Lanchon commented 4 years ago

the original in the jar was a regular java enum

lol of course i didn't notice this :)) sorry

I didn't think such a conversion would be possible as just a DexEdit, or more importantly the existing fields were enum entries, not static AppType fields.

interesting... it probably is possible. an enum is just syntactic sugar. the values indeed are static final XXX = constructor(of, args);. as long as you dexedit to a regular class and provided the needed constructor, the original <clinit> should initialize the original values (static fields). you just need to dexreplace and dexremove the Enum methods you don't want. just extend DynaEnum and dexremove the methods defined in the original enum and DynaEnum. dexremove will remove them from the class, so the class will inherit them from DynaEnum.

andrewleech commented 4 years ago

one workaround that might help you for now is using a 'clean assembleDebug' or similar as a build target, as shown here:

Adding a gradle [clean assembleDebug] target is definitely a step in the right direction. Do you know whether there's a way to just clean the compiler artifacts, leaving the decodeApk and dedex outputs there? Runing [clean assembleDebug] does resolve the issue, but needing re-run the dxp apk extraction each time take build time from a few seconds up to a minute and a half.

update: scratch that, I found that by deleting build/intermediates/javac I get the same result, ie no more build failures. It also only adds a few seconds to the build time. So in build.gradle adding

tasks.withType(JavaCompile) {
    options.compilerArgs << '-proc:none'
    project.delete(files("${buildDir}/intermediates/javac"))
}

Permanenetly fixes my issue! (I already had this task with the compilerArgs from a previous issue)

Lanchon commented 4 years ago

lol, i told you! enable the gradle build cache. clean builds will be almost instantaneous

andrewleech commented 4 years ago

Oh I did look into that, everything I could find said the gradle cache is automatically enabled by recent versions of Android studio. Is there something different you do?

Lanchon commented 4 years ago

i dont remember, let me take a look. what i can tell you is, the cache works for stuff like apktool decode and dex2jar. if clean build causes those tasks to run, then it is not enabled in your setup.

Lanchon commented 4 years ago

Run with --build-cache on the command-line
- in your case: ./gradlew clean assembleDebug --build-cache
- Gradle will use the build cache for this build only.
Put org.gradle.caching=true in your gradle.properties
- Gradle will try to reuse outputs from previous builds for all builds, unless explicitly disabled with --no-build-cache.

Lanchon commented 4 years ago

everything I could find said the gradle cache is automatically enabled by recent versions of Android studio

i think that was the Android Gradle plugin build cache. this was a "proprietary" solution for android. later, Gradle develop their own and better Gradle-wide cache, and then Google removed the ad-hoc old build cache from the Android plugin.

andrewleech commented 4 years ago

Ah, I see. I guess this is what you mean, this makes more sense: https://docs.gradle.org/current/userguide/build_cache.html

yep, [clean debug]:

BUILD SUCCESSFUL in 5s

thank you, thank you very much

DexPatcher / dexpatcher-gradle

Every second build fails due to compiler not finding things changed with DexReplace #28