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

[RNMobile] Upgrade Gradle to 8.2.1 and AGP to 8.1.0 #52872

Merged

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jul 24, 2023

Related PR:

What?

Apply needed changes to upgrade Gradle to version 8.2.1 and AGP (Android Gradle Plugin) to version 8.0.2 8.1.0.

Why?

This is needed to match the versions that will be used in the Android app. Otherwise, we won't be able to do composite builds, which are needed during the development workflow to test native changes without publishing the Android binaries.

Closes wordpress-mobile/gutenberg-mobile#5990.

How?

This PR contains several changes introduced by @ParaskP7 in the forked branch build/upgrade-gradle-to-8.2.1-and-agp-to-8.0.2. The main changes are:

  • Upgrade Gradle and AGP in react-native-aztec package.
  • Upgrade Gradle and AGP in react-native-bridge package.
  • Upgrade Gradle and AGP in react-native-editor package.
  • Extract versions into groups to make version management easier.
  • Move package from Android manifest files to build files.
  • Enable build config build feature.
  • Include correct react-native-aztec project in bridge.
  • Update Android CI jobs to use Java 17.
  • Add newer version of react-native-gradle-plugin via package @react-native/gradle-plugin.
  • Remove patch for react-native-gradle which is no longer need it.

Testing Instructions

  • Check that the Gutenberg demo app can be built.
  • Smoke test the editor.

⚠️ NOTE: AGP 8.1.0 requires Android Studio Giraffe | 2022.3.1 or above.

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

ParaskP7 added 30 commits July 21, 2023 18:42
FYI: You will notice an additional 'aztec' related 'junit' version. This
is because the node modules are using an older version of JUnit, the
'4.12', that is, comparing to the newer '4.13' version that the 'aztec'
modules are using. As such, and in order to avoid behavior changes, two
such version were extracted to the root 'build.gradle' file with an
accompanying TODO for the future.
Release Notes: https://docs.gradle.org/8.1.1/release-notes.html
Command: ./gradlew wrapper --gradle-version=8.1.1
--distribution-type=all

------------------------------------------------------------------------

FYI: You will notice that the associated 'gradle-wrapper.jar', 'gradlew'
and 'gradlew.bat' files are not updated. This is because of the breaking
changes that this upgrade introduces, including the 'AGP' upgrade and
its associated breaking changes (namespace, build option, etc). As such,
this Gradle upgrade command is only half done.

PS: When all the breaking changes are resolved, this Gradle command will
be issued again, which should fully complete this upgrade.
This is an AGP version '8.0' breaking change that requires 'namespace'
in module-level build scripts, see build failure below:

------------------------------------------------------------------------

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project
 '@wordpress_react-native-aztec'.
> Failed to notify project evaluation listener.
   > Could not create an instance of type
    com.android.build.api.variant.impl.LibraryVariantBuilderImpl.
      > Namespace not specified. Please specify a namespace in the
       module's build.gradle file like so:

        android {
            namespace 'com.example.namespace'
        }

        If the package attribute is specified in the source
         AndroidManifest.xml, it can be migrated automatically to the
         namespace value in the build.gradle file using the AGP Upgrade
         Assistant; please refer to https://developer.android.com/
         studio/build/agp-upgrade-assistant for more information.
   > Could not get unknown property 'bundleReleaseAar' for object of
    type org.gradle.api.publish.maven.internal.publication
    .DefaultMavenPublication.

------------------------------------------------------------------------

Explanation: "You must set the namespace in the module-level
'build.gradle.kts' file, rather than the manifest file."

For more info see: https://developer.android.com/build/releases/
gradle-plugin#
Release Notes: https://docs.gradle.org/8.1.1/release-notes.html
Command: ./gradlew wrapper --gradle-version=8.1.1
--distribution-type=all

------------------------------------------------------------------------

As per the previous d85ad7b commit and
its description, this change is a follow-up upgrade, which fully
completes this Gradle upgrade.
FYI: This was suggested automatically using the AGP upgrade assistant.
Release Notes: https://docs.gradle.org/8.2.1/release-notes.html
Command: ./gradlew wrapper --gradle-version=8.2.1
--distribution-type=all
Release Notes: https://docs.gradle.org/8.1.1/release-notes.html
Command: ./gradlew wrapper --gradle-version=8.1.1
--distribution-type=all

