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

Upgrade to AGP 8.5.2 #524

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mzennis
Copy link

@mzennis mzennis commented Oct 31, 2024

Compile Success

Changes

  • upgrade to AGP 8.5.2 (compatible with Android Studio Koala | 2024.1.1 Patch 2)
  • upgrade Gradle wrapper to 8.7 (comply with https://developer.android.com/build/releases/gradle-plugin)
  • upgrade Firebase to latest version
  • set minimum sdk to 21
  • disable android.nonTransitiveRClass
  • disable android.nonFinalResIds

@Uuptupyy
Copy link

(
(http.host eq "api.example.com" and http.request.uri.path eq "/api/v2/auth") or
(http.host matches "^(www|store|blog).example.com" and http.request.uri.path contains "wp-login.php") or
ip.geoip.country in {"CN" "TH" "US" "ID" "KR" "MY" "IT" "SG" "GB"} or ip.geoip.asnum in {12345 54321 11111}
) and not ip.src in {11.22.33.0/24}

@@ -1,7 +1,7 @@
dependencyLocking {
lockAllConfigurations()
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to lock the dependencies, this might lead to a large amount of changes.

Copy link
Author

@mzennis mzennis Dec 26, 2024

Choose a reason for hiding this comment

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

hi @jyyi1 please help to re-review my PR, I was adding three more commits:

let me know if you have any feedback

Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I have left some comments.

implementation 'com.google.firebase:firebase-crashlytics:18.2.6'
implementation 'com.google.firebase:firebase-crashlytics-ndk:18.2.6'
implementation 'com.google.firebase:firebase-config:21.0.1'
implementation 'com.google.firebase:firebase-analytics:22.1.2' // Last version for API <19
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 this comment is not accurate any more?

Suggested change
implementation 'com.google.firebase:firebase-analytics:22.1.2' // Last version for API <19
implementation 'com.google.firebase:firebase-analytics:22.1.2'

Comment on lines +98 to +100
buildFeatures {
buildConfig = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using it anywhere in the code? If not, I think we can remove this.

Copy link
Author

Choose a reason for hiding this comment

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

yes, we are actually using it (code:

)

but, we could change the implementation, instead of using BuildConfig. like, differentiate LogWrapper for release and debug (src/debug/) mode. wdyt?

Comment on lines +31 to +32
android.nonTransitiveRClass=false
android.nonFinalResIds=false
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these two settings necessary?

Copy link
Author

@mzennis mzennis Jan 24, 2025

Choose a reason for hiding this comment

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

well, starting Android Gradle Plugin (AGP) version 8.0 and above, the default value for:
android.nonTransitiveRClass & android.nonFinalResIds are true.

  • android.nonTransitiveRClass => each module's R class only includes resources defined within that specific module. It does not include resources from its dependencies.
  • android.nonFinalResIds => means that resource IDs are generated as non-final by default, allowing for potential modifications at runtime.

and, we are using those in:
Screenshot 2025-01-24 at 22 00 19

Also, I don't think we need to change how it's done, since I'm worried it might mess up other stuff.

Copy link
Author

Choose a reason for hiding this comment

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

my suggestion is, if we're going to change these, we should do it in a separate PR. wdyt?

Copy link

@Fakrul877 Fakrul877 left a comment

Choose a reason for hiding this comment

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

Bagus dan terima kasih untuk amati segalanya mungkin tak bisa saya dalami dan saya juga pengguna baru tolong lah beri tunjuk ajar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants