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

Bump Android SDK to 28 #20026

Closed
wants to merge 24 commits into from
Closed

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Jul 4, 2018

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-bot facebook-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
@dulmandakh
Copy link
Contributor Author

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
Copy link
Contributor

gengjiawen commented Jul 4, 2018

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
Copy link
Contributor

AndrewJack commented Jul 4, 2018

@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
Contributor

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

@MarkOSullivan94
Copy link

MarkOSullivan94 commented Jul 4, 2018

@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
Contributor

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

    public static ImmutableList<String> makeCompile(@NonNull CompileResourceRequest request) {
        ImmutableList.Builder<String> parameters = new ImmutableList.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.
            String lowerName = 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());

        return parameters.build();
    }

@gengjiawen
Copy link
Contributor

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

@kelset
Copy link
Contributor

kelset commented Jul 4, 2018

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
Copy link
Contributor

AndrewJack commented Jul 4, 2018

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
Copy link
Contributor Author

dulmandakh commented Jul 5, 2018

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

@facebook-github-bot
Copy link
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

jwaldrip commented Aug 4, 2018

Any update here?

@gengjiawen
Copy link
Contributor

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

Copy link
Contributor

@gengjiawen gengjiawen left a comment

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.

@dulmandakh dulmandakh changed the title Bump Android SDK to 27 Bump Android SDK to 28 Aug 7, 2018
@dulmandakh
Copy link
Contributor Author

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 hramos August 7, 2018 03:59
implementation 'com.google.code.findbugs:jsr305:3.0.2'
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
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.

build.gradle Outdated
}
dependencies {
classpath 'com.android.tools.build:gradle:2.3.3'
classpath 'com.android.tools.build:gradle:3.1.3'
Copy link
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
Contributor Author

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
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
Contributor Author

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
Contributor Author

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

implementation 'com.google.code.findbugs:jsr305:3.0.2'
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
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 ?

Copy link
Contributor

@gengjiawen gengjiawen left a comment

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
Contributor

maybe change the title ?

@dulmandakh
Copy link
Contributor Author

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

@dulmandakh dulmandakh closed this Aug 9, 2018
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 facebook#20026.
Pull Request resolved: facebook#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 facebook/react-native#20026.
Pull Request resolved: facebook/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 facebook/react-native#20026.
Pull Request resolved: facebook/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 facebook#20026.
Pull Request resolved: facebook#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 facebook#20026.
Pull Request resolved: facebook#20586

Differential Revision: D9414901

Pulled By: hramos

fbshipit-source-id: 580338e62a924c214accc5d944f17c81ad9e3f9f
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants