Move task operations out of onVariants to fix AGP Artifacts API confl…#5690
Move task operations out of onVariants to fix AGP Artifacts API confl…#5690RyanCommits wants to merge 5 commits intogetsentry:mainfrom
Conversation
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. |
|
@sentry review |
There was a problem hiding this comment.
Thank you for your contribution @RyanCommits! We really appreciate this🙇
I understand that the PR is still draft but since it came to our attention I've added some early feedback. We would also appreciate a small repro to help us test the fix.
Co-authored-by: Antonis Lilis <antonis.lilis@gmail.com>
Co-authored-by: Antonis Lilis <antonis.lilis@gmail.com>
|
Hi @antonis, thank you for the feedback. I've updated the branch with your feedback, as well as simplified what I was doing. I have a repro of it issue here: This other branch has the fix implemented: |
![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 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // callbacks and the AGP Artifacts API transform chain is fully established. | ||
| project.afterEvaluate { | ||
| if (releaseVariants.isEmpty()) { | ||
| project.logger.warn("[sentry] No release variants collected, onVariants may have run after afterEvaluate. Sourcemap upload tasks will not be registered.") |
There was a problem hiding this comment.
Misleading warning for projects with only debug variants
Low Severity
The warning assumes releaseVariants is empty because onVariants ran after afterEvaluate, but it could also be empty because the project legitimately has no release variants (only debug). The warning message "onVariants may have run after afterEvaluate" is misleading in this case and could confuse developers working on debug-only configurations or sample projects. The code cannot distinguish between timing issues and the legitimate absence of release variants.
| task.name.endsWith("JsAndAssets") && | ||
| !task.name.contains("Debug") | ||
| }.each { bundleTask -> | ||
| if (!bundleTask.enabled) return |
There was a problem hiding this comment.
Inefficient nested loop processes each bundle task multiple times
Medium Severity
The nested loop structure iterates over all release variants in the outer loop and all bundle tasks in the inner loop, causing each bundle task to be processed N times (once per variant). Lines 98-112 execute for every (variant, bundleTask) combination, but extractCurrentVariants only returns non-null for one matching variant per bundle task. This means (N-1)×M iterations do unnecessary work (extracting task properties, calling forceSourceMapOutputFromBundleTask) before returning early on line 122. The loops should be restructured to process each bundle task once with its matching variant.
There was a problem hiding this comment.
Thank you for this fix, Ryan. Also thanks to the Sentry folks taking a quick look at this. I have some additional code that can be used to reproduce and test the issue. This is all Groovy code that can be added to an Android app module's build.gradle.
Note that while this suggested fix does address the current issue, it's still possible to run into similar issues in the future if other tasks are configured in afterEvaluate. Ideally this file would be converted to using lazy evaluation (API's like configureEach -- tasks.matching {}.each {} resolves immediately), but that will take a larger effort since tasks are currently being created for matched tasks. Those can't be put into configureEach, because the tasks won't be registered unless the "bundle" tasks are resolved. Instead the cli and module tasks would need to be known and created ahead of resolving the "bundle" tasks.
import com.android.build.api.artifact.ArtifactTransformationRequest
import com.android.build.api.artifact.SingleArtifact
import com.android.build.api.variant.ApplicationVariant
import com.android.build.api.variant.BuiltArtifact
import javax.inject.Inject
import java.nio.file.Files
import java.nio.file.StandardCopyOption
buildscript {
repositories {
google()
mavenCentral()
}
dependencies {
classpath "com.android.tools.build:gradle-api:9.0.1"
}
}
androidComponents.onVariants(androidComponents.selector().all()) { ApplicationVariant variant ->
def variantName = variant.name.capitalize()
// Early task resolution cause incorrect output destination
// tasks.matching { task -> task.name.startsWith("assemble") || task.name.endsWith("bundle") }.forEach {}
// Configure artifact transformations. This can be put in an `afterEvaluate` block and still be valid.
// APK ends up in intermediates directory
def copyApkTask = project.tasks.register("copy${variantName}Apks", CopyApkTask)
def transformationRequest = variant.artifacts.use(copyApkTask)
.wiredWithDirectories(CopyApkTask::getInput, CopyApkTask::getOutput)
.toTransformMany(SingleArtifact.APK.INSTANCE)
copyApkTask.configure { CopyApkTask task ->
task.transformationRequest.set(transformationRequest)
}
// Bundle ends up with wrong extension
def copyBundleTask = project.tasks.register("copy${variantName}Bundle", CopyBundleTask)
variant.artifacts.use(copyBundleTask)
.wiredWithFiles(CopyBundleTask::getInput, CopyBundleTask::getOutput)
.toTransform(SingleArtifact.BUNDLE.INSTANCE)
}
abstract class CopyApkTask extends DefaultTask {
private final WorkerExecutor workers
@Inject
CopyApkTask(WorkerExecutor workers) {
this.workers = workers
}
@InputFiles
abstract DirectoryProperty getInput();
@OutputDirectory
abstract DirectoryProperty getOutput();
@Internal
abstract Property<ArtifactTransformationRequest<CopyApkTask>> getTransformationRequest();
@TaskAction
void copyApk() {
transformationRequest.get().submit(this, workers.noIsolation(), CopyApksWorkItem) {
BuiltArtifact builtArtifact, Directory outputLocation, CopyApksWorkItemParameters params ->
def inputFile = new File(builtArtifact.outputFile)
def outputFile = new File(outputLocation.asFile, inputFile.name)
params.inputApkFile.set(inputFile)
params.outputApkFile.set(outputFile)
outputFile
}
}
}
interface CopyApksWorkItemParameters extends WorkParameters, Serializable {
RegularFileProperty getInputApkFile()
RegularFileProperty getOutputApkFile()
}
abstract class CopyApksWorkItem implements WorkAction<CopyApksWorkItemParameters> {
final CopyApksWorkItemParameters workItemParameters
@Inject
CopyApksWorkItem(CopyApksWorkItemParameters workItemParameters) {
this.workItemParameters = workItemParameters
}
@Override
void execute() {
def input = workItemParameters.inputApkFile.get().asFile
def output = workItemParameters.outputApkFile.get().asFile
FileUtil.copy(input, output)
}
}
class FileUtil {
static void copy(File src, File dst) {
println "Copying $src to $dst"
dst.parentFile.mkdirs()
dst.delete()
Files.copy(src.toPath(), dst.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES)
}
}
abstract class CopyBundleTask extends DefaultTask {
@InputFile
abstract RegularFileProperty getInput();
@OutputFile
abstract RegularFileProperty getOutput();
@TaskAction
void copyBundle() {
def input = getInput().get().asFile
def output = getOutput().get().asFile
FileUtil.copy(input, output)
}
}| // callbacks and the AGP Artifacts API transform chain is fully established. | ||
| project.afterEvaluate { | ||
| if (releaseVariants.isEmpty()) { | ||
| project.logger.warn("[sentry] No release variants collected, onVariants may have run after afterEvaluate. Sourcemap upload tasks will not be registered.") |
There was a problem hiding this comment.
This warning is inaccurate. It should not be possible for onVariants to have been invoked afterEvaluate as it is called when the plugin is applied, which should not be in or after afterEvaluate in this case. Though it could indicate no release variants configured, as the AI review stated.
There was a problem hiding this comment.
wdyt of simplifying the message to [sentry] No release variants found. Source map upload tasks will not be registered.?
There was a problem hiding this comment.
Sounds great. The one you've got in #5714 looks good too!
|
The PR looks good! Would you be able to edit the changelog and add the following snippet? |
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Thank you for iterating with the fixes @RyanCommits 🙇
The PR looks overall good but we realized that it reverts the changes in #5253 and would cause a regression of #5236
We will investigate this further and iterate back.
….gradle
project.afterEvaluate{} is not needed: bundle tasks are already registered
by the time onVariants fires, matching the timing of the original tasks.findAll.
Moving the tasks.names.contains() check and tasks.named().configure{} directly
into onVariants keeps the fix simple and avoids the regression risk that
afterEvaluate introduced in the earlier PR #5690.
Also fixes the indentation of the ~240-line configure{} closure body so it
is visually distinct from the enclosing onVariants block.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 participants
📢 Type of change
📜 Description
Fixes issue where calling tasks.matching{}.each{} inside androidComponents.onVariants()
forces task realization during AGP's variant configuration phase, disrupting the
Artifacts API transform chain used by other plugins (e.g. Fullstory).
This caused APK transforms from other plugins to output to build/intermediates/
instead of build/outputs/, breaking downstream tooling that expects APKs in the
standard location.
The fix:
project.afterEvaluate within the plugins.withId block
This ensures the AGP Artifacts API transform chain is fully established before
Sentry accesses any tasks, preventing interference with other plugins.
💡 Motivation and Context
This is currently conflicting with the Fullstory plugin
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps