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

Change ProGuard rules to suppress the library causing the problem #394

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TWiStErRob
Copy link

@TWiStErRob TWiStErRob commented May 15, 2024

This re-fixes #297. cc @dkhalanskyjb @MarcelReiter @LouisCAD

Why?

The current solution on master is not broken, but it references the wrong library. Why? There are other libraries out there, in similar shoes to datetime, their problems will also be suppressed, by this library. This library is by no means authoritative to make a decision how other libraries should use kotlinx.serialization. Therefore it should be configuring ProGuard/R8 for itself. The other libraries need to be given a chance to detect and fix their own problems.

Background

In general, if a user decides to suppress kotlinx.serialization with -dontwarns in their project, that's a decision, but that's an end user decision, not a library one, it shouldn't apply to all users in the world.

How?

The warning mentions that Instant is using Serializable, in this case Instant is doing the "wrong" thing, and therefore it shouldn't be Serializable that's being suppressed. Ideally -dontwarn would be powerful enough to combine the suppression of datetime + serialization, but until then we should be more specific about the suppression. The specificity in this case is datetime, because serialization is a more generic library.

Testing

I used the https://github.com/MarcelReiter/dateTimeTest to confirm the fix. It only uses Instant, but each of the datetime primitives use @Serializable so the warning will apply to all, same with all kotlinx.datetime.serializers classes.

@dkhalanskyjb
Copy link
Contributor

There are other libraries out there, in similar shoes to datetime, their problems will also be suppressed, by this library.

If these other libraries are using the exact same pattern as kotlinx-datetime, their problems will automatically be fixed. If they have entirely other problems that are accidentally suppressed by the new proguard rules, please provide an example. We tried to find one, but couldn't.

we should be more specific about the suppression. The specificity in this case is datetime, because serialization is a more generic library.

The argument also goes in another direction: the current suppression is more specific, since it mentions exactly the minimum number of classes. What you're proposing is a wildcard suppression, which covers an arbitrary number of entities. If the user accidentally forces the wrong datetime library version in their config, the suppression suggested by you can hide the problem.

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.

Missing classes detected while running R8 / Proguard
2 participants