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

draft: bump AGP to 7 #32589

Closed
wants to merge 5 commits into from
Closed

draft: bump AGP to 7 #32589

wants to merge 5 commits into from

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Nov 13, 2021

Summary

Bump Android Gradle Plugin to 7.

Changelog

[Android] [Changed] - Bump Android Gradle Plugin to 7.

This will make Java 11 a requirement for users that are either:

  • Cloning react-native to contribute
  • Using react-native while building from source.
  • Creating new project from the template.

Test Plan

CI is green

@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. Contributor A React Native contributor. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 13, 2021
@react-native-bot react-native-bot added the Platform: Android Android applications. label Nov 13, 2021
@analysis-bot
Copy link

analysis-bot commented Nov 13, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 26e30a5
Branch: main

@dulmandakh dulmandakh changed the title bump AGP to 7 draft: bump AGP to 7 Nov 13, 2021
@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

CI is green

I believe you have to unset ANDROID_SDK_HOME as it should not be needed anymore.

@dulmandakh
Copy link
Contributor Author

I believe you have to unset ANDROID_SDK_HOME as it should not be needed anymore.

We need to update the docker image to fix the issue.

@cortinico
Copy link
Contributor

@lunaleaps Just as a FYI for you, this will make Java 11 a requirement for users that are either:

  1. Cloning react-native to contribute
  2. Using react-native while building from source.
  3. Creating new project from the template.

So as soon as this gets merged, we might want to highlight this in the release notes somewhere.

@lunaleaps
Copy link
Contributor

@cortinico This should be for 68 right? I think we should add this kind of information to changelog? Or maybe there needs to be a new type of notice like: [Release Notes] or something that we can pull out to put into a blog post

@cortinico
Copy link
Contributor

@cortinico This should be for 68 right?

Correct.

I think we should add this kind of information to changelog? Or maybe there needs to be a new type of notice like: [Release Notes] or something that we can pull out to put into a blog post

I'd say this should go both in the changelog and in the blogpost that is coming with this release.

As a side note: we should also merge those two
facebook/react-native-website#2800
facebook/react-native-website#2800

@dulmandakh
Copy link
Contributor Author

@cortinico CI is failing, but I couldn't reproduce it on my local machine. Please help

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

@cortinico CI is failing, but I couldn't reproduce it on my local machine. Please help

To reproduce you can rm -rf ReactAndroid/build and invoke the ./gradlew :ReactAndroid:configureNdkBuildDebug.

You should be able to fix it by adding:

    configureNdkBuildDebug.dependsOn(preBuild)
    configureNdkBuildRelease.dependsOn(preBuild)

inside the afterEvaluate{} block (that's sad) of ReactAndroid/build.gradle. If you have other alternatives they will be more than welcome 👍

@cortinico cortinico self-requested a review November 18, 2021 17:31
@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

Sadly it's still failing with a similar issue on the RNTester App

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,468,465 -9,357
android hermes armeabi-v7a 7,767,981 -9,053
android hermes x86 8,937,347 -9,249
android hermes x86_64 8,881,450 -9,218
android jsc arm64-v8a 9,783,149 -9,472
android jsc armeabi-v7a 8,743,865 -9,184
android jsc x86 9,731,971 -9,364
android jsc x86_64 10,332,827 -9,334

Base commit: 26e30a5
Branch: main

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Quick pudate: So this PR is good to land from my point of view. @dulmandakh I've pushed to commits that fixed the failures. Let me know if you're fine with them.

There are also a couple of fixes we need to apply internally to make sure the internal CI is also Green, so it could take a bit longer to land this PR (just as a FYI).

@cortinico
Copy link
Contributor

As a follow-up: this is the bug on AGP's end: https://issuetracker.google.com/issues/207403732

@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 272cfe5.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 24, 2021
nawbc pushed a commit to NawbExplorer/react-native that referenced this pull request Dec 7, 2021
Summary:
Bump Android Gradle Plugin to 7.

## Changelog

[Android] [Changed] - Bump Android Gradle Plugin to 7.

This will make Java 11 a requirement for users that are either:
* Cloning react-native to contribute
* Using react-native while building from source.
* Creating new project from the template.

Pull Request resolved: facebook#32589

Test Plan: CI is green

Reviewed By: ShikaSD

Differential Revision: D32427945

Pulled By: cortinico

fbshipit-source-id: c1ea464d87c3e397616c55154b3d8b1c3ea6c592
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. Contributor A React Native contributor. Merged This PR has been merged. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants