Bump AGP to `8.5.1` #12042

wzieba · 2024-07-17T12:34:07Z

Description

This PR updates Android Gradle Plugin to 8.5.0, mostly needed to address security vulnerabilities.

Testing information

Install the app in releaseVanilla variant as there are changes related to R8.
Smoke test the app, focus on testing paths from every module, including iap and cardreader

Dependencies

Bump AGP to 8.5.1 wordpress-mobile/WordPress-FluxC-Android#3065
Bump AGP to 8.5.1 wordpress-mobile/WordPress-Android#21082
Bump AGP to 8.5.1 WordPress/gutenberg#64044
I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Starting from Gradle 8.4, shrinking at library level is now respected. Library modules will be now shrinked in isolation, removing code that might be used by other modules. The solution is to not shrink each library module (no need as we don't publish them in isolation) and shrink only app modules.

They're not used

wpmobilebot · 2024-07-17T12:45:44Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`1e9c0ff`
	Direct Download	`woocommerce-wear-prototype-build-pr12042-1e9c0ff.apk`

wpmobilebot · 2024-07-17T12:57:23Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`1e9c0ff`
	Direct Download	`woocommerce-prototype-build-pr12042-1e9c0ff.apk`

To address `NotificationPermission` lint error. Actually, this permission is not needed at this moment, as the wear module doesn't use org.wordpress.android.util.AutoForeground or classes that overrides from it so it seems to be a false positive from lint. I don't want to suppress this issue though, as we might use AutoForeground in the future, and the POST_NOTIFICATIONS will be necessary then.

- unwrap `String#format(Context#getString` calls. Having two formats is unnecessary and can be confusing - update format arguments to reflect type declared in strings.xml

wzieba · 2024-07-23T11:39:06Z

WooCommerce-Wear/src/main/AndroidManifest.xml


    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.WAKE_LOCK" />
+    <uses-permission android:name="android.permission.POST_NOTIFICATIONS" />


More context on this:

I think it's a false positive: the WooCommerce-Wear doesn't use AutoForeground or any service that is inheriting from it.

I've experimented with removing org.wordpress:utils dependency from WooCommerce-Wear module and running lint was resulting with no errors.

To make sure there are no new services, I've compared manifests of WooCommerce-Wear with and without org.wordpress:utils. The diff is:

which is not relevant to POST_NOTIFICATIONS.

My conclusion: Android linter detects AutoForeground service in dependencies of the WooCommerce-Wear module and throws this lint error, not minding that we don't actually use the service.

I decided to add this permission anyway, as it's already present in WooCommerce module and I didn't want to suppress this warning, in case that we'll indeed need posting a notification from the wear app.

~~Only now I realised that we already request POST_NOTIFICATIONS in a few places in the app~~

woocommerce-android/WooCommerce/src/main/kotlin/com/woocommerce/android/util/WooPermissionUtils.kt

Lines 59 to 62 in e7e9368

@RequiresApi(VERSION_CODES.TIRAMISU)

fun requestNotificationsPermission(launcher: ActivityResultLauncher<String>) {

launcher.launch(POST_NOTIFICATIONS)

}

~~so adding this POST_NOTIFICATION in manifest is more of a correctness (as signilised by lint warning)~~

edit: yes, we use but in WooCommerce module, not WooCommerce-Wear

codecov-commenter · 2024-07-23T12:06:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.04%. Comparing base (e7e9368) to head (13eba9a).
Report is 612 commits behind head on trunk.

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #12042      +/-   ##
============================================
+ Coverage     39.99%   40.04%   +0.05%     
+ Complexity     5525     5505      -20     
============================================
  Files          1200     1200              
  Lines         68416    68295     -121     
  Branches       9577     9369     -208     
============================================
- Hits          27364    27350      -14     
+ Misses        38492    38445      -47     
+ Partials       2560     2500      -60

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wzieba · 2024-07-23T13:01:26Z

hi @kidinov 👋 can I request your assistance in smoke testing iap and cardreader modules?

ParaskP7

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟

I have left a single suggestion (💡) comment for you to consider. I am NOT going to approve this PR (for now), leaving the final approval to @kidinov instead (context). No matter, everything worked as expected for me so I can tentatively approve as well if you need me to (just let me know).

EXTRAS

Warning (⚠️): When this PR get merged into trunk dev will need to use AS Koala. AGP 8.5.0 is incompatible with previous versions of AS and thus devs wouldn't be able to (for example) sync the project otherwise (see Android Studio & AGP compatibility options).

settings.gradle

wzieba · 2024-07-25T09:02:21Z

hi @malinajirka ! 👋 can I have your assistance regarding smoke testing iap and cardreader modules? The changes introduced with AGP 8.4.0 are related to obfuscating library modules, so I'd like to make sure we don't introduce regression.

I don't have a store with the correct setup, so I think it'll be more efficient if someone from Woo could test it.

malinajirka

Hi @wzieba 👋

👋 can I have your assistance regarding smoke testing iap and cardreader modules? The changes introduced with AGP 8.4.0 are related to obfuscating library modules, so I'd like to make sure we don't introduce regression.

I've tested the card reader flows and I haven't noticed any regression.

With the removal of Site Creation in the app, the IAP flows are not used at the moment.

wzieba · 2024-07-25T09:51:44Z

Gotcha, thank you @malinajirka ! 🙌

wpmobilebot · 2024-08-06T14:54:26Z

Found 1 violations:

The PR caused the following dependency changes:

expand

-+--- androidx.databinding:viewbinding:8.1.0
++--- androidx.databinding:viewbinding:8.5.1
 +--- project :libs:cardreader
 |    \--- com.stripe:stripeterminal-localmobile:3.1.1
-|         \--- androidx.databinding:viewbinding:7.4.2 -> 8.1.0 (*)
+|         \--- androidx.databinding:viewbinding:7.4.2 -> 8.5.1 (*)
 \--- org.wordpress:mediapicker:0.3.1
-     \--- androidx.databinding:viewbinding:8.1.0 (*)
+     \--- androidx.databinding:viewbinding:8.1.0 -> 8.5.1 (*)

Please review and act accordingly

wzieba added 3 commits July 17, 2024 14:33

Bump AGP to 8.5.0

fb7dde0

Remove release build type from library modules

ac213cf

They're not used

wzieba added 2 commits July 23, 2024 11:29

Address StringFormatMatches lint warning

0bcd1e7

- unwrap `String#format(Context#getString` calls. Having two formats is unnecessary and can be confusing - update format arguments to reflect type declared in strings.xml

wzieba commented Jul 23, 2024

View reviewed changes

wzieba added status: do not merge Dependent on another PR, ready for review but not ready for merge. category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. labels Jul 23, 2024

wzieba added this to the 19.7 milestone Jul 23, 2024

wzieba marked this pull request as ready for review July 23, 2024 13:01

wzieba requested review from ParaskP7 and kidinov July 23, 2024 13:01

ParaskP7 reviewed Jul 24, 2024

View reviewed changes

settings.gradle Outdated Show resolved Hide resolved

wzieba modified the milestones: 19.7, 19.8 Jul 24, 2024

Bump AGP to 8.5.1

8d45720

wzieba changed the title ~~Bump AGP to 8.5.0~~ Bump AGP to 8.5.1 Jul 24, 2024

Merge branch 'trunk' into agp_8_5_0

13eba9a

kidinov removed their request for review July 25, 2024 08:24

wzieba requested a review from malinajirka July 25, 2024 09:02

malinajirka approved these changes Jul 25, 2024

View reviewed changes

wzieba mentioned this pull request Jul 26, 2024

Bump AGP to 8.5.1 wordpress-mobile/WordPress-Android#21082

Merged

wzieba changed the title ~~Bump AGP to 8.5.1~~ Bump AGP to 8.5.1 Jul 26, 2024

wzieba mentioned this pull request Jul 26, 2024

Bump AGP to 8.5.1 wordpress-mobile/WordPress-FluxC-Android#3065

Merged

wzieba modified the milestones: 19.8, 19.9 Jul 30, 2024

wzieba removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Aug 6, 2024

Merge branch 'trunk' into agp_8_5_0

1e9c0ff

wzieba enabled auto-merge August 6, 2024 14:52

wzieba merged commit ff2261b into trunk Aug 6, 2024

wzieba deleted the agp_8_5_0 branch August 6, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bump AGP to `8.5.1` #12042

Bump AGP to `8.5.1` #12042

Uh oh!

wzieba commented Jul 17, 2024 •

edited

Loading

Uh oh!

wpmobilebot commented Jul 17, 2024 •

edited

Loading

Uh oh!

wpmobilebot commented Jul 17, 2024 •

edited

Loading

Uh oh!

wzieba Jul 23, 2024

Uh oh!

wzieba Jul 24, 2024 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 23, 2024 •

edited

Loading

Uh oh!

wzieba commented Jul 23, 2024

Uh oh!

ParaskP7 left a comment

Uh oh!

Uh oh!

wzieba commented Jul 25, 2024

Uh oh!

malinajirka left a comment

Uh oh!

wzieba commented Jul 25, 2024

Uh oh!

wpmobilebot commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	@RequiresApi(VERSION_CODES.TIRAMISU)
	fun requestNotificationsPermission(launcher: ActivityResultLauncher<String>) {
	launcher.launch(POST_NOTIFICATIONS)
	}

Bump AGP to 8.5.1 #12042

Bump AGP to 8.5.1 #12042

Uh oh!

Conversation

wzieba commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing information

Dependencies

Uh oh!

wpmobilebot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wzieba Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

wzieba Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wzieba commented Jul 23, 2024

Uh oh!

ParaskP7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wzieba commented Jul 25, 2024

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

wzieba commented Jul 25, 2024

Uh oh!

wpmobilebot commented Aug 6, 2024

The PR caused the following dependency changes:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Bump AGP to `8.5.1` #12042

Bump AGP to `8.5.1` #12042

wzieba commented Jul 17, 2024 •

edited

Loading

wpmobilebot commented Jul 17, 2024 •

edited

Loading

wpmobilebot commented Jul 17, 2024 •

edited

Loading

wzieba Jul 24, 2024 •

edited

Loading

codecov-commenter commented Jul 23, 2024 •

edited

Loading