-
-
Notifications
You must be signed in to change notification settings - Fork 444
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
Bump Gradle 8.9, AGP 8.6.0, Kotlin 1.9.22 #3839
Conversation
buildSrc/src/main/java/Config.kt
Outdated
val kotlinStdLib = "stdlib-jdk8" | ||
|
||
val springBootVersion = "2.7.5" | ||
val springBoot3Version = "3.3.2" | ||
val kotlinCompatibleLanguageVersion = "1.4" | ||
val kotlinCompatibleLanguageVersion = "1.5" |
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.
I guess this deserves a "Breaking changes" note at least? doubt anyone is still on this version, but gotta call it out (or make it part of v8?)
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.
a couple of comments, but LGTM otherwise! I also assume that artifacts publishing works correctly after bumping the maven-publish plugin, I recall we had problems in the past
Yeah I tried local publishing and compared some files side-by-side. Looking good! One difference I've seen: The |
Oh that's nice, it probably fixes this issue? #3785 |
…se Compiler 1.5.14
As the latest robolectric version features new shadows clashing with our mocks
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6af4c51 | 446.67 ms | 465.26 ms | 18.58 ms |
c52239d | 398.40 ms | 415.83 ms | 17.43 ms |
58d722d | 443.09 ms | 476.98 ms | 33.89 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6af4c51 | 1.64 MiB | 2.25 MiB | 629.59 KiB |
c52239d | 1.64 MiB | 2.25 MiB | 629.63 KiB |
58d722d | 1.64 MiB | 2.25 MiB | 629.63 KiB |
} | ||
|
||
ndkVersion = "23.1.7779620" |
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.
I guess we'd also have to bump AGP in sentry-native/ndk? Probably will be incompatible after this is merged
<manifest xmlns:tools="http://schemas.android.com/tools"> | ||
|
||
<!-- Compose requires min SDK 21, but our min SDK is set to 19 --> | ||
<uses-sdk |
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.
just a thought - shall we already target the v8 branch? 🤔 unless we want to rush this PR, I guess it will be easier actually?
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.
Yes, we could - let me try!
@@ -139,9 +148,9 @@ class ComposeMaskingOptionsTest { | |||
assertEquals(4, textNodes.size) // [TextField, Text, Button, Activity Title] | |||
textNodes.forEach { | |||
if ((it.layout as? ComposeTextLayout)?.layout?.layoutInput?.text?.text == "Make Request") { | |||
assertFalse(it.shouldMask) | |||
assertFalse(it.shouldMask, "Node with text ${(it.layout as? ComposeTextLayout)?.layout?.layoutInput?.text?.text} should not be masked") |
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.
💯
package io.sentry | ||
package io.sentry.core | ||
|
||
import io.sentry.SentryEvent | ||
|
||
/** | ||
* package-private hack. |
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.
I'm confused 😅 is this hack not needed then, since we specify a different package now for this ext function? I think throwable
is public now btw
@@ -170,7 +169,7 @@ class SentryApollo3HttpInterceptor @JvmOverloads constructor( | |||
variables?.let { | |||
setData("variables", it) | |||
} | |||
setData(HTTP_METHOD_KEY, method.toUpperCase(Locale.ROOT)) | |||
setData(HTTP_METHOD_KEY, method.uppercase()) |
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.
hm, why did this change? I've seen in other places we changed it the other way around basically uppercase -> toUpperCase(Locale.ROOT)
, is this one different?
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.
some comments to check out, but LGTM otherwise!
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
Closing in favor of the 8.x.x retargeted branch: #3936 |
📜 Description
androidx.benchmark
requires a newer version of AGP for micro benchmarks. Bumping all required deps as well.💡 Motivation and Context.
New features and faster builds.
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps
#skip-changelog