-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement Licensing service #2069
Conversation
Hi, |
Also FakeStore can have a different signature of GmsCore and in this case maxSdkVersion on android.permission.GET_ACCOUNTS can break things. |
With the current Kotlin version, which needs to be included for wire (protobuf library), it seems that yes.
I will remove the attribute. |
If you configure kotlin to be JVM 8 compatible, you can keep Java Version to 8 as well. We do the same everywhere else
|
@mar-v-in Thanks, applied. |
It isn't really a problem for me, bit since it contact a Google server many people would probably like to have a setting for it (disabled by default) |
@ale5000-git Since fakestore doesn't have a UI, a setting for this can't easily be implemented. Note that the licensing request is only made if a Google account has been added. (Apps are also expected to have the |
@ale5000-git Setting is added to UI in #2076. |
Now we need the code that checks for the preference since #2076 is merged :-) |
You are absolutely right. I'm getting all confused here with the many branches I'm handling. What is pushed now should make sense but maybe I got something confused yet again 😞 |
I cannot test it currently but it seems fine 😃 |
Just noticed a minor comment issue, here: https://github.com/microg/GmsCore/pull/2069/files#diff-93490511fa787a3e0c583ec081e5e22c2e49da9a8688b70db5b99168fbc7f7bcR40 NOT_LICENSED => The application is licensed to the user, but there is an updated application version ... LICENSED_OLD_KEY => The application is not licensed to the user Only the opposite make sense. |
@fynngodau |
@fynngodau |
@ale5000-git Thank you, seems I swapped them on accident.
Yes, I noticed, I was just very busy and had no time to update this PR like planned. It should be good now, but I don't have the time to test the latest state as well as would be desirable. |
@@ -70,7 +70,7 @@ public void run() { | |||
ignoreIntent.putExtra(INTENT_KEY_IGNORE_PACKAGE_NAME, callerPackageName); | |||
ignoreIntent.putExtra(INTENT_KEY_NOTIFICATION_ID, callerUid); | |||
PendingIntent ignorePendingIntent = PendingIntent.getBroadcast( | |||
context, callerUid * 2 + 1, ignoreIntent, PendingIntent.FLAG_IMMUTABLE | |||
context, callerUid * 2 + 1, ignoreIntent, PendingIntent.FLAG_MUTABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not one bit of an idea why, in my emulator tests, the INTENT_KEY_IGNORE_PACKAGE_NAME
extra doesn't reach the receiver when FLAG_IMMUTABLE
is set.
vending-app/build.gradle
Outdated
@@ -17,6 +17,8 @@ android { | |||
versionCode vendingAppVersionCode | |||
minSdkVersion androidMinSdk | |||
targetSdkVersion androidTargetSdk | |||
|
|||
multiDexEnabled true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the pipeline is failing due to the 65,536 method limit? I'm quite confused, as this is not happening locally. Anyhow, this commit is supposed to enable multidex, but I'm unsure if that's an appropriate solution in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it have this lint error:
/home/runner/work/GmsCore/GmsCore/vending-app/src/main/java/com/android/vending/licensing/LicenseServiceNotificationRunnable.java:95:
Error: Call requires permission which may be rejected by user:
code should explicitly check to see if permission is available (with checkPermission)
or explicitly handle a potential SecurityException [MissingPermission]
notificationManager.notify(callerUid, notification);
Did you run also lint locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ale5000-git I noticed the pipeline failing, but no, I haven't run the lint locally. I likely won't have time to fix this issue in the next days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, take your time.
Your contribution is really appreciated.
With the notifications lint issue solved, here comes the next lint issue:
I'm, uh, again, not sure why this is coming up even though the folder exists since 93e4884, which is included in |
@fynngodau |
The That's why it triggers this lint in every submodule that uses |
@ale5000-git @mar-v-in Thanks for the pointer and explanation. I have disabled the faulty lint. |
@fynngodau An alternative could be to enable minifyEnabled which will strip unused code, decreasing the number of methods and the final size. @mar-v-in |
I'm fine with using Proguard to remove unused methods or classes to reduce file size, however note that doing so imposes the risk to remove actually used methods. As soon as reflection is in place, proguard might be behaving wrong. So you need to take care of proper testing every functionality when activating it. I don't like the renaming and obfuscation that comes with proguard / r8 minification. I'm pretty sure it can be turned off though. Renaming/obfuscation may add nondeterminism and generally makes it harder to link the resulting binary and it's behavior to the source code, which IMO is an important . However, I also don't see any issue with using Multidex. I'm not aware of it causing issues in older versions as long as the androidx multidex support library is used correctly on SDK > 14, which is already our min sdk. |
Is there any use or need of reflection in FakeStore? About renaming and obfuscation I agree. The main issue with multidex on old Android versions is that some methods must be in the first dex and the developer must figure it himself otherwise it break without anyone noticing it (if it just break less used features). |
This shouldn't happen a lot in practice. The app's Application class is loaded first and the build system tries to include it and all the code it uses in the primary There are two cases not covered by this:
The latter is what we've seen in practice causing issues in microG, partially because we don't correctly implement the dynamite loader (which should take care of loading the correct dex files as well). However third-parties are not supposed to load code from the vending package, so this shouldn't be relevant here. |
Instead of
|
@mar-v-in |
The split install service is a service, which is part of the Application. Loading code directly is when the API does not use IPC, but loads and executes the code inside the third-party app itself. This happens for Maps SDK, security provider, cronet, Vision APIs and a few others that are loaded using dynamite. |
In addition to other advantages I think it will also improve performance if there is a single dex and it is small (expecially on 1GB devices where the memory is also slow). If I'm not wrong beside install referrer, dummy in-app billing and this license code there is basically nothing in FakeStore so I suggest to enable this now (since currently it should be really easy to test regressions). |
This reverts commit da7bb3d.
@ale5000-git Multidex is now disabled, and postprocessing as you suggested is now enabled. |
@fynngodau |
Implemented as of now: