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

Please create Bugsnag tasks using Gradle's creation avoidance API #201

Closed
rallat opened this issue Apr 30, 2020 · 5 comments
Closed

Please create Bugsnag tasks using Gradle's creation avoidance API #201

rallat opened this issue Apr 30, 2020 · 5 comments
Labels
feature request Request for a new feature released This feature/bug fix has been released

Comments

@rallat
Copy link

rallat commented Apr 30, 2020

Expected behavior

Create bugsnag tasks lazily using this API
https://docs.gradle.org/current/userguide/task_configuration_avoidance.html
Rather than creating them you can register them

Observed behavior

Plugin should lazily create tasks to avoid hitting configuration time.
Our build scan shows that the bugsnag plugin is creating 921 task at configuration time. However, most of them are for variants not related to the task running so I believe they could be registered instead of created as shared above.

com.bugsnag.android.gradle | Plugin | 0.186s |   | Applied to 1 project | 916 tasks created

Steps to reproduce

Run a build --scan https://guides.gradle.org/creating-build-scans/
and go to Performance > configuration > search for com.bugsnag.android.gradle
For us it creates 921 tasks the creation can be deferred to when they are actually needed

Version

4.22.3

Additional information

[Insert any additional information]

@mattdyoung
Copy link

Hi @rallat

Thanks for the suggestion! This sounds like a useful improvement to our gradle plugin so we'll take a look at doing this when we have the bandwidth.

@mattdyoung mattdyoung added backlog We hope to fix this feature/bug in the future feature request Request for a new feature labels May 1, 2020
@bugsnagbot bugsnagbot added scheduled Work is starting on this feature/bug and removed backlog We hope to fix this feature/bug in the future labels Jun 30, 2020
@ZacSweers
Copy link
Contributor

ZacSweers commented Aug 9, 2020

This is mostly done on the v5 branch, there's just one little problem area in the plugin's registerBugsnagTasksForVariant() logic that needs cleaning up. I've been talking with @fractalwrench a bit about it, started down that path but haven't quite gotten it working just yet. Here's a patch of what I got to so far, want to revisit after #257 is in

Index: src/main/kotlin/com.bugsnag.android.gradle/MappingFileProvider.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/kotlin/com.bugsnag.android.gradle/MappingFileProvider.kt	(revision 714a6e91f9fbdefefe9494fb3b3d19e492e95a8e)
+++ src/main/kotlin/com.bugsnag.android.gradle/MappingFileProvider.kt	(date 1596949593000)
@@ -3,9 +3,7 @@
 import com.android.build.gradle.AppExtension
 import com.android.build.gradle.api.ApkVariant
 import com.android.build.gradle.api.ApkVariantOutput
-import org.gradle.api.GradleException
 import org.gradle.api.Project
-import org.gradle.api.file.Directory
 import org.gradle.api.file.RegularFile
 import org.gradle.api.provider.Provider
 import java.io.File
@@ -14,9 +12,11 @@
 /**
  * Creates a Provider which finds the mapping file for a given variantOutput.
  */
