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

Fix proguard/r8 rules for Android #300

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Conversation

ahmedre
Copy link
Contributor

@ahmedre ahmedre commented Oct 17, 2024

Libraries that require proguard rules should publish them automatically to r8 by using consumerProguardFiles. Currently, Ferrostar declared this, but left the corresponding rules files empty. This patch moves the rules from proguard-rules.pro so that R8 automatically picks them up without apps using Ferrostar needing to do anything.

Moreover, it removes proguard-rules.pro and the corresponding release block from Android libraries, since whether the build type is release or debug (and whether or not r8 is enabled) is dependant on the app module.

@ahmedre
Copy link
Contributor Author

ahmedre commented Oct 17, 2024

How I tested -

  • built our app as release - ran it and it crashed.
  • applied these changes locally, bumped the version, and did a ./gradlew publishToMavenLocal.
  • updated the app to use the updated snapshot version.
  • built the app as release - ran it and it didn't crash.

Libraries that require proguard rules should publish them automatically
to r8 by using consumerProguardFiles. Currently, Ferrostar declared
this, but left the corresponding rules files empty. This patch moves the
rules from proguard-rules.pro so that R8 automatically picks them up
without apps using Ferrostar needing to do anything.

Moreover, it removes proguard-rules.pro and the corresponding release
block from Android libraries, since whether the build type is release or
debug (and whether or not r8 is enabled) is dependant on the app module.
Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I just did some research on this and I think you're correct. Will merge after CI checks pass.

@ianthetechie ianthetechie merged commit 02657e3 into stadiamaps:main Oct 17, 2024
14 checks passed
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.

3 participants