issue 3037: Fix android compilation warnings #3086

pacamara · 2018-01-18T21:00:35Z

Fixes #3037 "Fix android compilation warnings"

Summary:

Remove nearly all android build warnings, using sed to modify build.gradle files of node_modules. For node_modules already forked by status there are separate PRs (see refs below).

4 warnings left:

"Task.leftShift(Closure) method has been deprecated" -- from node_modules/realm/android/build.gradle. Have tested fix for this, but cannot use sed as fix is multiline. Should we fork realm or leave this one?
"registerResGeneratingTask is deprecated, use registerGeneratedFolders(FileCollection)" -- from one of the plugins, not from status code.
"warning: the transform cache was reset" -- from XCode build, not android.
"Configuration 'compile' in project ':app' is deprecated. Use 'implementation' instead." -- stackoverflow says could also be from a plugin.

The warnings (now silenced) about unnecessary removal of BLUETOOTH, WRITE_CONTACTS and RECORD_VIDEO permissions were leftovers from when some node_modules were adding these.

Testing notes:

Need to run scripts/setup again before building.

Steps to test:

scripts/setup
build as normal
observe log for build warnings

status: ready

jeluard

Thanks!

I'd rather not have scripts hot patching dependencies, seems brittle.
Would you mind creating issues in the related projects? This also includes the one we already forked as we don't intend to diverge from original upstreams.

jeluard · 2018-01-19T07:14:39Z

android/app/build.gradle

@@ -117,7 +117,6 @@ def getVersionName = { ->

 android {
    compileSdkVersion 24
-    buildToolsVersion "26.0.2"


What is the potential downside here? Looks like there was a reason for this difference in version?

It's not essential to remove this as 26.0.2 is already the minimum supported version for Android Gradle Plugin 3.0.1. But there's no longer a need to specify it explicitly as the gradle plugin now specifies a default buildToolsVersion. Felt it was better to go with that, what do you think?

For the upstream projects these lines must be removed by updateUnforkedNodeModules.sh else we get errors like

WARNING: The specified Android SDK Build Tools version (25.0.3) is ignored, as it is below the minimum supported version (26.0.2) for Android Gradle Plugin 3.0.1. Android SDK Build Tools 26.0.2 will be used. To suppress this warning, remove "buildToolsVersion '25.0.3'" from your build.gradle file, as each version of the Android Gradle Plugin now has a default version of the build tools.

jeluard · 2018-01-19T07:14:58Z

android/app/src/main/AndroidManifest.xml

@@ -21,13 +21,8 @@
    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />

    <!-- these permissions should be removed -->
-    <!-- react-native-orientation adds an unnecessary permission; here we remove it -->
-    <uses-permission android:name="android.permission.BLUETOOTH" tools:node="remove"/>


Are those not necessary anymore?

Yes, looks like BLUETOOTH, WRITE_CONTACTS and RECORD_VIDEO were introduced by upstream projects sometime in the past, but these problems have now been fixed.

~/dev/status-react/node_modules $ grep -r BLUETOOTH * | grep AndroidManifest ~/dev/status-react/node_modules $ grep -r RECORD_VIDEO * | grep AndroidManifest ~/dev/status-react/node_modules $ grep -r WRITE_CONTACTS * | grep AndroidManifest react-native-contacts/README.md:Add permissions to your `android/app/src/main/AndroidManifest.xml` file. Add only the permissions you need (i.e. if you don't need the _WRITE_CONTACTS_ permission then there's no need to add it). ~/dev/status-react/node_modules $

So there is no longer a need to remove them from AndroidManifest.xml, and trying to do so gives the warning.

jeluard · 2018-01-19T07:15:15Z

android/app/src/main/AndroidManifest.xml

    <!-- react-native-camera -->
    <uses-permission android:name="android.permission.RECORD_AUDIO" tools:node="remove"/>
-    <uses-permission android:name="android.permission.RECORD_VIDEO" tools:node="remove"/>
    <!-- React Native unnecessary permissions (https://github.com/facebook/react-native/issues/5886) -->
    <uses-permission android:name="android.permission.READ_PHONE_STATE" tools:node="remove"/>


This one is still necessary?

Yes, looks like it's pulled in by instabug dependency:

~/dev/status-react/node_modules $ grep -r RECORD_AUDIO * | grep AndroidManifest instabug-reactnative/android/build/intermediates/exploded-aar/com.instabug.library/instabug/3.4.0/AndroidManifest.xml: <uses-permission android:name="android.permission.RECORD_AUDIO" /> ~/dev/status-react/node_modules $

Could you update the comment to add this info?

pacamara · 2018-01-19T09:22:09Z

I'd rather not have scripts hot patching dependencies, seems brittle.
Would you mind creating issues in the related projects?

Sure no problem, will start creating the issues. 😄

Note though there is precedent for patching this way, pkg-hacks.js already does this for the javascript files, creating the "hacking..." lines of output during scripts/setup.

jeluard · 2018-01-19T14:11:40Z

@pacamara Right this is part of rn-nodeify but I'd rather move in the direction of removing those.

It's ok if there are more warning, it's better to address those cleanly in upstream dependencies (or remove those completely) in the long run IMO.

pacamara · 2018-01-19T17:22:32Z

@jeluard Ah I see, got it. Have:

created all the issues in upstream projects.
added the comment about RECORD_AUDIO.
also changed from api to implementation for the dependencies directly used by status-react, because api is only recommended for the dependencies used by library projects (such as the upstream node_modules).

To be clear, do you want updateUnforkedNodeModules.sh removed from this PR?

jeluard · 2018-01-19T17:49:42Z

To be clear, do you want updateUnforkedNodeModules.sh removed from this PR?

I would say yes as it could hide stuff by mistake. It's ok if we have some warnings due to 3rd party deps.

Thanks!

pacamara · 2018-01-19T20:30:16Z

Roger that 😄
Have:

moved the script to a gist: https://gist.github.com/pacamara/b69fef08920b06fbfdf536c1be355d05
also updated react-native-svg to 6.0.1-rc.3 in package.json, the maintainer has already fixed it
that required upgrading android/app/build.gradle to compileSdkVersion 27

The build warnings from node_modules are back but do not include react-native-svg.

jeluard · 2018-01-20T09:19:15Z

android/app/build.gradle

@@ -116,8 +116,7 @@ def getVersionName = { ->
 }

 android {
-    compileSdkVersion 24
-    buildToolsVersion "26.0.2"
+    compileSdkVersion 27


Does that impact our target mobile version?

No there is no impact on the target version. 😄
From https://medium.com/google-developers/picking-your-compilesdkversion-minsdkversion-targetsdkversion-a098a0341ebd (article by a dev at google):

It should be emphasized that changing your compileSdkVersion does not change runtime behavior...
Therefore it is strongly recommended that you always compile with the latest SDK.

jeluard · 2018-01-20T09:19:37Z

@pacamara Awesome thanks for the changes!

pacamara · 2018-01-20T12:00:28Z

@jeluard You're welcome! 🍻

pacamara · 2018-01-21T19:05:30Z

@jeluard Wondering if we want to hold off from asking upstream maintainers of node_modules to upgrade to gradle 3? It's a breaking change for freshly generated react native apps, unless manual changes are made to the generated code. See
software-mansion/react-native-svg#581

Perhaps we should wait until React Native itself is upgraded to gradle 3... what do you think?

jeluard · 2018-01-22T07:33:11Z

@pacamara Makes sense. Do you know if RN has planned such a change?

pacamara · 2018-01-22T21:17:51Z

Makes sense. Do you know if RN has planned such a change?

@jeluard Can't find an open issue in react-native relating to a planned upgrade. Issue facebook/react-native#16536 suggests users upgrade gradle manually. Shall I close the issues in upstream node_modules projects? Or could ask maintainers to create a separate gradle 3 tag?

jeluard · 2018-01-23T07:12:09Z

@pacamara Sounds good! I guess we can wait now for more movement from RN. Maintainers are free to close your issues if they feel like it I guess?

dmitryn · 2018-01-23T09:54:29Z

run-android works fine for me (except warnings coming from node_modules which were discussed above).

Building in Android Studio gives me 2 errors though:

/Users/trybeee/Library/Android/sdk/ndk-bundle/build/core/setup-app.mk
Error:(81) Android NDK: Application targets deprecated ABI(s): mips64 armeabi mips    
Error:(82) Android NDK: Support for these ABIs will be removed in a future NDK release.

But it doesn't build in develop branch either.

pacamara · 2018-01-24T01:46:43Z

@dmitryn Pushed another commit to fix the ABI problem. I was not experiencing it because my NDK version was out of date (now upgraded to v16):

set APP_ABI to only non-deprecated ABIs per https://developer.android.com/ndk/guides/abis.html
also upgrade APP_PLATFORM to match minSdkVersion per https://developer.android.com/ndk/guides/application_mk.html

I guess we can wait now for more movement from RN. Maintainers are free to close your issues if they feel like it I guess?

@jeluard Sure! 🍻

Signed-off-by: Dmitry Novotochinov <[email protected]>

pacamara · 2018-02-02T10:02:40Z

Hi @jeluard @dmitryn , it's still showing as pending maintainer confirmation, could you confirm the payout please?

dmitryn · 2018-02-02T14:18:07Z

@pacamara where do you see it? Did you get the tokens to your address?

pacamara · 2018-02-02T14:47:35Z

@dmitryn They just arrived a few minutes ago -- thank you! 🍻

where do you see it?

Previously, the "pending maintainer confirmation" status was on the issue page and also on the SOB page.

pacamara force-pushed the develop_issue3037_fixWarnings branch from c945f27 to b3eb6b6 Compare January 18, 2018 21:59

flexsurfer added android bounty developer-xp labels Jan 19, 2018

jeluard suggested changes Jan 19, 2018

View reviewed changes

pacamara force-pushed the develop_issue3037_fixWarnings branch 2 times, most recently from b94342f to 0272507 Compare January 19, 2018 17:16

pacamara force-pushed the develop_issue3037_fixWarnings branch from 0272507 to 84ec4eb Compare January 19, 2018 20:26

jeluard reviewed Jan 20, 2018

View reviewed changes

jeluard approved these changes Jan 20, 2018

View reviewed changes

jeluard requested review from dmitryn and yenda January 23, 2018 07:12

dmitryn approved these changes Jan 23, 2018

View reviewed changes

pacamara force-pushed the develop_issue3037_fixWarnings branch from 84ec4eb to 3f4ea46 Compare January 24, 2018 01:39

issue 3037: Fix android compilation warnings

6dd585f

Signed-off-by: Dmitry Novotochinov <[email protected]>

dmitryn force-pushed the develop_issue3037_fixWarnings branch from 3f4ea46 to 6dd585f Compare February 1, 2018 12:27

dmitryn merged commit 6dd585f into status-im:develop Feb 1, 2018

dmitryn mentioned this pull request Feb 1, 2018

Add conditions to check for core-contributors in merge PR script #3155

Merged

oskarth mentioned this pull request Feb 5, 2018

[FIX #3037] Fixes some android compilation warnings #3087

Closed

dmitryn mentioned this pull request Mar 29, 2018

Update instabug to 2.8.1 #3725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue 3037: Fix android compilation warnings #3086

issue 3037: Fix android compilation warnings #3086

pacamara commented Jan 18, 2018 •

edited

Loading

jeluard left a comment

jeluard Jan 19, 2018

pacamara Jan 19, 2018

jeluard Jan 19, 2018

pacamara Jan 19, 2018 •

edited

Loading

jeluard Jan 19, 2018

pacamara Jan 19, 2018

jeluard Jan 19, 2018

pacamara Jan 19, 2018

pacamara commented Jan 19, 2018

jeluard commented Jan 19, 2018

pacamara commented Jan 19, 2018 •

edited

Loading

jeluard commented Jan 19, 2018

pacamara commented Jan 19, 2018 •

edited

Loading

jeluard Jan 20, 2018

pacamara Jan 20, 2018

jeluard commented Jan 20, 2018

pacamara commented Jan 20, 2018

pacamara commented Jan 21, 2018

jeluard commented Jan 22, 2018

pacamara commented Jan 22, 2018

jeluard commented Jan 23, 2018

dmitryn commented Jan 23, 2018

pacamara commented Jan 24, 2018

pacamara commented Feb 2, 2018

dmitryn commented Feb 2, 2018

pacamara commented Feb 2, 2018

issue 3037: Fix android compilation warnings #3086

issue 3037: Fix android compilation warnings #3086

Conversation

pacamara commented Jan 18, 2018 • edited Loading

Summary:

Testing notes:

Steps to test:

jeluard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacamara Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacamara commented Jan 19, 2018

jeluard commented Jan 19, 2018

pacamara commented Jan 19, 2018 • edited Loading

jeluard commented Jan 19, 2018

pacamara commented Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeluard commented Jan 20, 2018

pacamara commented Jan 20, 2018

pacamara commented Jan 21, 2018

jeluard commented Jan 22, 2018

pacamara commented Jan 22, 2018

jeluard commented Jan 23, 2018

dmitryn commented Jan 23, 2018

pacamara commented Jan 24, 2018

pacamara commented Feb 2, 2018

dmitryn commented Feb 2, 2018

pacamara commented Feb 2, 2018

pacamara commented Jan 18, 2018 •

edited

Loading

pacamara Jan 19, 2018 •

edited

Loading

pacamara commented Jan 19, 2018 •

edited

Loading

pacamara commented Jan 19, 2018 •

edited

Loading