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

chore: more specific native-api-usage.json file #10131

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

farfromrefug
Copy link
Collaborator

PR Checklist

What is the current behavior?

What is the new behavior?

Fixes/Implements/Closes #[Issue Number].

@nx-cloud
Copy link

nx-cloud bot commented Dec 15, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e816919. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


🟥 Failed Commands
nx run apps-automated:ios --timeout=600
✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@cla-bot cla-bot bot added the cla: yes label Dec 15, 2022
@CatchABus
Copy link
Contributor

CatchABus commented Dec 17, 2022

@farfromrefug What impact will this have for devs that use unregistered apis inside their apps?
For instance, I could make use of java.text.* (e.g. SimpleDateFormat) which I don't see there.

@farfromrefug
Copy link
Collaborator Author

@CatchABus yes if your app was not setting the correct native api usage from the app code it might fail now if it was "covered" by N native-api-usage. But I would not call that a regression in the sense that your app should handle that.
By the almost all community plugins set the correct native-api-usage for android. Not sure about nativescript plugins

@NathanWalker NathanWalker added this to the 8.5 milestone Dec 21, 2022
@jcassidyav
Copy link
Contributor

This is a tough task to test, but what I'm doing is comparing ths to what I get from running https://github.com/jcassidyav/generate-metadata-filter and comparing the gaps in each.

I haven't gone through everything but so far I have found these missing rules:

"android.content:ClipData",
"android.content:ContentResolver",
"android.graphics.drawable:StateListDrawable",
"android.webkit:URLUtil"

@farfromrefug
Copy link
Collaborator Author

farfromrefug commented Jan 20, 2023

@jcassidyav nice will add those. BTW what is the real difference between android.content:ContentResolver and android.content:ContentResolver* . From what i have tested metadata are even bigger with android.content:ContentResolver which does not really make sense to me

EDIT: it seems my old tests about * were wrong. I tested again and * behave as expected and is thus not needed here.

@jcassidyav
Copy link
Contributor

@farfromrefug That is great at least it makes sense.

I think though that the * were hiding another problem, e.g.

     "android.widget:ImageView",
     "android.widget.ImageView:ScaleType",

Does not allow access to android.widget.ImageView.ScaleType but this works:

     "android.widget:ImageView",
     "android.widget:ImageView.ScaleType",

I think it is
LHS = package name
RHS = class name

previously android.widget:ImageView*, was allowing the class name ImageView.ScaleType.

@farfromrefug
Copy link
Collaborator Author

@jcassidyav wow that is a big issue ! thanks for catching this. I was updating all my plugins with that change, might have broken stuff!

Also removed `androidx.appcompat:R.attr` as i dont think we are using it
@farfromrefug
Copy link
Collaborator Author

@jcassidyav i updated the file. Does that look good to you?
Also removed androidx.appcompat:R.attr as i dont think we need it

@jcassidyav
Copy link
Contributor

@farfromrefug When I run the generator against core I get android-native-api-usage.zip.

It may be too aggressive in picking up what is required but it is picking up some rules which are definitely required, I haven’t had a chance to look at the discrepancies yet.

It is easy to spot things which are just newed up in the typescript, once they have been highlighted, but if it is the type of a parameter to something else then very difficult.

But here is one example that is required:

"android.view.accessibility:AccessibilityManager.TouchExplorationStateChangeListener",

Also "android.inputmethodservice.Keyboard:Key" should be "android.inputmethodservice:Keyboard.Key",

@farfromrefug
Copy link
Collaborator Author

@jcassidyav OK great i LL take a look

@jcassidyav
Copy link
Contributor

@farfromrefug I was able to get the /apps/automated tests running with the usage enabled by adding:

These may be only used in the test code ( hard to tell for sure):

  "androidx.appcompat.content.res:AppCompatResources",
        "android.os:MessageQueue",
        "android.os:Message",
        "android.view:ViewGroup.MarginLayoutParams",