------------------------------------------------------------------------

FYI: You will notice that the associated 'gradle-wrapper.jar', 'gradlew'
and 'gradlew.bat' files are not updated. This is because of the breaking
changes that this upgrade introduces, including the 'AGP' upgrade and
its associated breaking changes (namespace, build option, etc). As such,
this Gradle upgrade command is only half done.

PS: When all the breaking changes are resolved, this Gradle command will
be issued again, which should fully complete this upgrade.
This is an AGP version '8.0' breaking change that requires 'namespace'
in module-level build scripts, see build failure below:

------------------------------------------------------------------------

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project
 '@wordpress_react-native-bridge'.
> Failed to notify project evaluation listener.
   > Could not create an instance of type
    com.android.build.api.variant.impl.LibraryVariantBuilderImpl.
      > Namespace not specified. Please specify a namespace in the
       module's build.gradle file like so:

        android {
            namespace 'com.example.namespace'
        }

        If the package attribute is specified in the source
         AndroidManifest.xml, it can be migrated automatically to the
         namespace value in the build.gradle file using the AGP Upgrade
         Assistant; please refer to https://developer.android.com/
         studio/build/agp-upgrade-assistant for more information.
   > Could not get unknown property 'bundleReleaseAar' for object of
    type org.gradle.api.publish.maven.internal.publication
    .DefaultMavenPublication.

------------------------------------------------------------------------

Explanation: "You must set the namespace in the module-level
'build.gradle.kts' file, rather than the manifest file."

For more info see: https://developer.android.com/build/releases/
gradle-plugin#
This is an AGP version '8.0' breaking change that changes the build
option default values, see build failure below:

------------------------------------------------------------------------

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':react-native-bridge'.
> Failed to notify project evaluation listener.
   > com.android.builder.errors.EvalIssueException: defaultConfig
    contains custom BuildConfig fields, but the feature is disabled.
   > Could not get unknown property 'bundleReleaseAar' for object of
    type org.gradle.api.publish.maven.internal.publication
    .DefaultMavenPublication.

------------------------------------------------------------------------

Explanation: "Starting with AGP 8.0, the default values for these flags
have changed to improve build performance. AGP 8.0 doesn't generate
'BuildConfig' by default. You need to specify this option using the DSL
in the projects where you need it."

For more info see: https://developer.android.com/build/releases/
gradle-plugin#default-changes
Release Notes: https://docs.gradle.org/8.1.1/release-notes.html
Command: ./gradlew wrapper --gradle-version=8.1.1
--distribution-type=all

------------------------------------------------------------------------

As per the previous 5ff4e6e commit and
its description, this change is a follow-up upgrade, which fully
completes this Gradle upgrade
FYI: This was suggested automatically using the AGP upgrade assistant.
Release Notes: https://docs.gradle.org/8.2.1/release-notes.html
Command: ./gradlew wrapper --gradle-version=8.2.1
--distribution-type=all
@fluiddot
Copy link
Contributor Author

@fluiddot The changes so far looks good to me. I've left one comment about the settings.gradle changes for react-native-bridge.

For the GitHub action failure related to JDK 17, you can add the following step prior to the task execution so that CI has access to JDK 17:

- name: Set Up JDK 17
  uses: actions/setup-java@v3
  with:
    java-version: 17
    distribution: 'corretto'

You can see an example of this in this PR.

Great, thanks @oguzkocer for sharing the update 🙇 !

We are also updating the publish to S3 Gradle Plugin version to 0.8.0 for all our projects as part of this AGP update. Could you update this & this to reflect that? You'll also need to remove the line with androidSourcesJar from the publish block here & here. Here is an example commit that encapsulates this change.

Hope this helps and let me know if you have any questions/concerns!

Sure thing, I'll apply the updates you shared. Thanks!

<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android">

<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="org.wordpress.mobile.ReactNativeGutenbergBridge">
Copy link
Contributor Author

@fluiddot fluiddot Jul 25, 2023

Choose a reason for hiding this comment

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

The React Native CLI fetches the package attribute from AndroidManifest files, so we can't remove it. Otherwise, we encounter the following error when installing the Pods for the demo project:

