Skip to content

Bump Android SDK to 28 - #20026

Closed
dulmandakh wants to merge 24 commits into
react:masterfrom
dulmandakh:bump-sdk-27
Closed

Bump Android SDK to 28#20026
dulmandakh wants to merge 24 commits into
react:masterfrom
dulmandakh:bump-sdk-27

Conversation

@dulmandakh

@dulmandakhdulmandakh commented Jul 4, 2018

Copy link
Copy Markdown
Contributor

This PR will bump Android SDK to 28, build tools to 28.0.2 and support library to 27.1.1. But issue related to aapt2 is breaking RNTester app.

Solution

After some research I found that we need to compile (or copy) resources using aapt2 compile command. aapt2 is located in ${sdkDir}/${buildToolsVersion} directory.

And resource bundling is handled in copy function of the local-cli/bundle/saveAssets.js file. In react.gradle file we can access sdkDir via project.android.sdkDirectory, buildToolsVersion via project.android.sdkDirectory.

The solution might be pass aapt2 path to bundler and use it to compile resources.

I'm not proficient in gradle, and would like to discuss to find better solution.

Please help @gengjiawen@hramos@kelset

Resources

Test Plan:

Everything should build and run as usual.

Release Notes:

[ANDROID] [ENHANCEMENT] [SDK] - Bump to 27

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 4, 2018
@dulmandakhdulmandakh mentioned this pull request Jul 4, 2018
13 tasks
@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

There is no major or breaking change in 27, as it carries Oreo codename and 8.1 version number.

Once merged I would like to focus more on NDK and JSC part.

@gengjiawen

gengjiawen commented Jul 4, 2018

Copy link
Copy Markdown
Contributor

Aapt2 through path is not the right way.
I am looking into gradle dsl to find the related api. But I am not expert too. Any chance you know something we can hook @janicduplessis ?

@AndrewJack

AndrewJack commented Jul 4, 2018

Copy link
Copy Markdown
Contributor

@CFKevinRef updated the gradle file to work with Android gradle plugin v3, but it was reverted.

Commit: d16ff3b
revert: 3f8a04b

Gradle v3 file: https://github.com/facebook/react-native/blob/d16ff3bd8b92fa84a9007bf5ebedd8153e4c089d/react.gradle

try adding that back in?

@gengjiawen

Copy link
Copy Markdown
Contributor

Maybe not adding it back, since it's not a clean fix. We need aapt2 to support instant run.

@MarkOSullivan94

MarkOSullivan94 commented Jul 4, 2018

Copy link
Copy Markdown

@gengjiawen do we need to support instant run? If not, could we not disable AAPT2 for now?

If you are experiencing issues while using AAPT2, you can disable it by setting android.enableAapt2=false in your gradle.properties file and restarting the Gradle daemon by running ./gradlew --stop from the command line.

Source: https://developer.android.com/studio/build/gradle-plugin-3-0-0

If we don't need to support it, I don't see why we couldn't do what @AndrewJack suggested and add that commit back in.

@gengjiawen

Copy link
Copy Markdown
Contributor

This is some snippet in AaptV2CommandBuilder, maybe related. But I have find the method we can get the aapt2 path.

publicstaticImmutableList<String> makeCompile(@NonNullCompileResourceRequestrequest) {
ImmutableList.Builder<String> parameters = newImmutableList.Builder<>();
if (request.isPseudoLocalize()) {
parameters.add("--pseudo-localize");
}
if (!request.isPngCrunching()) {
// Only pass --no-crunch for png files and not for 9-patch files as that breaks them.StringlowerName = request.getInputFile().getPath().toLowerCase(Locale.US);
if (lowerName.endsWith(SdkConstants.DOT_PNG)
&& !lowerName.endsWith(SdkConstants.DOT_9PNG)) {
parameters.add("--no-crunch");
}
}
parameters.add("--legacy");
parameters.add("-o", request.getOutputDirectory().getAbsolutePath());
parameters.add(request.getInputFile().getAbsolutePath());
returnparameters.build();
}

