Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) #18

ParaskP7 · 2022-02-15T15:21:47Z

This PR is part of Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier) PR. There are 13 libraries that are upgraded to Gradle 7.4 & AGP 7.1.1 (& Disable Jetifier).

All those PRs follow the general outline below:

Gradle version upgraded to 7.3.3 with the ./gradlew wrapper --gradle-version=7.3.3 --distribution-type=all command.
AGP version upgrade to 7.0.4 (see android/settings.gradle.kts change).
Build output diff:
- To identify and fix new warnings/errors that got added.
- To identify and verify old warnings/errors that got removed.
Lint output diff
- To identify and fix new warnings/errors that got added.
- To identify and verify old warnings/errors that got removed.
The above 4 steps were redone with a follow-up update to Gradle 7.4 & AGP 7.1.1.
The above 5 steps were done for both, the android-exoplayer and android projects.
Jetifier drop (see android/gradle.properties change).
Version bump to 5.2.0-wp-3 (see package.json).
Tarball update to 5.2.0-wp-3 (see react-native-video-5.2.0-wp-3.tgz).
The build was also verified through Jitpack (see build.log).

This PR also included the following changes:

Set Jitpack JDK to openjdk11 (see jitpack.yml change).
Guard com.squareup.okhttp3:okhttp dependency declaration (see android-exoplayer/build.gradle change).

To test - Now

These changes can be tested as part of the gutenberg PR which is updated to use the temporarily non-tagged node module project dependencies generated with these changes.

To test - Later ⚙️

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.

FYI: It's best to leave the testing step to the gutenberg PR review since even if these PRs are merged, they won't impact anything until the package.json file is updated. Worst case scenario, a follow up PR will get opened to fix any issues that are found during testing.

Follow up

Once these PRs are merged in, a new tag will get created for each library and the package.json file will be again updated accordingly.

The 'com.dipien.byebyejetifier' plugin was used to determine whether Jetifier can be disable for this project. After configuring 'Bye Bye Jetifier' and running the below command: ./gradlew canISayByeByeJetifier -Pandroid.enableJetifier=false The output was clear, Jetifier can be now safely disabled for this project: > Task :canISayByeByeJetifier ========================================= Project: : ========================================= * No legacy android support usages found ===================================================== ... * No dependencies with legacy android support usages! ... ===================================================== ... ... =============================== ... You can say Bye Bye Jetifier. * ... ===============================

Lint error message: "It looks like you are trying to substitute a version variable, but using single quotes ('). For Groovy string interpolation you must use double quotes ("). To insert the value of a variable, you can use ${variable} inside a string literal, but only if you are using double quotes!"

ParaskP7 · 2022-02-25T11:06:00Z

👋 @mchowning !

Can you verify that this change I did here, which removes this incorrect OkHttp dependency declaration, is actually correct? 🙇

PS: I did that because otherwise Lint errors for both, this repo and the react-native-bridge module within the gutenberg repo.

mchowning · 2022-02-25T15:54:12Z

android-exoplayer/build.gradle

