-
Notifications
You must be signed in to change notification settings - Fork 3
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
Upgrade to gradle 7.1.1 #12
Conversation
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.
@@ -0,0 +1,2 @@ | |||
install: | |||
- cd android-exoplayer && ./gradlew publishToMavenLocal |
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.
Does this publish a version of exoplayer to our Maven repo? And am I correct to assume we do this instead of using the upstream one because of the Gradle 7 tweaks done in this PR?
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.
No, it publishes to Jitpack as this is the jitpack configuration file. The way it works is they publish it to mavenLocal
which is the ~/.m2
directory and then update a few things and serve it to the public. It's a nice hack, but a hack nevertheless. You can run rm -rf ~/.m2 && ./gradlew publishToMavenLocal && exa ~/.m2 --tree
to see what publishing this artifact does. (If you don't have exa, this will be a good chance to install it 😄 )
The reason we publish android-exoplayer
instead of android
is that's the one that's used in mobile-gutenberg, but they want to keep the old one.
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=all
command. There were a few cases wheregradlew
file was missing or wasn't executable, and these were also addressed as part of this change.settings.gradle.kts
file 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-bridge
as a project dependency and now thatreact-native-bridge
itself 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.android
extension by removing the fancyminSdk
,targetSdk
etc 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
&dependencies
sections 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-publish
plugin and sets up thepublishing
block. This allows Jitpack to publish the artifacts and with this update we are actually only one step away from publishing them to S3 🤷jitpack.yml
where 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.json
file 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.json
file again.