error Failed to build the app: No package name found. Found errors in gutenberg-mobile/gutenberg/node_modules/@wordpress/react-native-bridge/android/react-native-bridge/src/main/AndroidManifest.xml.

This can be tested by removing the package attribute and running the command npx react-native config (this command is used behind the scenes when installing the Pods).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is okay (safe) to keep this package definition here @fluiddot , that is, as long as we make sure to have the same namespace defined on the inner react-native-bridge/build.gradle level, just we already did. 👍

PS: We just need to not forget to update one without the other, that is, if we ever need to do something like that anyway (but I doubt that this will be happening anytime soon). 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is okay (safe) to keep this package definition here @fluiddot , that is, as long as we make sure to have the same namespace defined on the inner react-native-bridge/build.gradle level, just we already did. 👍

PS: We just need to not forget to update one without the other, that is, if we ever need to do something like that anyway (but I doubt that this will be happening anytime soon). 🤷

Good point. I think having an inline comment in both build.gradle and AndroidManifest.xml files reminding this would be helpful to avoid forgetting that both need to be updated. I'll add this 💨 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied my above suggestion in 67ca6bd.

Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @fluiddot !

From where I stand, everything LGTM, thank you so much for taking this to the finish line, you are a rockstar! 🙇 ❤️ 🚀

As such, I am personally going to approve this, but I suggest having @oguzkocer taking a final look, getting his approval here too, that is, before we proceed with the JP/WPAndroid testing part via this JP/WPAndroid#18775 client PR.

@fluiddot
Copy link
Contributor Author

From where I stand, everything LGTM, thank you so much for taking this to the finish line, you are a rockstar! 🙇 ❤️ 🚀

As such, I am personally going to approve this, but I suggest having @oguzkocer taking a final look, getting his approval here too, that is, before we proceed with the JP/WPAndroid testing part via this JP/WPAndroid#18775 client PR.

Thanks, you're more than welcome @ParaskP7 😊. All CI jobs are passing, but I encountered issues related to CI jobs and Java 17 in the Gutenberg Mobile PR. I'm actively working on it, at least to solve the ones related to CircleCI. However, I'm a bit blocked with Buildkite so I'm wondering if you could take a look when you are available (reference). Thanks!

@fluiddot fluiddot marked this pull request as ready for review July 25, 2023 14:45
@ParaskP7
Copy link
Contributor

Thanks, you're more than welcome @ParaskP7 😊.

❤️ x ❤️ ^ ❤️

All CI jobs are passing, but I encountered issues related to CI jobs and Java 17 in the wordpress-mobile/gutenberg-mobile#5989. I'm actively working on it, at least to solve the ones related to CircleCI.

Yea, I am aware of that, just took a look... 😞 🤷 😞

