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 build error and runtime error after proguard enabled. #5146

Closed
wants to merge 4 commits into from

Conversation

tdzl2003
Copy link
Contributor

@tdzl2003 tdzl2003 commented Jan 6, 2016

Fix:

  1. :app:packageRelease FAILED caused by proguard exception: java.io.IOException: Please correct the above warnings first.
  2. Fix runtime exception
java.lang.ExceptionInInitializerError
   at com.facebook.react.ReactInstanceManagerImpl.recreateReactContextInBackgroundFromBundleFile(ReactInstanceManagerImpl.java:308)

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek to be a potential reviewer.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 6, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

Thanks for the fix!

@@ -40,10 +40,13 @@

-keep class * extends com.facebook.react.bridge.JavaScriptModule { *; }
-keep class * extends com.facebook.react.bridge.NativeModule { *; }
-keep class * { native <methods>; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try changing this to?

# For native methods, see http://proguard.sourceforge.net/manual/examples.html#native
-keepclasseswithmembernames class * {
    native <methods>;
}

keep class gives 7.1MB APK, keepclasseswithmembernames 6.8MB.

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 tried but it didn't work. There's still errors:

E/dalvikvm( 1032): ERROR: couldn't find native method
E/dalvikvm( 1032): Requested: Lcom/facebook/react/bridge/ReadableNativeArray;.getType:(I)Lcom/facebook/react/bridge/ReadableType;

Maybe there's another problem. I'll take a look and give a update for this PR.

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 read the code decompiled from classes-proguard. All other methods is fine but getType is missing. I don't know why...

@tdzl2003
Copy link
Contributor Author

tdzl2003 commented Jan 6, 2016

-keepclassmembers class * {
    native <methods>;
}

this work for me. We should not only keep their names, but also keep native methods self, otherwise it may be stripped if not used.

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

Does this work as noted in the docs (http://proguard.sourceforge.net/manual/examples.html#native)?

-keepclasseswithmembernames,includedescriptorclasses class * {
    native <methods>;
}

@facebook-github-bot
Copy link
Contributor

@tdzl2003 updated the pull request.

@tdzl2003
Copy link
Contributor Author

tdzl2003 commented Jan 6, 2016

Nope. getType() method is still stripped. -keepclassmembers,includedescriptorclasses would be better to prepare the future.

I think getType is stripped but other methods are kept because getType wasn't used in modules provided by MainReactPackage

@tdzl2003
Copy link
Contributor Author

tdzl2003 commented Jan 6, 2016

includedescriptorclasses has no effect now because all the classes used by native method have annotation DoNotStrip

@facebook-github-bot
Copy link
Contributor

@tdzl2003 updated the pull request.

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

ok ok thanks for looking!

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1678391639113596/int_phab to review.

@ghost ghost closed this in 5f3d08d Jan 7, 2016
csudanthi pushed a commit to healthiqeng/react-native that referenced this pull request Jan 16, 2016
Summary:
Fix:
1. :app:packageRelease FAILED caused by proguard exception: `java.io.IOException: Please correct the above warnings first.`
2. Fix runtime exception
```
java.lang.ExceptionInInitializerError
   at com.facebook.react.ReactInstanceManagerImpl.recreateReactContextInBackgroundFromBundleFile(ReactInstanceManagerImpl.java:308)
```
Closes facebook#5146

Reviewed By: svcscm

Differential Revision: D2807252

Pulled By: mkonicek

fb-gh-sync-id: 03d004405c7cca14a71230086b95351cfacbc055
mkonicek pushed a commit that referenced this pull request Jan 18, 2016
Summary:
Fix:
1. :app:packageRelease FAILED caused by proguard exception: `java.io.IOException: Please correct the above warnings first.`
2. Fix runtime exception
```
java.lang.ExceptionInInitializerError
   at com.facebook.react.ReactInstanceManagerImpl.recreateReactContextInBackgroundFromBundleFile(ReactInstanceManagerImpl.java:308)
```
Closes #5146

Reviewed By: svcscm

Differential Revision: D2807252

Pulled By: mkonicek

fb-gh-sync-id: 03d004405c7cca14a71230086b95351cfacbc055
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
Fix:
1. :app:packageRelease FAILED caused by proguard exception: `java.io.IOException: Please correct the above warnings first.`
2. Fix runtime exception
```
java.lang.ExceptionInInitializerError
   at com.facebook.react.ReactInstanceManagerImpl.recreateReactContextInBackgroundFromBundleFile(ReactInstanceManagerImpl.java:308)
```
Closes facebook/react-native#5146

Reviewed By: svcscm

Differential Revision: D2807252

Pulled By: mkonicek

fb-gh-sync-id: 03d004405c7cca14a71230086b95351cfacbc055
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants