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

Make the Review work for r8 shrinker and Linker set to “Link All” #9

Open
saamerm opened this issue Sep 27, 2020 · 16 comments
Open

Comments

@saamerm
Copy link
Collaborator

saamerm commented Sep 27, 2020

When the linker settings is set to ‘Link All’, occasionally libraries will stop working.
We need to verify that the review works when a developer sets their Android project to Link All.

If it doesn’t work, let’s figure out a solution to the problem.

@saamerm
Copy link
Collaborator Author

saamerm commented Sep 27, 2020

Just verified that currently, the package crashes during runtime without a proguard file as shown in #10

@PatGet is there a way we can change the library to prevent users from having to add those lines to the proguard?

cc: @AntvissMedia

@saamerm saamerm changed the title Verify the Review works for “Link All” Make the Review work for r8 shrinker and Linker set to “Link All” Sep 28, 2020
@PatGet
Copy link
Owner

PatGet commented Sep 28, 2020

Seems like I could add a proguard.txt to the nuget just like dotnet/android#4449 (comment) mentions for the Xamarin.Android.Support.v7.AppCompat nuget package. Let me have a look...

@PatGet
Copy link
Owner

PatGet commented Sep 29, 2020

Can you please test https://www.nuget.org/packages/PlayCore/1.8.0.1-beta? I used your branch, uncommented the 3 lines from the proguard.txt and it still worked. But give it a try yourself please.

@PatGet
Copy link
Owner

PatGet commented Oct 1, 2020

@saamerm have you been able to test?

@saamerm
Copy link
Collaborator Author

saamerm commented Oct 2, 2020

Hmmm it didn't work unfortunately. It acted just like it does when its locally built. It didn't crash the app, but it didn't show the UI this time either

@PatGet
Copy link
Owner

PatGet commented Oct 2, 2020

Your "local" proguard worked just fine, the same proguard from the nuget made it not crash but work differently? That is strange.

@saamerm
Copy link
Collaborator Author

saamerm commented Oct 2, 2020

Are your changes in this repo? Could you please add them so I can retest in both ways, using the nuget and the library code itself

@PatGet
Copy link
Owner

PatGet commented Oct 2, 2020

Pushed the changes just now.

@saamerm
Copy link
Collaborator Author

saamerm commented Oct 5, 2020

Alright so I retested with a newer Visual Studio for Mac install, I was having issues with the older one. I tested the behaviour, if you just build the sample in dx, link none, and as before it works well. Apart from that I tested the following 4 scenarios:

Installing from VS for Mac

No nuget, just the library
The behaviour, if you just build the sample in dx, link none, it works well. But as soon as you build with d8, r8, link all, the app crashes with the "Class Not Found Exception" for tasks shown below even though the pro guard was clearly there in the library project. And as soon as I added a proguard.txt file in the Android project and set that file's build action correctly, there was no crash on load anymore.

Using the NuGet package.
While using the nuget package instead of the library in the sample app, there was no crash in the sample code, even on clicking the review button

Installing from Internal App Sharing

Using the NuGet package.
While using the nuget package instead of the library in the sample app, I built and signed an archive and uploaded the app to Internal app sharing on the Play Store. The app opened alright, but I got the java.lang.NoSuchMethodError on tapping the review button. I found that using adb logcat

AndroidRuntime: java.lang.NoSuchMethodError: no non-static method "Lcom/google/android/play/core/appupdate/e;.getAppUpdateInfo()Lcom/google/android/play/core/tasks/Task;"

@saamerm
Copy link
Collaborator Author

saamerm commented Oct 5, 2020

Not sure why the library wouldn't work, maybe, we need to fix the sample. Were you able to test it and it worked for you?

We also need to add this to the proguard
-keep class com.google.android.play.core.common.PlayCoreDialogWrapperActivity because when the task class functionality provided is used for asynchronous programming, it causes crashes without it.

@PatGet
Copy link
Owner

PatGet commented Oct 5, 2020

I concentrated only on the startup part (which already crashed and that was fixed) so I did not test review functionality with my app.
I guess I need to add the Sample to internal Test, my closed test currently takes two days till its certified by google.

I checked the native SDK ZIP File and it comes with a handfull of proguard files. I guess i need to include all the rules.

I still find it strange that the "3 Lines ProGuard" in the App directly works, in the Nuget not, while changing behaviour. I would have expected that it has the same result or also a NoSuchMethodError .

@PatGet
Copy link
Owner

PatGet commented Nov 18, 2020

I don´t get it to work on my site and have not enough time for dig deeper into how proguard and Nuget work for now.
I would just accept your PR that has the ProGuard built into the Sample if you have no other idea @saamerm .

@saamerm
Copy link
Collaborator Author

saamerm commented Nov 18, 2020

Sure no worries! Yeah let’s merge that in and I’ll try to take a stab at it @PatGet

@saamerm
Copy link
Collaborator Author

saamerm commented Nov 18, 2020

We still need to update the README to talk about the proguard stuff though

@saamerm
Copy link
Collaborator Author

saamerm commented Mar 23, 2021

Haven’t been able to figure this out yet. Spoke with Tomasz (www.Twitter.com/Cheesebaron), maintainer of multiple nuget packages (MVVMCross) with millions of downloads, who said that it’s not possible.
Spoke with Jonathan (www.Twitter.com/redth) who said it is but not able to figure it out. If any one else wants to try, please do!

@PatGet
Copy link
Owner

PatGet commented Mar 23, 2021

Thanks @saamerm . I talked with Jonathan too, also with Jon Douglas. Both said it should work in theory. But it doesn´t and at least for me it has no priority right now. Will keep the issue open.

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

No branches or pull requests

2 participants