-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Conversation
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
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! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
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. |
@hramos any news on this? |
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. |
@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. |
I was referring to external tests. They run on Circle, you can find the definitions at 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. |
RNTester/android/app/build.gradle
Outdated
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' |
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.
👍 We're using 27.0.2 internally.
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.
Good to know.
Typo fix, but mostly I want Circle to re-run tests.
@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 looks like Circle CI is needing to upgrade the emulator and download the Android SDK 27 for it to pass test_android
|
@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" |
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.
Actually, we rely on buildToolsVersion
in the CI. That's why test_android
is failing.
MAJOR=`echo $BUILD_TOOLS_VERSION | sed 's/\..*//'` |
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.
The new gradle version will choose the build tools version itself, but specifying it here won't (afaik) hurt either.
@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 |
ReactAndroid/build.gradle
Outdated
@@ -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' |
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.
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" /> |
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.
This is useless.
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.
should be completely removed. Also libraries should not even define targetSdk
@msand Any time to resolve the conflict ? |
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 #18095 would also be affected by this. |
@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. |
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. |
Don't you need to update react.gradle file too? That all depends on implementation details of the old version of Android tools. |
# Conflicts: # build.gradle # local-cli/link/android/patches/makeBuildPatch.js # scripts/android-setup.sh
@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 |
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.
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.
@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. |
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.
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'
closing this PR because SDK 27, support library 27.x and android gradle plugin 3.1 is landed in master. |
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
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
Test Plan
Run CI
Related PRs
Update docs
Release Notes
[ANDROID] [ENHANCEMENT] [Framework] - Upgrade to latest stable and hopefully less vulnerable versions.