However, I'm a bit blocked with Buildkite so I'm wondering if you could take a look when you are available (wordpress-mobile/gutenberg-mobile#5989 (comment)). Thanks!

As per my comment here I think it is best to have @oguzkocer take a quick look when he becomes available later today. In the meanwhile, unfortunately, I'll need to switch context and work on something different for the rest of my day today.

fluiddot added 6 commits July 25, 2023 16:58
This comment will avoid forgetting about updating the package name in all places needed.
…-agp-to-8.0.2

# Conflicts:
#	package-lock.json
#	packages/react-native-aztec/android/gradle/wrapper/gradle-wrapper.jar
#	packages/react-native-aztec/android/gradle/wrapper/gradle-wrapper.properties
#	packages/react-native-aztec/android/gradlew.bat
#	packages/react-native-bridge/android/gradle/wrapper/gradle-wrapper.jar
#	packages/react-native-bridge/android/gradle/wrapper/gradle-wrapper.properties
#	packages/react-native-bridge/android/gradlew.bat
#	packages/react-native-editor/android/gradle/wrapper/gradle-wrapper.jar
#	packages/react-native-editor/android/gradle/wrapper/gradle-wrapper.properties
#	packages/react-native-editor/android/gradlew.bat
#	packages/react-native-editor/android/settings.gradle
#	packages/react-native-editor/package.json
@fluiddot fluiddot added the [Type] Build Tooling Issues or PRs related to build tooling label Jul 28, 2023
@fluiddot fluiddot changed the title [RNMobile] Upgrade Gradle to 8.2.1 and AGP to 8.0.2 [RNMobile] Upgrade Gradle to 8.2.1 and AGP to 8.1.0 Jul 28, 2023
@fluiddot
Copy link
Contributor Author

Heads up that I've updated AGP to version to a newer version (8.1.0) in order to match WP-Android. Additionally, after updating the branch with trunk, I removed a patch we introduced for the RN upgrade 0.71.11 that is no longer necessary due to using a newer version of the React Native Gradle Plugin (665c586).

Copy link
Contributor Author

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I've tested the following items and all succeeded:

  • Publish react-native-aztec library locally ✅
  • Publish react-native-bridge library locally ✅
  • Build and run react-native-editor
  • Build and run WP-Android using this PR and composite build ✅
    NOTE: I had to locally bump AGP and update with trunk when using the WP-Android PR.

The PR is now ready to review and once we have a green light in wordpress-mobile/WordPress-Android#18775, we could proceed to merge it. Thanks!

androidxRecyclerviewVersion = '1.1.0'

// test
junitAztecVersion = '4.13' // TODO: Align 'junit' versions.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ParaskP7 should be this version aligned with WordPress-Android or AztecEditor-Android?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking whether this question was answered in another thread, or is it still outstanding? If it's correct that we should use the WordPress-Android version, should we remove the TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it hasn't been answered yet. I assume aligning the versions is not critical and only a matter of keeping version consistency across libraries. In any case, I'll let @ParaskP7 and @oguzkocer share insights about this. In the meantime, I'll remove the TODO. If we need to follow up on this, we could do it in a GitHub issue.

@@ -34,6 +34,7 @@
"@react-native-community/blur": "4.2.0",
"@react-native-community/slider": "https://raw.githubusercontent.com/wordpress-mobile/react-native-slider/v3.0.2-wp-4/react-native-community-slider-3.0.2-wp-4.tgz",
"@react-native-masked-view/masked-view": "0.2.6",
"@react-native/gradle-plugin": "0.72.11",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally, we tried patching the React Native Gradle plugin, as discussed here. But this turned out to be complex and hard to maintain. For this reason, the incompatibility with the Gradle plugin has been solved by adding a specific and newer version. This seems safe even though we use an older React Native version.

NOTE: The React Native Gradle Plugin is a transitive dependency of React Native.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer this patch as WP-Android will use an Android Gradle Plugin version above the one used by the React Native Gradle Plugin (8.2.1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems this file got automatically reformatted when updating with trunk and committing 🤔. I checked the format of other workflow files (example) and now it's matching it, so looks like a safe change to apply.

cc @priethor as author of #52760

Copy link
Contributor

@priethor priethor Jul 28, 2023

Choose a reason for hiding this comment

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

Aha, the linter didn't throw any errors, but it's true the spacing wasn't matching the other yaml files. Thanks for catching this, and feel free to push the changes! 👍

@SiobhyB SiobhyB self-requested a review July 28, 2023 09:45
Copy link
Contributor

@SiobhyB SiobhyB left a comment

Choose a reason for hiding this comment

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

@fluiddot, thank you for all the work on this! I confirmed that the demo app builds as expected for me and, looking through the changes, I only had one question that is likely not blocking. Based on that, as well as the questions/review from Petros and Oguz, going ahead to approve. 👏 👏 🎉

androidxRecyclerviewVersion = '1.1.0'

// test
junitAztecVersion = '4.13' // TODO: Align 'junit' versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking whether this question was answered in another thread, or is it still outstanding? If it's correct that we should use the WordPress-Android version, should we remove the TODO?

@fluiddot fluiddot enabled auto-merge (squash) July 28, 2023 17:41
@fluiddot fluiddot merged commit 6e4bc84 into trunk Jul 28, 2023
@fluiddot fluiddot deleted the rnmobile/0.69.4-upgrade-gradle-to-8.2.1-and-agp-to-8.0.2 branch July 28, 2023 18:13
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - Automation Label used to initiate Mobile App PR Automation Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Gradle 8.2.1 & AGP 8.0.2
5 participants