libgdx / gdx-liftoff

A modern setup tool for libGDX Gradle projects
Apache License 2.0
516 stars 46 forks source link

minifyEnabled true can create problems #133

Closed JojoIce closed 10 months ago

JojoIce commented 10 months ago

The minifyEnabled has been changed to be true from liftoff.

This can create some difficult to trace problems. I for example use play-services-ads, and if fails with some class not found error, probably from some introspection with classnames or similar. Fortunately, my own code failed in the same matter and that was easier to find and debug and therefore I could understand where the problem came from. I also tried to preserve the play-services-ads from minifying, but did not succeed. Maybe a note in the Troubleshooting page regarding this can help out a lot.

tommyettinger commented 10 months ago

From eerie shadows black as night, Eyes shine red with murd'rous sight, Giant strength from frozen might, @Frosty-J ! I summon thee to make this right!

(Frosty-J was adamant that Liftoff change minifyEnabled to true for releases.)

The minifyEnabled setting is only on for releases using Android by default; debug builds have it off. It really should be on for releases, since it can help with file size and many users don't want to release un-obfuscated code straight to the web.

I'll close this once Mr. J explains why he was in favor of enabling this by default.

Frosty-J commented 10 months ago

I'll close this once Mr. J explains why he was in favor of enabling this by default.

I was in favour for the reasons you stated. It may also help app speed a little, but this was more relevant in Android's early days. It is the norm in the Android world for it to be on for releases, and no-one likes to be surprised to learn their app is almost open source.

minifyEnabled true can create problems

True, this should be noted, especially with a change in default. Your builds should produce a mapping file you can use with ReTrace or Google Play to view the stack trace in unobfuscated form. By the sounds of it, you probably forgot to exclude something in the rules that depends on reflection. Pretty sure the Play Services stuff is meant to automatically have the right exclusions applied by AGP, but gosh darn heck is something probably broken as per usual! Further reading is available at https://developer.android.com/build/shrink-code#troubleshoot

tommyettinger commented 10 months ago

I just added two lines to the default Android ProGuard file to enable generating a mapping on every build:

-keepattributes LineNumberTable,SourceFile
-renamesourcefileattribute SourceFile

There's also a link in a comment to how to use the mapping.txt file that generates.

JojoIce commented 10 months ago

Yes, I understand the reasons for wanting minfication and mostly agree with them even though size and performance seem questionable today.

By the sounds of it, you probably forgot to exclude something in the rules that depends on reflection.

Well, that is a bit my point. See it like this. Bobby was sitting developing his shiny new game and getting close to release. Thinking it is best to have all the latest and greatest, and the only way to do that seems to be to create a new project with liftoff and try to copy all the needed things from the old proj to the new. Testing the new project will after some pain finally look nice, and the game is released, only for Bobby to realize that once released it will do nothing but crash inexplicably. Especially since previously releasing to internal testing just worked fine. How could he forget something that he never knew anything about?

Again: Maybe a note in the Troubleshooting page regarding this can help out a lot.

And if even google can't get it right with the ads service, how can then all the Bobbys? I think however that the ads service already is obfuscated, so it might be that it does some introspection from its code into mine or Bobbys code maybe???

Either way, I am quite sure that this will cause a lot of headaches for a lot of people given its rather inexplicable nature and relative difficulty of debugging.

If you have any idea of how to fix the problem with google ads then I am all ears, since I don't even know where to begin. And then how many more problems will there be once that is fixed?

By the way, high up on my wish list would be to have:

https://github.com/MrStahlfelge/gdx-gamesvcs https://developers.google.com/android/guides/setup (Google Play services) https://github.com/dkimitsa/robovm-robopods/tree/alt/firebase

In the Third-party list (with the correct minification voodoo done for them). How many 3rd party libs survive minification?

So I am also for minification but it would be nice (needed) for users to get help with alleviating the pains that come with it. For me currently, the only solution I see is however quite simple once I know the solution, and that is to switch minifyEnabled to false, since that is the only way I know how to get it running sadly. Debugging googles already minified code is well above my skill level.

Frosty-J commented 10 months ago

Bobby should test a release build of his game after making such a large change as regenerating the project with new versions of everything. Release builds on Android aren't guaranteed to behave the same as debug builds - the same applies in other fields of programming, too.

Out of the libraries you've listed:

You may find these ProGuard rules useful as an example. They are from a project that uses gdx-gamesvcs, gdx-controllers, gdx-gameanalytics, gdx-pay, gdx-pushmessages, gdx-websockets, and kryonet. The Box2D line can be ignored, as that project doesn't use it. You can see the bulk of it is just for his own purposes, but there is the odd line that could be relevant such as -keep class com.android.vending.billing.** and -keep class com.google.android.gms.games.multiplayer.InvitationEntity.

How many 3rd party libs survive minification?

Mostly they're okay. Either they work without further steps or they document what to do - example. Unpopular libraries that the author has only tested on desktop are the most likely to have been overlooked in respect to ProGuard.

JojoIce commented 10 months ago

So I have been digging a bit further...

The thing that I really like about liftoff is that numbskulls like Bobby and I with relative ease can get to the point of doing what we really like, which is coding games. I have done the best I can with my limited knowledge to try to help a bit making it easier, especially with iOS things.

Release builds on Android aren't guaranteed to behave the same as debug builds - the same applies in other fields of programming, too.

Sure, just saying that for me it probably would have saved me (and maybe many all the Bobbys out there) a few hours if some info from people who know what they are doing had been in the Troubleshooting page or similar, but I think we agree there. Also in most fields of programming, it is desired to have the dev/staging as similar as possible to prod. If my daytime environment would have as big differences between dev and prod we'd have big crisis meetings.

