-
Notifications
You must be signed in to change notification settings - Fork 0
Upgrade to gradle 7.1.1 #5
Conversation
…dependency properly
mokagio
left a comment
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.
Same considerations as my first review in this Upgrade to Gradle 7.1.1 series of PR. 👍
- CI in the Gutenberg PR that introduces these changes is green.
- I verified the Gutenberg PR uses the latest commit in this branch.
- CI in the gutenberg-mobile PR is red, but it's due to an npm issue (
cd() never called!) which I don't think relates to these changes.
| versionCode 1 | ||
| versionName "1.0" | ||
| minSdkVersion 21 | ||
| targetSdkVersion 30 |
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.
Out of curiosity, is there a way to inherit these settings from android/build.gradle?
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 could add a property to the extension, but I think this one needs a little bit of a project structure re-work. I don't think that change would be high impact, so I am leaving it as is.
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.
Sure. Thanks for the explanation 😄
This PR is part of Gradle 7 upgrade for react-native libraries for Gutenberg. There are 13 libraries that are upgraded to Gradle 7 and they follow the general outline below:
./gradlew wrapper --gradle-version=7.1.1 --distribution-type=allcommand. There were a few cases wheregradlewfile was missing or wasn't executable, and these were also addressed as part of this change.settings.gradle.ktsfile by utilizing Plugin DSL. Since only one settings file is executed per build, except for composite builds, this allows us to change the plugin versions from the root project. For example, all these projects are added toreact-native-bridgeas a project dependency and now thatreact-native-bridgeitself uses Plugin DSL, it can set the Android Gradle plugin & Kotlin plugin version without making changes to these libraries. This should make our lives easier when it comes to upgrading to new versions of Android Gradle plugin.androidextension by removing the fancyminSdk,targetSdketc setting. If these need to be updated I think they should be done directly as a PR and the fancy code is just making things harder to follow. It's also perfectly fine for a library to be on a lower version if it works fine with it. I think this setting is much more important for applications.repositories&dependenciessections of the build file. I am still not super happy with this one, but it'll need to be improved again at a later time, possibly with the new Gradle 7 dependency management features. Most of them are exactly the same, with a few exceptions where it was necessary.gradle.properties. I went for a simple version which should work pretty much all the time.maven-publishplugin and sets up thepublishingblock. This allows Jitpack to publish the artifacts and with this update we are actually only one step away from publishing them to S3 🤷jitpack.ymlwhere it's not necessary, or simplifies it greatly by only including what's necessary - such as switching to a different directory because Jitpack can't find the project.There might be some minor changes for a few projects, but hopefully they'll be self-explanatory. Also the first few PRs I opened may be a little different, but it seems to work and I think we can live with that inconsistency. There is just too many PRs to keep everything straight in my head :(
To test
These changes can be tested as part of the gutenberg-mobile PR or WordPress-Android PR which is updated to use the bundle generated with these changes. Having said that, I think it's best to leave the testing step to the gutenberg PR review since even if we merge these PRs in, it won't impact anything until we update the
package.jsonfile that adds these libraries. So, worst case scenario, we just open a follow up PR to fix any issues we find during testing. It's up to the reviewer to decide how to go about testing.Follow up
Once these PRs are merged in, we'll create a new tag for each library and update the
package.jsonfile again.