refactor(android): Convert sentry.gradle to Kotlin DSL (sentry.gradle.kts)#6119
refactor(android): Convert sentry.gradle to Kotlin DSL (sentry.gradle.kts)#6119
Conversation
….kts) Convert the Groovy build script to Kotlin DSL for type safety and better IDE support. Remove legacy RN < 0.71 code paths. Auto-migrate old sentry.gradle references in Expo plugin. Closes #2327 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
|
|
@cursor review |
|
@sentry review |
Keep a thin sentry.gradle that forwards to sentry.gradle.kts so existing wizard and manually configured projects continue to work without changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| "bundle${variantCapitalized}JsAndAssets" | ||
| ).find { tasks.names.contains(it) } |
There was a problem hiding this comment.
Bug: Task names are constructed with invalid characters (@, +), which will cause the Gradle build to fail during configuration.
Severity: CRITICAL
Suggested Fix
Sanitize the releaseName variable before using it in a task name to replace or remove any characters that are not in the allowed set of [a-zA-Z0-9_.-].
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/sentry.gradle.kts#L302-L303
Potential issue: The script constructs Gradle task names using a `releaseName` variable
that contains characters like `@` and `+`. Gradle task names are restricted to
`[a-zA-Z0-9_.-]`. The inclusion of these invalid characters will cause an
`InvalidUserDataException` during the configuration phase, preventing the build from
starting for any project that uses this script.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| val bundleOutput = result.bundleOutput | ||
| val sourcemapOutput = result.sourcemapOutput | ||
| val packagerSourcemapOutput = result.packagerSourcemapOutput |
There was a problem hiding this comment.
Bug: The script incorrectly requires an intermediate source map directory, causing source map uploads to be silently skipped for all non-Hermes builds.
Severity: HIGH
Suggested Fix
Modify extractBundleTaskArguments to treat jsIntermediateSourceMapsDir as optional. The logic should proceed correctly even if this property is null or missing, ensuring source map uploads are not skipped.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/sentry.gradle.kts#L322
Potential issue: The script's logic in `extractBundleTaskArguments` now requires the
`jsIntermediateSourceMapsDir` property to be present. For React Native projects not
using the Hermes engine, this property is often missing. When it is, the function
returns early with null values. This causes the parent `processVariant` function to also
return early, effectively and silently disabling Sentry's source map upload
functionality for all non-Hermes builds, which is a major feature regression.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5a227d5. Configure here.
| var reactRoot: File? = props["workingDir"] as? File | ||
| if (reactRoot == null) { | ||
| val rootProvider = props["root"] as? org.gradle.api.provider.Provider<*> | ||
| reactRoot = rootProvider?.get() as? File |
There was a problem hiding this comment.
Directory to File conversion missing for root property
High Severity
The root property of BundleHermesCTask is a DirectoryProperty, so rootProvider?.get() returns an org.gradle.api.file.Directory object — not a java.io.File. The cast as? File silently returns null, causing reactRoot to always be null on RN 0.71+. The method then returns early, and source maps are never uploaded. The same file already handles this correctly in extractBundleTaskArguments using is org.gradle.api.file.Directory -> dir.asFile, but the same pattern was not applied here.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5a227d5. Configure here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| val osCompatibility = if (Os.isFamily(Os.FAMILY_WINDOWS)) listOf("cmd", "/c", "node") else emptyList() | ||
| if (System.getenv("SENTRY_DOTENV_PATH") == null && file("$reactRoot/.env.sentry-build-plugin").exists()) { | ||
| environment("SENTRY_DOTENV_PATH", "$reactRoot/.env.sentry-build-plugin") | ||
| } | ||
| commandLine(osCompatibility + args) |
There was a problem hiding this comment.
Windows sentry-cli invocation incorrectly prepends 'node' before the CLI executable
In the doLast exec spec for the Sentry CLI upload task, the Windows osCompatibility prefix is set to listOf("cmd", "/c", "node"), but the args list begins with cliExecutable (the sentry-cli binary, e.g. sentry-cli.exe or a path under node_modules/.bin), not a JavaScript file. The resulting command line becomes cmd /c node <sentry-cli> ..., which will attempt to execute the sentry-cli binary as a Node.js script and fail on Windows. The previous copyDebugIdScript exec correctly uses listOf("cmd", "/c") because that command does invoke node. This will break Sentry source map uploads for React Native Android builds on Windows hosts.
Verification
Read lines 369-548 of packages/core/sentry.gradle.kts. Compared the Windows handling at line 375 (copyDebugIdScript invocation) which uses listOf("cmd", "/c") because args[0] is "node", versus line 471 where args[0] is cliExecutable (set at line 436 to either sentryProps.getProperty("cli.executable") or "$cliPackage/bin/sentry-cli"). Adding "node" to the prefix would cause Windows to execute sentry-cli as if it were a node script.
Identified by Warden code-review · 4PF-YEM
| val process = ProcessBuilder(listOf("node", hasSourceMapDebugIdScript, sourcemapOutput.toString())) | ||
| .directory(reactRoot) | ||
| .start() | ||
| process.waitFor() | ||
| project.logger.lifecycle("Check generated source map for Debug ID: ${process.inputStream.bufferedReader().readText()}") |
There was a problem hiding this comment.
ProcessBuilder may deadlock waiting for child process with full stdout pipe
process.waitFor() is called before reading stdout. If the has-sourcemap-debugid.js script writes more output than the OS pipe buffer can hold (typically 64KB on Linux), the child process will block writing to stdout and waitFor() will block forever, hanging the build. The standard fix is to use redirectErrorStream(true) plus consuming the stream before waitFor(), or ProcessBuilder.Redirect.PIPE with concurrent reading.
Verification
Read lines 379-383: ProcessBuilder is constructed without redirecting stderr or pre-consuming stdout, then waitFor() is called and stdout is read only afterward. This is a well-known JVM Process pitfall. Even though the script likely outputs only a small amount in normal operation, any unexpected verbose output or warning could fill the pipe buffer.
Identified by Warden find-bugs · 93X-HEJ
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d2eadf8+dirty | 3842.42 ms | 1228.91 ms | -2613.50 ms |
| ad66da3+dirty | 3820.96 ms | 1214.43 ms | -2606.52 ms |
| 5fe1c6c+dirty | 1220.79 ms | 1217.63 ms | -3.16 ms |
| 0d9949d+dirty | 1211.38 ms | 1219.67 ms | 8.29 ms |
| bc0d8cf+dirty | 3830.33 ms | 1220.52 ms | -2609.81 ms |
| 3817909+dirty | 1183.90 ms | 1187.50 ms | 3.60 ms |
| 04207c4+dirty | 1191.27 ms | 1189.78 ms | -1.48 ms |
| 100ce80+dirty | 3842.93 ms | 1229.52 ms | -2613.41 ms |
| 3d377b5+dirty | 1218.48 ms | 1219.51 ms | 1.03 ms |
| 890d145+dirty | 1223.59 ms | 1231.37 ms | 7.78 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d2eadf8+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| ad66da3+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 5fe1c6c+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 0d9949d+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| bc0d8cf+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 3817909+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 04207c4+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 100ce80+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 3d377b5+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d2eadf8+dirty | 3841.53 ms | 1216.15 ms | -2625.39 ms |
| ad66da3+dirty | 3855.02 ms | 1213.43 ms | -2641.59 ms |
| 5fe1c6c+dirty | 1201.36 ms | 1209.15 ms | 7.78 ms |
| 0d9949d+dirty | 1203.94 ms | 1202.27 ms | -1.67 ms |
| bc0d8cf+dirty | 3834.64 ms | 1223.91 ms | -2610.73 ms |
| 3817909+dirty | 1210.76 ms | 1215.64 ms | 4.89 ms |
| 04207c4+dirty | 1228.55 ms | 1226.04 ms | -2.51 ms |
| 100ce80+dirty | 3843.57 ms | 1226.46 ms | -2617.12 ms |
| 3d377b5+dirty | 1201.55 ms | 1201.80 ms | 0.25 ms |
| 890d145+dirty | 1212.98 ms | 1220.10 ms | 7.12 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d2eadf8+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| ad66da3+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 5fe1c6c+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 0d9949d+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| bc0d8cf+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 3817909+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 04207c4+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 100ce80+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 3d377b5+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
1 participant
📢 Type of change
📜 Description
Converts
sentry.gradle(Groovy) tosentry.gradle.kts(Kotlin DSL).BundleTaskArgs,ForceSourceMapResult,VariantInfo) instead of Groovy dynamic listsgroovy.lang.Closure<Boolean>withdoCall()forextra[...]properties to ensure proper Groovy interop from consumingbuild.gradlefilesjava.lang.reflect.Proxy) since AGP types aren't on the.ktsscript classpathList.execute()withProcessBuilderfor process executiongroovy.json.JsonSlurper/JsonOutputfor JSON (same library, guaranteed available in Gradle)extractBundleTaskArgumentsLegacy(RN < 0.71 dead code — minimum supported is 0.71+)sentry.gradlereferences tosentry.gradle.ktson upgradebuild.gradlereferences across samples, e2e tests, perf tests, and patch scripts💡 Motivation and Context
Closes #2327
Gradle's Kotlin DSL provides type safety, better IDE support, and is the recommended script language going forward. The Groovy
.gradleformat is in maintenance mode. This aligns with the sentry-java/android SDK which already uses Kotlin DSL.💚 How did you test it?
modifyAppBuildGradle, including new migration test)sentryExpoNativeCheck)📝 Checklist
sendDefaultPIIis enabled🔮 Next steps
sentry.gradle.kts