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

Upgrade Gradle and Android SDK versions. #17747

Closed
wants to merge 7 commits into from
Closed

Conversation

msand
Copy link
Contributor

@msand msand commented Jan 26, 2018

Motivation

Address Path Traversal Vulnerability
Improve build speeds by restricting which dependencies leak their APIs to other modules.
Improve performance on up to date Android phones.
Decrease memory use.

https://developer.android.com/studio/releases/gradle-plugin.html#3-0-0
https://developer.android.com/studio/build/gradle-plugin-3-0-0-migration.html#new_configurations

To maintain your application along with each Android release, you should increase the value of this attribute to match the latest API level, then thoroughly test your application on the corresponding platform version.
https://developer.android.com/guide/topics/manifest/uses-sdk-element.html

https://medium.com/google-developers/picking-your-compilesdkversion-minsdkversion-targetsdkversion-a098a0341ebd

https://developer.android.com/training/basics/supporting-devices/platforms.html#sdk-versions

Align with FBSDK:
Facebook SDK for Android Changelog 4.x

Changelog and release notes for the Facebook SDK for Android.
4.30.0 - January 24, 2018
Facebook SDK
Added

Protection against Path Traversal Vulnerability: https://support.google.com/faqs/answer/7496913
Support for variant-aware dependency management in Gradle 4.1 and Android Studio 3.0

Path Traversal Vulnerability
What’s happening

Beginning January 16th, 2018, Google Play will block publishing of any new apps or updates which contain the Path traversal Vulnerability. Your published APK version will remain unaffected, however any updates to the app will be blocked unless you address this vulnerability.

Android SDK versions
https://developer.android.com/sdk/api_diff/24/changes.html
https://developer.android.com/sdk/api_diff/25/changes.html
https://developer.android.com/sdk/api_diff/26/changes.html
https://developer.android.com/sdk/api_diff/27/changes.html

https://developer.android.com/about/versions/oreo/android-8.0.html
Various media api improvements
Picture-in-Picture mode
Partial Java 8 support
https://developer.android.com/studio/write/java8-support.html

https://developer.android.com/about/versions/oreo/android-8.1.html

Memory optimizations. Improved memory usage across the platform to ensure that apps can run efficiently on devices with 1GB or less RAM.

Flexible targeting options. New hardware feature constants to let you target the distribution of your apps to normal or low-RAM devices through Google Play.

Google Play.While all apps will be available on devices running Android Oreo (Go edition), Google Play will give visibility to apps specifically optimized by developers to provide a great experience for billions of people with the building for billions guidelines.

Test Plan

Run CI

Related PRs

Update docs

Release Notes

[ANDROID] [ENHANCEMENT] [Framework] - Upgrade to latest stable and hopefully less vulnerable versions.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 26, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@hramos
Copy link
Contributor

hramos commented Jan 26, 2018

Thanks for opening the PR. We want to make sure the Android SDK version we use here matches the one we use at Facebook. I'll see what has been done internally and I will get back to this.

@nitaliano
Copy link

@hramos any news on this?

@hramos
Copy link
Contributor

hramos commented Feb 8, 2018

Not yet. Current priority is getting all of our tests back to green. Until then, we cannot make this sort of change to the codebase.

@nitaliano
Copy link

nitaliano commented Feb 12, 2018

@hramos are you referring to internal fb tests? If you're talking about external facing tests is there any place where we can all break them down and hand them out to people to help get them to start passing? I think staying up to date with the native tools is very important so I would be happy to help in anyway that I can.

@hramos
Copy link
Contributor

hramos commented Feb 14, 2018

I was referring to external tests. They run on Circle, you can find the definitions at .circleci/config.yml. These are now green as of last week.

We do have a large suite of tests that run internally, that for the most part concern the use of React Native in our own product's surfaces. I am currently focusing on catching test breakages internally at Facebook before they land and sync to GitHub. There are several reasons this may happen even with our internal tests in place: we use our own test infrastructure that is quite different from Circle; we may be using newer buck builds that have yet to reach open source; we have a monorepo that depends on the latest Metro packager which may also not yet exist in npm; and so on.

While we use buck to build the project internally, we do not do so from within the same open source React Native repo. I am working on resolving that blind spot, in order to catch buck failures that have broken our Circle tests in the past.

Which brings me to this PR: as I am adding buck back to our internal tests, I am running into issues due to the use of different Android SDK versions between internal/open source. Once I solve this, I should come back to this PR and suggest which exact versions we should be using in order to match internal.

Stay tuned.

compile fileTree(dir: 'libs', include: ['*.jar'])
compile 'com.android.support:appcompat-v7:23.0.1'
api fileTree(dir: 'libs', include: ['*.jar'])
api 'com.android.support:appcompat-v7:27.0.2'
Copy link
Contributor