These are definitly used in core:

        "android.graphics:ColorFilter",
        "android.widget:TabHost.TabSpec",
        "androidx.transition:Explode",
        "java.lang:Runtime",
        "android.widget:FrameLayout.LayoutParams",
        "android.view:View.AccessibilityDelegate",
        "java.lang.reflect:Method",
        "java.lang.reflect:AccessibleObject",
        "android.widget:HorizontalScrollView",
        "androidx.core.widget:NestedScrollView",
        "android.view:WindowManager.LayoutParams",
        "android.view:WindowManager",
}

@farfromrefug
Copy link
Collaborator Author

@jcassidyav thanks a lot. I was adding missing by testing in my production apps. I will run the tests too!
Thanks for helping on this!

@farfromrefug
Copy link
Collaborator Author

@jcassidyav
about android.graphics:ColorFilter i dont think we need it. We only use getColorFilter in the tests and setColorFilter is only used with a color.

"android.widget:FrameLayout.LayoutParams", i dont see where it is used. We use CommonLayoutParams which inherits android.widget.FrameLayout.LayoutParams but we dont actually use any method from android.widget.FrameLayout.LayoutParams

"java.lang.reflect:AccessibleObject", i dont think we use that class. we use java.lang.reflect.Field which inherits java.lang.reflect:AccessibleObject but we dont seem to be needing to marshall it. Though we do use it in the tests!

"android.widget:HorizontalScrollView" we use a class inheriting this but i dont think we need the methods from that parent class

"android.view:WindowManager.LayoutParams","android.view:WindowManager", cant find where we use those. Do you know?

@jcassidyav
Copy link
Contributor

jcassidyav commented Feb 7, 2023

@farfromrefug

java.lang.reflect:AccessibleObject from my tests is required because we call setAccessible.
android.widget:HorizontalScrollView smoothScrollTo is called.
android.view:WindowManager.LayoutParams comes from here
android.graphics:ColorFilter I think you are correct I didn't look closely at what the params to setColorFilter were.

The others, you are probably correct also, the tests fall apart so completly I thought they were used in core. That said for a small cost maybe they are worth keeping to let the tests run?

@farfromrefug
Copy link
Collaborator Author

farfromrefug commented Feb 7, 2023

@jcassidyav smoothScrollTo you are right! HorizontalScrollView does NOT inherite NestedScrollView1
java.lang.reflect:AccessibleObject you are right too used by list-picker!
android.view:WindowManager.LayoutParams i just tested and that line works without android.view:WindowManager.LayoutParams. The reason is that the softInputMode returns an int even if it is part of android.view:WindowManager.LayoutParams. So the getter + setter will work

About the test ones i actually added them to the apps App_Resources so we should not need to add them to the core.

As a note on what all that can do, i pushed metadata filtering + proguard to its best (might even do better):

  • use very detailed whitelist for N and plugins
  • use whitelist to auto generate proguard file
  • added and use a new custom field proguard_blacklist to my app native-api-usage.json which i use to tell gradle which classes it can fully remove
    The result are those:
  • without native-api-usage/gradle: 18.4mb apk and 2900ms app cold start(lowest)
  • with native-api-usage/gradle: 14.4mb apk and 2700ms app cold start(lowest)

Seems to me it is worse going as detailed as possible

@jcassidyav
Copy link
Contributor

@farfromrefug Strange, I get
Caused by: java.lang.NoSuchMethodError: no non-static method "Landroid/view/Window;.getAttributes()Landroid/view/ViewGroup$LayoutParams;"

when calling getAttributes() on a window without specifying android.view:WindowManager.LayoutParams in the usage

@farfromrefug
Copy link
Collaborator Author

@jcassidyav indeed weird I will investigate. Thanks !

@farfromrefug
Copy link
Collaborator Author

@jcassidyav i added all the fixes we talked about. Even android.view:WindowManager.LayoutParams cause even though it does not crash here, your explanation makes sense

@NathanWalker NathanWalker modified the milestones: 8.5, 9.0 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants