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] Android minify builds #644

Merged
merged 21 commits into from
Oct 4, 2023
Merged

Conversation

shepherd-l
Copy link
Contributor

@shepherd-l shepherd-l commented Oct 3, 2023

Description

One Line Summary

Adds proguard rules to fix Android builds with minify enabled

Details

Motivation

Fix runtime error, AndroidJavaException: java.lang.ClassNotFoundException: com.onesignal.OneSignal for Android builds with Minify enabled.

Scope

OneSignalConfig.plugin has changed to OneSignalConfig.androidlib.

Custom notification icons are now located in Assets/Plugins/Android/OneSignalConfig.androidlib/src/main/res

Users must run the "Copy Android plugin to Assets" step in Window > OneSignal SDK Setup to migrate.

Testing

Manual testing

Tested OneSignal initialization and custom notification icons, app build with Unity 2022.3.10f1 with Minify enabled for release and development builds of the OneSignal example app on a emulated Pixel 4 with Android 12.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@shepherd-l shepherd-l force-pushed the fix/proguardAndroidlib branch 2 times, most recently from f355f6b to 9c41d79 Compare October 3, 2023 04:41
Match compileSdkVersion of Android SDK and remove build tools version
@@ -1,6 +1,5 @@
fileFormatVersion: 2
guid: 006a8c36b25a144879a84c0bf1ce24a7
folderAsset: yes
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd this folderAsset: yes went away, it is still a folder. Did Unity change something where they no longer flag folders in their .meta files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's mentioned here:
https://forum.unity.com/threads/loadable-plugin-directory-import-behaviour-change-androidlib-bundle-framework-and-plugin.1381113/

I think they applied this to Unity 2022

When using Unity 2022.3.10, .meta files where not generated in com.onesignal.unity.android/Editor/OneSignalConfig.androidlib/consumer-proguard.pro
but did so in Unity 2021.3.30

I think we should upgrade the example app Unity Editor version to 2022 and also regenerate the Custom template files - maybe in a PR after the release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I loaded the example project in 2022.3.10 - OneSignalConfig.plugin.meta also has this change

@@ -0,0 +1,3 @@
-keep class com.onesignal.** { *; }

-keep class kotlinx.coroutines.android.AndroidDispatcherFactory {*;}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment to this line that indicate we are working around an issue with this library?

}

// Rename OneSignalConfig.plugin to OneSignalConfig.androidlib
AssetDatabase.MoveAsset(_pluginV3ExportPath, _pluginExportPath);
Copy link
Member

Choose a reason for hiding this comment

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

We also need to account for moving the icons and .wav file to /src/main.

@@ -67,10 +66,23 @@ public override bool IsRequired
}

protected override void _runStep() {
var files = Directory.GetFiles(_pluginPackagePath, "*", SearchOption.AllDirectories);
var filteredFiles = files.Where(file => !file.EndsWith(".meta"));
if (Directory.Exists(_pluginV3ExportPath)) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be cleaner to move this migration logic into its own method.

@@ -102,5 +114,10 @@ public override bool IsRequired

private static readonly string _manifestPackagePath = Path.Combine(_pluginPackagePath, "AndroidManifest.xml");
private static readonly string _manifestExportPath = Path.Combine(_pluginExportPath, "AndroidManifest.xml");

// OneSignalConfig name used in the 3.x version of the SDK
Copy link
Member

Choose a reason for hiding this comment

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

While the name was primarily used in 3.x.x I think it is important to include this folder name was used in v5 as well. So maybe say old name used from 3.x.x to 5.0.2 in a comment.

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
- Sending VSAttribution data from the editor
- iOS notifications clicked event firing if the app was cold started from clicking a notification
- ClassNotFoundException: com.onesignal.OneSignal for Android builds with minify enabled
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a sub note to this that you must run the export Android step again?, as I see ExportAndroidResourcesStep has changes.

  • I see we have in our 3.x.x migration guide to run this, but this is something customers on 5.0.0 to 5.0.2 will need to be aware of they need to re-run.

Ran setup step again to apply androidlib nit fixes
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 67 files at r1, 1 of 2 files at r2, 3 of 5 files at r3, all commit messages.
Reviewable status: 25 of 67 files reviewed, 6 unresolved discussions (waiting on @jennantilla, @jkasten2, @nan-li, and @shepherd-l)

@shepherd-l shepherd-l merged commit 425f868 into user-model/main Oct 4, 2023
1 check passed
@shepherd-l shepherd-l deleted the fix/proguardAndroidlib branch October 4, 2023 19:39
jinliu9508 pushed a commit that referenced this pull request Feb 23, 2024
jinliu9508 pushed a commit that referenced this pull request Feb 27, 2024
jinliu9508 pushed a commit that referenced this pull request Feb 27, 2024
jinliu9508 pushed a commit that referenced this pull request Feb 27, 2024
jinliu9508 pushed a commit that referenced this pull request Feb 27, 2024
jinliu9508 pushed a commit that referenced this pull request Mar 1, 2024
jinliu9508 pushed a commit that referenced this pull request Mar 4, 2024
jinliu9508 pushed a commit that referenced this pull request Mar 4, 2024
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