-fun createMappingFileProvider(project: Project,
-                              variant: ApkVariant,
-                              variantOutput: ApkVariantOutput): Provider<RegularFile> {
+fun createMappingFileProvider(
+    project: Project,
+    variant: ApkVariant,
+    variantOutput: ApkVariantOutput
+): Provider<RegularFile> {
     val fileProvider: Provider<File> = project.provider {
         val mappingFile = findMappingFile(project, variant, variantOutput)
         val logger = project.logger
Index: src/main/kotlin/com.bugsnag.android.gradle/BugsnagPlugin.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/kotlin/com.bugsnag.android.gradle/BugsnagPlugin.kt	(revision 714a6e91f9fbdefefe9494fb3b3d19e492e95a8e)
+++ src/main/kotlin/com.bugsnag.android.gradle/BugsnagPlugin.kt	(date 1596950485000)
@@ -12,6 +12,7 @@
 import org.gradle.api.Plugin
 import org.gradle.api.Project
 import org.gradle.api.Task
+import org.gradle.api.file.ConfigurableFileCollection
 import org.gradle.api.file.RegularFile
 import org.gradle.api.provider.Provider
 import org.gradle.api.tasks.TaskProvider
@@ -123,8 +124,8 @@
         variant: ApkVariant,
         bugsnag: BugsnagPluginExtension
     ) {
-        variant.outputs.configureEach {
-            val output = it as ApkVariantOutput
+        variant.outputs.configureEach { baseVariantOutput ->
+            val output = baseVariantOutput as ApkVariantOutput
             val jvmMinificationEnabled = variant.buildType.isMinifyEnabled || hasDexguardPlugin(project)
             val ndkEnabled = isNdkUploadEnabled(bugsnag, project.extensions.getByType(AppExtension::class.java))
 
@@ -134,36 +135,44 @@
             }
 
             // register bugsnag tasks
-            val manifestInfoFile = registerManifestUuidTask(project, variant, output)
-            val proguardTask = when {
-                jvmMinificationEnabled -> registerProguardUploadTask(project, variant, output, bugsnag)
-                else -> null
-            }
-            val symbolFileTask = when {
-                ndkEnabled -> registerSharedObjectUploadTask(project, variant, output, bugsnag)
-                else -> null
-            }
-            val releasesTask = registerReleasesUploadTask(project, variant, output, bugsnag)
-
             // Set the manifest info provider to prevent reading
             // the manifest more than once
-            proguardTask?.get()?.let { task ->
-                task.manifestInfoFile.set(manifestInfoFile)
-                val mappingFileProvider = createMappingFileProvider(project, variant, output)
-                task.mappingFileProperty.set(mappingFileProvider)
-                releasesTask.get().jvmMappingFileProperty.set(mappingFileProvider)
-                task.uploadRequestClient.set(proguardUploadClient)
+            // TODO make mappingfileprovider a task and cacheable?
+            val mappingFileProvider = createMappingFileProvider(project, variant, output)
+            val manifestInfoFileProvider = registerManifestUuidTask(project, variant, output)
+            val proguardTaskProvider = when {
+                jvmMinificationEnabled -> registerProguardUploadTask(
+                    project,
+                    variant,
+                    output,
+                    bugsnag,
+                    manifestInfoFileProvider,
+                    mappingFileProvider,
+                    proguardUploadClient
+                )
+                else -> null
             }
-
-            symbolFileTask?.get()?.let { task ->
-                val ndkSearchDirs = symbolFileTask.get().searchDirectories
-                releasesTask.get().ndkMappingFileProperty.from(ndkSearchDirs)
-                task.uploadRequestClient.set(ndkUploadClient)
+            val symbolFileTaskProvider = when {
+                ndkEnabled -> registerSharedObjectUploadTask(
+                    project,
+                    variant,
+                    output,
+                    bugsnag,
+                    manifestInfoFileProvider,
+                    ndkUploadClient
+                )
+                else -> null
             }
-            releasesTask.get().uploadRequestClient.set(releasesUploadClient)
-            releasesTask.get().manifestInfoFile.set(manifestInfoFile)
-            symbolFileTask?.get()?.manifestInfoFile?.set(manifestInfoFile)
-            releasesTask.get().manifestInfoFile.set(manifestInfoFile)
+            registerReleasesUploadTask(
+                project,
+                variant,
+                output,
+                bugsnag,
+                manifestInfoFileProvider,
+                if (proguardTaskProvider != null) mappingFileProvider else null,
+                releasesUploadClient,
+                symbolFileTaskProvider?.map { it.searchDirectories }
+            )
         }
     }
 
@@ -201,18 +210,26 @@
     /**
      * Creates a bugsnag task to upload proguard mapping file
      */
-    private fun registerProguardUploadTask(project: Project,
-                                           variant: ApkVariant,
-                                           output: ApkVariantOutput,
-                                           bugsnag: BugsnagPluginExtension): TaskProvider<BugsnagUploadProguardTask> {
+    private fun registerProguardUploadTask(
+        project: Project,
+        variant: ApkVariant,
+        output: ApkVariantOutput,
+        bugsnag: BugsnagPluginExtension,
+        manifestInfoFileProvider: Provider<RegularFile>,
+        mappingFileProvider: Provider<RegularFile>,
+        uploadRequestClient: UploadRequestClient
+    ): TaskProvider<BugsnagUploadProguardTask> {
         val outputName = taskNameForOutput(output)
         val taskName = "uploadBugsnag${outputName}Mapping"
         val path = "intermediates/bugsnag/requests/proguardFor${outputName}.json"
-        val requestOutputFile = project.layout.buildDirectory.file(path)
-        return project.tasks.register(taskName, BugsnagUploadProguardTask::class.java) {
-            it.requestOutputFile.set(requestOutputFile)
-            addTaskToExecutionGraph(it, variant, output, project, bugsnag, bugsnag.uploadJvmMappings.get())
-            it.configureWith(bugsnag)
+        val requestOutputFileProvider = project.layout.buildDirectory.file(path)
+        return project.tasks.register<BugsnagUploadProguardTask>(taskName) {
+            requestOutputFile.set(requestOutputFileProvider)
+            manifestInfoFile.set(manifestInfoFileProvider)
+            mappingFileProperty.set(mappingFileProvider)
+            this.uploadRequestClient.set(uploadRequestClient)
+            addTaskToExecutionGraph(variant, output, project, bugsnag, bugsnag.uploadJvmMappings.get())
+            configureWith(bugsnag)
         }
     }
 
@@ -220,20 +237,24 @@
         project: Project,
         variant: ApkVariant,
         output: ApkVariantOutput,
-        bugsnag: BugsnagPluginExtension
+        bugsnag: BugsnagPluginExtension,
+        manifestInfoFileProvider: Provider<RegularFile>,
+        uploadRequestClient: UploadRequestClient
     ): TaskProvider<out BugsnagUploadNdkTask> {
         // Create a Bugsnag task to upload NDK mapping file(s)
         val outputName = taskNameForOutput(output)
         val taskName = "uploadBugsnagNdk${outputName}Mapping"
         val path = "intermediates/bugsnag/requests/ndkFor${outputName}.json"
-        val requestOutputFile = project.layout.buildDirectory.file(path)
+        val requestOutputFileProvider = project.layout.buildDirectory.file(path)
         return BugsnagUploadNdkTask.register(project, taskName) {
-            this.requestOutputFile.set(requestOutputFile)
+            requestOutputFile.set(requestOutputFileProvider)
             projectRoot.set(bugsnag.projectRoot.getOrElse(project.projectDir.toString()))
             searchDirectories.from(getSearchDirectories(project, variant))
             variantOutput = output
             objDumpPaths.set(bugsnag.objdumpPaths)
-            addTaskToExecutionGraph(this, variant, output, project, bugsnag, true)
+            manifestInfoFile.set(manifestInfoFileProvider)
+            this.uploadRequestClient.set(uploadRequestClient)
+            addTaskToExecutionGraph(variant, output, project, bugsnag, true)
             configureWith(bugsnag)
         }
     }
@@ -242,7 +263,11 @@
         project: Project,
         variant: ApkVariant,
         output: ApkVariantOutput,
-        bugsnag: BugsnagPluginExtension
+        bugsnag: BugsnagPluginExtension,
+        manifestInfoFileProvider: Provider<RegularFile>,
+        mappingFileProvider: Provider<RegularFile>?,
+        uploadClient: UploadRequestClient,
+        searchDirectories: Provider<ConfigurableFileCollection>?
     ): TaskProvider<out BugsnagReleasesTask> {
         val outputName = taskNameForOutput(output)
         val taskName = "bugsnagRelease${outputName}Task"
@@ -259,23 +284,28 @@
             metadata.set(bugsnag.metadata)
             builderName.set(bugsnag.builderName)
             gradleVersion.set(project.gradle.gradleVersion)
-            addTaskToExecutionGraph(this, variant, output, project, bugsnag, bugsnag.reportBuilds.get())
+            mappingFileProvider?.let { jvmMappingFileProperty.set(it) }
+            manifestInfoFile.set(manifestInfoFileProvider)
+            this.uploadRequestClient.set(uploadClient)
+            searchDirectories?.let { ndkMappingFileProperty.from(it) }
+            addTaskToExecutionGraph(variant, output, project, bugsnag, bugsnag.reportBuilds.get())
             configureMetadata()
         }
     }
 
-    private fun addTaskToExecutionGraph(task: Task,
-                                        variant: ApkVariant,
-                                        output: ApkVariantOutput,
-                                        project: Project,
-                                        bugsnag: BugsnagPluginExtension,
-                                        autoUpload: Boolean) {
+    private fun Task.addTaskToExecutionGraph(
+        variant: ApkVariant,
+        output: ApkVariantOutput,
+        project: Project,
+        bugsnag: BugsnagPluginExtension,
+        autoUpload: Boolean
+    ) {
         if (shouldUploadDebugMappings(output, bugsnag)) {
             findAssembleBundleTasks(project, variant, output).forEach {
-                task.mustRunAfter(it)
+                mustRunAfter(it)
 
                 if (autoUpload) {
-                    it.finalizedBy(task)
+                    it.finalizedBy(this)
                 }
             }
         }

@ZacSweers
Copy link
Contributor

Should clarify, #257 does completely remove remaining create() calls, but it's not quite enough because of the above manual task configuration via get() calls

@ZacSweers
Copy link
Contributor

I think this is done now!

@phillipsam
Copy link

This has been released in v5.0.0

@phillipsam phillipsam added released This feature/bug fix has been released and removed scheduled Work is starting on this feature/bug labels Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature released This feature/bug fix has been released
Projects
None yet
Development

No branches or pull requests

5 participants