@hramos hramos Feb 14, 2018

Choose a reason for hiding this comment

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

👍 We're using 27.0.2 internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know.

Typo fix, but mostly I want Circle to re-run tests.
@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
@react-native-bot react-native-bot added Android Ran Commands One of our bots successfully processed a command. labels Mar 14, 2018
@facebook-github-bot
Copy link
Contributor

@msand I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@hramos hramos added Type: Enhancement A new feature or enhancement of an existing feature. and removed 🌟Feature Request labels Mar 19, 2018
@MarkOSullivan94
Copy link

@hramos looks like Circle CI is needing to upgrade the emulator and download the Android SDK 27 for it to pass test_android

Your emulator is out of date, please update by launching Android Studio:

  • Start Android Studio
  • Select menu "Tools > Android > SDK Manager"
  • Click "SDK Tools" tab
  • Check "Android Emulator" checkbox
  • Click "OK"

Error: could not find version of the Android SDK.
Specifically, the directory /opt/android/sdk/platforms/android- does not exist.
You probably need to specify the right version using the SDK Manager from within Android Studio.

@facebook-github-bot
Copy link
Contributor

@msand I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@@ -231,12 +231,11 @@ task packageReactNdkLibsForBuck(dependsOn: packageReactNdkLibs, type: Copy) {
}

android {
compileSdkVersion 23
buildToolsVersion "23.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we rely on buildToolsVersion in the CI. That's why test_android is failing.

MAJOR=`echo $BUILD_TOOLS_VERSION | sed 's/\..*//'`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new gradle version will choose the build tools version itself, but specifying it here won't (afaik) hurt either.

@charpeni
Copy link
Contributor

charpeni commented May 3, 2018

@MarkOSullivan94 Thanks for the heads up.

The error is located here: #17747.

Also, it looks like we may have to clear the CircleCI cache, because new Android deps are not installed if $ANDROID_HOME/installed-dependencies is there.

@@ -292,7 +291,7 @@ android {
dependencies {
compile fileTree(dir: 'src/main/third-party/java/infer-annotations/', include: ['*.jar'])
compile 'javax.inject:javax.inject:1'
compile 'com.android.support:appcompat-v7:23.0.1'
api 'com.android.support:appcompat-v7:27.0.2'
compile 'com.facebook.fbui.textlayoutbuilder:textlayoutbuilder:1.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

replace compile with implementation

@@ -3,7 +3,7 @@
package="com.facebook.react.tests.gradle"
android:versionCode="1"
android:versionName="1.0" >
<uses-sdk android:targetSdkVersion="7" />
<uses-sdk android:targetSdkVersion="27" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is useless.

Choose a reason for hiding this comment

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

should be completely removed. Also libraries should not even define targetSdk

@gengjiawen
Copy link
Contributor

@msand Any time to resolve the conflict ?

@MarkOSullivan94
Copy link

MarkOSullivan94 commented May 30, 2018

I haven't looked into the source code of create-react-native-app in too much detail so perhaps someone here would know the answer to my question: How will this change affect create-react-native-app?

I know the eject command builds the android and ios directories for RN projects but how would it know what gradle version / target API version to use?

#18095 would also be affected by this.

@facebook-github-bot
Copy link
Contributor

@msand I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@MarkOSullivan94
Copy link

I think Circle CI is broken. The checks it's showing are from 1 month ago: https://circleci.com/gh/facebook/react-native/41794?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Current status: 4 failing and 5 successful checks.

@tasomaniac
Copy link

Don't you need to update react.gradle file too? That all depends on implementation details of the old version of Android tools.

msand added 2 commits June 8, 2018 23:26
# Conflicts:
#	build.gradle
#	local-cli/link/android/patches/makeBuildPatch.js
#	scripts/android-setup.sh
@msand
Copy link
Contributor Author

msand commented Jun 8, 2018

@gengjiawen @tasomaniac Fixed conflicts and addressed feedback

@@ -2,4 +2,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-2.14.1-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.5.1-all.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to myself or whoever imports this to FB: If this PR is approved, please see D8459570 for an example of how to update the offline Gradle cache as part of the import process. This will needed in order to land this PR without breaking the internal React Native OSS Android test.

@facebook-github-bot
Copy link
Contributor

@msand I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

Copy link
Collaborator

@Titozzz Titozzz left a comment

Choose a reason for hiding this comment

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

This PR also need to update the cli link and unlink command, that currently use a regex onto compile 'com.package' instead of implementation 'com.package'

@dulmandakh
Copy link
Contributor

closing this PR because SDK 27, support library 27.x and android gradle plugin 3.1 is landed in master.

@dulmandakh dulmandakh closed this Aug 24, 2018
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. Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.