@gengjiawen

Copy link
Copy Markdown
Contributor

The commit doesn't handle aapt2. If you disable aapt2, current code works fine.

@kelset

Copy link
Copy Markdown
Contributor

Side-related to appt2 (and reason why I think we should care/have aapt2 support): #16906 (and by extension react-navigation/react-navigation#3097)

@AndrewJack

AndrewJack commented Jul 4, 2018

Copy link
Copy Markdown
Contributor

Could keep both scripts in react native, if there's a concern that it'll break a lot of builds.
react.gradle &react-v3.gradle

We are currently using the linked gradle script with aapt2 enabled.

Additionally the current react.gradle script doesn't work with Android Gradle plugin 3.1.3 anyway, aapt2 enabled or not.

@dulmandakh

dulmandakh commented Jul 5, 2018

Copy link
Copy Markdown
ContributorAuthor

Also App Bundling is not working when android.enableAapt2=false, and we need to fix the issue.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@dulmandakh 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.

@jwaldrip

Copy link
Copy Markdown

Any update here?

@gengjiawen

Copy link
Copy Markdown
Contributor

With #20526 get merged, @dulmandakh Let's rebase code and test again ?

@gengjiawengengjiawen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nope, all has better IDE support.

@dulmandakhdulmandakh changed the title Bump Android SDK to 27Bump Android SDK to 28Aug 7, 2018
@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

Bumped Android SDK to 28, which was released just yesterday. We still use Support Library 27.x because 28.x is not stable yet.

@dulmandakh
dulmandakh requested a review from hramosAugust 7, 2018 03:59
implementation 'com.squareup.okhttp3:okhttp:3.10.0'
implementation 'com.squareup.okhttp3:okhttp-urlconnection:3.10.0'
implementation 'com.squareup.okio:okio:1.14.0'
compile 'org.webkit:android-jsc:r174650'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean most of them, like okhttp, fresco, appcompat. And also the compile is still exist.

Comment threadbuild.gradle Outdated
}
dependencies {
classpath 'com.android.tools.build:gradle:2.3.3'
classpath 'com.android.tools.build:gradle:3.1.3'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe 3.1.4.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

IIRC, 3.1.3 is the latest version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nope, 3.1.4. It's out today.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@gengjiawen done, thanks 👍

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

Android CI was green before Appveyor change requested by @gengjiawen, and it's broken due to issue on Circle CI.

implementation 'com.squareup.okhttp3:okhttp:3.10.0'
implementation 'com.squareup.okhttp3:okhttp-urlconnection:3.10.0'
implementation 'com.squareup.okio:okio:1.14.0'
compile 'org.webkit:android-jsc:r174650'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you change it to api or implementation, what's the error ?

@gengjiawengengjiawen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

jsc has problem of building using api ?

@gengjiawen

Copy link
Copy Markdown
Contributor

maybe change the title ?

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

Closing this PR, because I decided to break it into smaller and easy to review PRs.

facebook-github-bot pushed a commit that referenced this pull request Aug 21, 2018
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of #20026.
Pull Request resolved: #20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
@dulmandakh
dulmandakh deleted the bump-sdk-27 branch August 22, 2018 03:15
kelset pushed a commit that referenced this pull request Aug 23, 2018
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of #20026.
Pull Request resolved: #20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
aleclarson pushed a commit to aleclarson/react-native that referenced this pull request Sep 16, 2018
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of react#20026.
Pull Request resolved: react#20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of react/react-native#20026.
Pull Request resolved: react/react-native#20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Apr 14, 2019
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of react/react-native#20026.
Pull Request resolved: react/react-native#20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of react#20026.
Pull Request resolved: react#20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
This PR bumps Android Support Library version to 27.1.1.
FYI, originally was a part of react#20026.
Pull Request resolved: react#20586
Differential Revision: D9414901
Pulled By: hramos
fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Platform: AndroidAndroid applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@dulmandakh@gengjiawen@AndrewJack@MarkOSullivan94@kelset@facebook-github-bot@jwaldrip@react-native-bot