At least after some time, I realized it was easy to make debug crash in the same way as the release, so it was easer and quicker to debug.

Sorry the 3 3rd party lib, was just me injecting a wish to have ads, leaderboard and stats easily added via Liftoff, since I would imagine many would want that. My mind just wandered a tiny bit when thinking about the implications the minify true can have on all those 3rd party libs, but maybe they are already minify safe.

Yes, you are right, Google ads did not suffer from the minify, it was throwing the same nasty Exception minify or not and it still worked, but it shows how difficult it is to debug this stuff (for Bobby and me)... The problem was really a much less scary-looking message in the abyss of logcat.

On the other hand, create the classic libgdx project using liftoff. I tried to do it with gdx-setup as well, but rage-quit after 45 min of stress. Eventually got stuck at some mixed zip libs... Anyway, add the following in public void create() of your game object:

new com.badlogic.gdx.graphics.g2d.GlyphLayout();

Yes took me another few hours to condense the problem down to this...

At least for me, it resulted in:

FATAL EXCEPTION: GLThread 16596
    Process: com.trilligames.testmini, PID: 11004
         java.lang.ExceptionInInitializerError
        at com.trilligames.testmini.testmini.create(testmini.java:19)
            at com.badlogic.gdx.backends.android.AndroidGraphics.onSurfaceChanged(AndroidGraphics.java:312)
            at android.opengl.GLSurfaceView$GLThread.guardedRun(GLSurfaceView.java:1557)
            at android.opengl.GLSurfaceView$GLThread.run(GLSurfaceView.java:1272)
         Caused by: java.lang.RuntimeException: Class cannot be created (missing no-arg constructor): com.badlogic.gdx.graphics.g2d.GlyphLayout$GlyphRun
            at com.badlogic.gdx.utils.ReflectionPool.<init>(ReflectionPool.java:41)
            at com.badlogic.gdx.utils.Pools.get(Pools.java:29)
            at com.badlogic.gdx.utils.Pools.get(Pools.java:38)
            at com.badlogic.gdx.graphics.g2d.GlyphLayout.<clinit>(GlyphLayout.java:48)
            at com.trilligames.testmini.testmini.create(testmini.java:19) 
            at com.badlogic.gdx.backends.android.AndroidGraphics.onSurfaceChanged(AndroidGraphics.java:312) 
            at android.opengl.GLSurfaceView$GLThread.guardedRun(GLSurfaceView.java:1557) 
            at android.opengl.GLSurfaceView$GLThread.run(GLSurfaceView.java:1272) 

Without minify it is smooth sailing. That put fear in me that it is not only a few unpopular desktop libs overlooking the trivialities of minifying/ProGuard... Not sure what I did wrong there, but scares me as well. I copied the proguard-rule.pro from the gdx-setup project and same thing. It seems like the rabbit hole goes really deep.

Thanks for the link to the link to the git repo. Unfortunately, I still could not figure out how to bring my code back from obfuscation.

I could not for the life of me figure out how to wrestle proguard-rules into not obfuscating away my constructor if I only created an object using reflection. Everything is fine if I first create an object of the same class at the start, and then use reflection to get it going. I guess I will have to keep digging. It is scary and a mystery to Bobby and me...

tommyettinger commented 10 months ago

ProGuard is annoyingly sparsely documented. What's worse is that I just spent quite a while trying to see if a feature would actually work with desktop ProGuard... but it only works with R8, on Android. It would be really nice to have here. R8 allows libraries to store some ProGuard options in their META-INF/proguard folder, and have those apply to the resulting assembled ProGuarrd configuration. Despite the name, this feature does not appear to exist in ProGuard! If it did, some of this could be much easier. With desktop ProGuard unable to get some of its config from libraries, it's essentially impossible to automatically handle ProGuard from a tool like Liftoff -- the best we could do is reimplement R8's merging feature for other platforms, and hope it all works. If libraries need specific ProGuard config, in theory they should document it, but in practice many libraries aren't tested on Android.

As for GlyphLayout, I seem to remember scene2d.ui, Font, and a few other classes (like Color) needed ProGuard to be told to keep them. GlyphLayout is closely linked to Font, so it makes sense too. Recent versions of R8 default to doing some extra optimization that removes default constructors; I'm not really sure who thought this was a good idea, but this "full" mode of R8 can be disabled. In gradle.properties, setting android.enableR8.fullMode=false should help some with the case you listed, but I haven't tried yet, and it only applies to Android because only R8 does that constructor purge.

JojoIce commented 10 months ago

You, sir, are a genius! That solved all the problems, including the class that I knew I needed to add pro-guard rules but didn't even get to since I got stuck at other problems. That also made the "sloppy" gdx code/config work as well. Not sure how much it actually obfuscates and how much code it removes with this flag, and don't have time to investigate right now, but this puts the obfuscation in a whole new light. So minifyEnabled=true together with android.enableR8.fullMode=false could probably be a good solution as defaults for liftoff.

What I quickly read here: https://developer.android.com/build/shrink-code#full-mode makes me think that fullMode=true it will recognize that a class is introspected but assume that you are not planning to do anything with it so it obfuscates the crap out of it and with fullMode off it assumes that you actually "introspect it for a reason" and will leave it be. Need more time to understand, but that makes me believe that fullMode=false is perfectly fine.

tommyettinger commented 10 months ago

Fantastic; this should be a good solution as a default, then. This makes the ProGuard rules match those used with AGP 7.x, as gdx-setup needs. Users can change it to true if they're aware of it and want whatever tiny optimization it ekes out, but then at least they know they're applying it. I'll add android.enableR8.fullMode=false to the default Android ProGuard rules. I'm definitely only going to enable minifying for releases, since it takes a while, and isn't generally needed or wanted during the development/debugging phase.