@@ -55,7 +55,6 @@ dependencies {
    implementation('com.google.android.exoplayer:extension-okhttp:2.13.3') {
        exclude group: 'com.squareup.okhttp3', module: 'okhttp'
    }
-    implementation 'com.squareup.okhttp3:okhttp:${OKHTTP_VERSION}'


Would the better fix here be to just change this to double-quotes? We could also do a PR to push this fix upstream, we're not the first to notice it: TheWidlarzGroup@3dc607c.

Just for reference: here is the PR adding this line to react-native-video: TheWidlarzGroup#2340

👋 @mchowning !

Thanks for the response! 🥇

Would the better fix here be to just change this to double-quotes?

I actually tried that but it doesn't work as the OKHTTP_VERSION is not available, it only resides within the RN note module. Thus, the build failed when I tried.

My question is if we actually need this dependency, since we are able to build even without it. 🤔

We could also do a PR to push this fix upstream, we're not the first to notice it: TheWidlarzGroup@3dc607c.

We can try doing that, yes. 👍

Just for reference: here is the PR adding this line to react-native-video: TheWidlarzGroup#2340

👍

I actually tried that but it doesn't work as the OKHTTP_VERSION is not available, it only resides within the RN note module. Thus, the build failed when I tried.

Ah good point. We could always wrap this in a project.hasProperty(...), but keeping it simple here by just removing it also seems fine since we obviously haven't needed it.

If you want to go with the "removal" approach you have here, I'm curious if you have any thoughts about possibly commenting out this line and adding a comment describing why it is being commented out. We generally try to keep these forks as close to the source as possible, and I'm thinking it might be hard for someone to backtrack and figure out why we removed this line when the inevitable conflict on this line arises in the future. I don't feel strongly, I'm more just thinking out loud, so let me know what you think.

It builds without this dependency because okhttp is a transitive dependency of com.facebook.fresco:imagepipeline. If this project is using okhttp directly, we should keep it and use the version we want. If the project is not using it directly, it should be fine to remove it as whichever dependency needs it should also have it in its pom file.

👋 @mchowning @oguzkocer !

Thank you so much for your input! 🙏

Ah good point. We could always wrap this in a project.hasProperty(...), but keeping it simple here by just removing it also seems fine since we obviously haven't needed it.

This is a good idea. 👍

If you want to go with the "removal" approach you have here, I'm curious if you have any thoughts about possibly commenting out this line and adding a comment describing why it is being commented out. We generally try to keep these forks as close to the source as possible, and I'm thinking it might be hard for someone to backtrack and figure out why we removed this line when the inevitable conflict on this line arises in the future. I don't feel strongly, I'm more just thinking out loud, so let me know what you think.

Yes, this is a good argument, on keeping the forks as close to the source as possible, I agree. 👍

It builds without this dependency because okhttp is a transitive dependency of com.facebook.fresco:imagepipeline.

Yes, actually after running ./gradlew dependencies and checking the before and after result, com.squareup.okhttp3:okhttp-urlconnection is also using okhttp as a transitive dependency, as the whole androidx.appcompat:appcompat dependency itself.

I also did a quick analysis, using ./gradlew dependencies, a before and after diff for the removal of the explicit 'com.squareup.okhttp3:okhttp:${OKHTTP_VERSION}' dependency and the only diff I found are the below line, which is found twice on the top level and the com.facebook.react:react-native:0.66.2 tree:

+--- com.squareup.okhttp3:okhttp:${OKHTTP_VERSION} -> 4.9.1 (*)

This makes me think that:

The ${OKHTTP_VERSION} is being hardcoded and doesn't change to the OKHTTP_VERSION=3.12.12 as defined by the gradle.properties of the ReactAndroid project. Thus, it seems to me that Gradle just ignores while building and then Lint produces the Incorrect Interpolation error.

With or without this incorrectly defined dependency, due to the Incorrect Interpolation error, the end result of building the project is the same. However, this is solely based on building and linting this project individually. If, for some reason, unknown to me due to my lack of React Native experience, when this project is build and used in combination with the ReactAndroid project, it might actually do some code-generation magic to substitute this OKHTTP_VERSION version with the actual version to produce the final result. Thus, then I would no longer feel comfortable removing this dependency.

If this project is using okhttp directly, we should keep it and use the version we want. If the project is not using it directly, it should be fine to remove it as whichever dependency needs it should also have it in its pom file.

Good thinking. 🥇

From what I am seeing this project is using a couple of okhttp3 imports within the DataSourceUtil, which makes me think, just to be on the safe side, since I am not having much React Native experience, to keep this dependency and suppress this Incorrect Interpolation Lint error to avoid future such discussions and have Lint success no matter. Wdyt? 🤔

Commit here: 4fa6833

From what I am seeing this project is using a couple of okhttp3 imports within the DataSourceUtil, which makes me think, just to be on the safe side, since I am not having much React Native experience, to keep this dependency and suppress this Incorrect Interpolation Lint error to avoid future such discussions and have Lint success no matter. Wdyt?

That doesn't quite feel right to me since we're suppressing a lint warning that is highlighting the fact that this line of code doesn't actually do anything. I don't feel super-strongly, but in light of @oguzkocer 's comment noting that the build only succeeds because okhttp is a transitive dependency (thanks for pointing that out btw), I'd lean toward fixing this with the project.hasProperty(...) wrapper and then switching to using double quotes. That way we can set that variable and actually use it if we want to, and it will be more informative to any developer that bumps into this issue in the future.

👋 @mchowning !

...I'd lean toward fixing this with the project.hasProperty(...) wrapper and then switching to using double quotes.

Yes, I think this is better. The only reason I didn't proceed with this change is because you mentioned that we generally try to keep these forks as close to the source as possible and I wasn't sure If I should proceed with this change.

Please expect me applying this change today. 👍

This is now done: b1bff8b

For more info about that read full discussion here: https://github.com/ /pull/18#discussion_r814883725

ParaskP7 · 2022-04-06T09:32:42Z

Since we removed the source code for these projects from react-native-bridge, we don’t need these updates to happen together with the other projects anymore. That means we can close this draft PR for now and reopen it, if need be, in the future. There is little value in merging this now and diverging from the source repo.

PS: Eventually, we might upgrade Gradle & AGP when using a newer version of React Native if the new RN version requires it.

ParaskP7 added 4 commits February 15, 2022 17:08

Build: Update gradle to 7.3.3 for android exoplayer project

7961edc

Build: Update agp to 7.0.4 for android exoplayer project

602c830

Build: Update gradle to 7.3.3 for android project

eae00e7

Build: Update agp to 7.0.4 for android project

dda5684

ParaskP7 added the enhancement New feature or request label Feb 15, 2022

ParaskP7 requested a review from oguzkocer February 15, 2022 15:21

ParaskP7 self-assigned this Feb 15, 2022

oguzkocer approved these changes Feb 16, 2022

View reviewed changes

ParaskP7 added 8 commits February 23, 2022 15:34

Build: Update gradle to 7.4 for android exoplayer project

018921e

Build: Update agp to 7.1.1 for android exoplayer project

4a490a9

Build: Update gradle to 7.4 for android project

1546e6a

Build: Update agp to 7.1.1 for android project

e6eef39

Build: Bump version to 5.2.0-wp-3

e8f82d0

Build: Generate new tarball for 5.2.0-wp-3

669b5ec

ParaskP7 changed the title ~~Upgrade Gradle to 7.3.3 & AGP to 7.0.4~~ Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Drop Jetifier) Feb 23, 2022

ParaskP7 changed the title ~~Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Drop Jetifier)~~ Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier) Feb 23, 2022

ParaskP7 added 2 commits February 24, 2022 18:27

Build: Generate new tarball for 5.2.0-wp-3

bfb0880

ParaskP7 changed the title ~~Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier)~~ Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) Feb 24, 2022

Set jitpack jdk to openjdk11

9a599f2

mchowning reviewed Feb 25, 2022

View reviewed changes

oguzkocer self-requested a review February 25, 2022 16:37

ParaskP7 added 2 commits February 28, 2022 11:39

Build: Add back incorrect and as such unused okhttp dep declaration

4fa6833

For more info about that read full discussion here: https://github.com/ /pull/18#discussion_r814883725

Build: Guard incorrect and as such unused http dependency declaration

b1bff8b

ParaskP7 mentioned this pull request Mar 8, 2022

Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier) WordPress/gutenberg#39097

Closed

24 tasks

ParaskP7 closed this Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) #18

Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) #18

ParaskP7 commented Feb 15, 2022 •

edited

Loading

ParaskP7 commented Feb 25, 2022

mchowning Feb 25, 2022

ParaskP7 Feb 25, 2022

mchowning Feb 25, 2022

oguzkocer Feb 25, 2022

ParaskP7 Feb 28, 2022 •

edited

Loading

mchowning Mar 2, 2022

ParaskP7 Mar 3, 2022

ParaskP7 Mar 3, 2022

ParaskP7 commented Apr 6, 2022

Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) #18

Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) #18

Conversation

ParaskP7 commented Feb 15, 2022 • edited Loading

ParaskP7 commented Feb 25, 2022

mchowning Feb 25, 2022

Choose a reason for hiding this comment

ParaskP7 Feb 25, 2022

Choose a reason for hiding this comment

mchowning Feb 25, 2022

Choose a reason for hiding this comment

oguzkocer Feb 25, 2022

Choose a reason for hiding this comment

ParaskP7 Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

mchowning Mar 2, 2022

Choose a reason for hiding this comment

ParaskP7 Mar 3, 2022

Choose a reason for hiding this comment

ParaskP7 Mar 3, 2022

Choose a reason for hiding this comment

ParaskP7 commented Apr 6, 2022

ParaskP7 commented Feb 15, 2022 •

edited

Loading

ParaskP7 Feb 28, 2022 •

edited

Loading