Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proguard will remove SwingDispatcherFactory (MainDispatcherFactory) on JVM #4025

Open
mikedawson opened this issue Jan 27, 2024 · 10 comments
Open
Labels
docs KDoc and API reference

Comments

@mikedawson
Copy link

Describe the bug

If you use Proguard on JVM (e.g. as part of a Compose/Desktop application as per compose docs), then the MainDispatcherFactory is removed. When the application runs, it crashes because there is no main dispatcher.

The following Proguard rules are required:

-keep class kotlinx.coroutines.internal.MainDispatcherFactory { *; }
-keep class kotlinx.coroutines.swing.SwingDispatcherFactory { *; }

Proguard on Desktop/JVM doesn't have automatic rule consumption like Android, so documenting the requirement e.g. here https://github.com/Kotlin/kotlinx.coroutines#r8-and-proguard is critical.

Provide a Reproducer

This can be reproduced from this branch of our code:
https://github.com/UstadMobile/UstadMobile/tree/dev-desktop-conveyor

Remove the above lines from app-desktop/compose-desktop.pro , and the crash will be observed.

Alternatively:

  1. Create a new Compose JVM/Desktop app
  2. Enable obfuscation as per compose docs)
  3. Run it.
@mikedawson mikedawson added the bug label Jan 27, 2024
@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Jan 31, 2024

I'm not sure that's our problem. The classes Proguard removes are explicitly listed as services: https://github.com/Kotlin/kotlinx.coroutines/blob/master/ui/kotlinx-coroutines-swing/resources/META-INF/services/kotlinx.coroutines.internal.MainDispatcherFactory We don't do anything funny with our dispatcher factories, it's the standard Java mechanism: https://docs.oracle.com/javase/tutorial/sound/SPI-intro.html

If we decide that it's a bug that kotlinx-coroutines-swing doesn't provide Proguard rules, then every library out there that provides any services at all must also include the corresponding keep rule for Proguard, or it's buggy, which I think is absurd. I'd recommend raising this problem to Proguard. It's certainly not impossible for them to fix this. For example, R8 recognizes service files and doesn't require such keep rules: https://github.com/Kotlin/kotlinx.coroutines/blob/master/ui/kotlinx-coroutines-android/resources/META-INF/com.android.tools/r8-upto-3.0.0/coroutines.pro

@mikedawson
Copy link
Author

I agree that the the proguard situation where rules are handled manually isn't great, but that's how it is. Compose/Desktop multiplatform uses Proguard, not R8 . So either the rules for using any given project need documented, or the developer making a Compose/JVM has to go through all jar dependencies looking for services.

KCEF is documenting this: https://github.com/DatL4g/KCEF/?tab=readme-ov-file#proguard

If someone is using library A via library B, question is who should document this? I think the authors of Library A would need to document their library's proguard requirements, and the authors of library B include the requirements of Library A. Yes it could and should be automated, but the current production toolchain does not have that automation.

@dkhalanskyjb
Copy link
Collaborator

What I think is a problem on our side is the error message:

        "Module with the Main dispatcher is missing. " +                        
            "Add dependency providing the Main dispatcher, e.g. 'kotlinx-coroutines-android' " + 
            "and ensure it has the same version as 'kotlinx-coroutines-core'"

When Proguard breaks the correctness of the program, this error message is not helpful. Maybe we could expand the error message to something like

        "Module with the Main dispatcher is missing. " +                        
            "Add dependency providing the Main dispatcher, e.g. 'kotlinx-coroutines-android' " + 
            "of the same version as 'kotlinx-coroutines-core' and ensure that modification doesn't remove " +
            "implementations of MainDispatcherFactory"

Or maybe that's too lengthy, so we could add a Main dispatcher troubleshooting section to README.md and do something like

"Module with the Main dispatcher is missing. See https://github.com/Kotlin/kotlinx.coroutines/README.md#Missing_Main_dispatcher

It would be nice if we could recommend a universal Proguard rule like "keep all classes implementing MainDispatcherFactory," and it looks like we can do that; I'd still need to check this.

@mikedawson, I'd like to use your reproducer. What Gradle command should I run? When I remove the -keep rules from app-desktop/compose-desktop.pro and run ./gradlew proguardReleaseJars, the build succeeds.

@mikedawson
Copy link
Author

The build will succeed - but if you remove the keep rule and run it (e.g. ./gradlew app-desktop:runReleaseDistributable it will crash)

@mikedawson
Copy link
Author

That said - @dkhalanskyjb I would disagree with just changing the error message. Yes, I think until everyone starts reading documentation, changing the error message is definitely a good idea.

However, the proguard task can take some time. The release compilation can take a while (on my project takes about 4 mins on a machine with 64GB of RAM). If everyone has to build/run/repeat to discover this with a 5-10 libraries, that is a considerable amount of lost time.

Using Proguard is a supported procedure with stable / release versions of Compose/Desktop, so if I follow the documentation for a supported use case, I shouldn't be getting errors with a core use case (eg not an edge case scenario).

@dkhalanskyjb
Copy link
Collaborator

./gradlew app-desktop:runReleaseDistributable

I get

Exception in thread "main" java.lang.IllegalStateException: No MediaInfo found

Is this expected?

@mikedawson
Copy link
Author

@dkhalanskyjb - yes, our desktop app requires mediainfo. If running ubuntu, you can do apt-get install mediainfo . On Windows you can put it app-resources/mediainfo (our installer build does this itself). If the mediainfo is anywhere on your path command, it will find it..

dkhalanskyjb added a commit that referenced this issue Feb 6, 2024
With this change, `kotlinx-coroutines-android` no longer needs to
specify for Proguard and the older R8 versions that the Main
dispatcher and the uncaught exception handler must be kept;
instead, the rules shipped with the coroutines library do that for
every implementation now.

Fixes #4025
@dkhalanskyjb
Copy link
Collaborator

Managed to pinpoint the problem, thanks.

Ok, so do I get it correctly that you don't want us (the kotlinx-coroutines library) to document the Proguard rules, as the users would still need to go and find that documentation, but instead your problem is with Compose? Do you think it's enough for Compose team to update https://github.com/JetBrains/compose-multiplatform/tree/master/tutorials/Native_distributions_and_local_execution#minification--obfuscation ?

@dkhalanskyjb dkhalanskyjb added docs KDoc and API reference and removed bug labels Feb 9, 2024
@amal
Copy link

amal commented Feb 9, 2024

FYI, ProGuard can add support for JVM in Guardsquare/proguard#337
Also, it can just be reused manually from the Android task. So I'm sure it's handy to package ProGuard rules in jars.
It's a better future-proof solution than manual configuration by the end user.

@dkhalanskyjb
Copy link
Collaborator

Filed an issue for the Compose documentation: JetBrains/compose-multiplatform#4288

@amal, I saw the issue, but the maintainers didn't even respond to it yet. In any case, the scheme for supplying rules on Desktop, even if they do support that, may end up different from what they use for Android, so it's meaningless to adapt our code right now to hypothetical future changes that we don't know anything about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs KDoc and API reference
Projects
None yet
Development

No branches or pull requests

3 participants