Avoid eagerly evaluating unstrippedNativeLibsOverride during the configuration phase

firebase-crashlytics-gradle/src/functionalTest/kotlin/com/google/firebase/crashlytics/buildtools/gradle/CrashlyticsExtensionTests.kt

@@ -62,70 +62,46 @@ class CrashlyticsExtensionTests {
   @Test
   fun `set unstrippedNativeLibsDir to single path`() {
     buildFile.writeText(
-      """
-        import com.google.firebase.crashlytics.buildtools.gradle.CrashlyticsExtension
-
-        plugins {
-          id("com.android.application") version "8.1.4"
-          id("com.google.gms.google-services") version "4.4.1"
-          id("com.google.firebase.crashlytics") version "$pluginVersion"
-        }
-
-        android {
-          compileSdk = 33
-          namespace = "com.google.firebase.testing.crashlytics"
-
-          buildTypes {
-            debug {
-              configure<CrashlyticsExtension> {
-                unstrippedNativeLibsDir = "/some/absolute/string/path"
-              }
-            }
-          }
-        }
-      """
+      getBuildFileStringTemplate("unstrippedNativeLibsDir = \"/some/absolute/string/path\"")
     )
 
-    val result = buildGradleRunner(projectDir, "-d", ":tasks", "--configuration-cache")
+    val result =
+      buildGradleRunner(
+        projectDir,
+        "generateCrashlyticsSymbolFileDebug",
+        "verifyCrashlyticsPaths",
+        "--configuration-cache"
+      )
 
     assertThat(result.output).contains("/some/absolute/string/path")
   }
 
   @Test
   fun `set unstrippedNativeLibsDir to array of multiple path types`() {
     buildFile.writeText(
-      """
-        import com.google.firebase.crashlytics.buildtools.gradle.CrashlyticsExtension
-
-        plugins {
-          id("com.android.application") version "8.1.4"
-          id("com.google.gms.google-services") version "4.4.1"
-          id("com.google.firebase.crashlytics") version "$pluginVersion"
-        }
-
-        android {
-          compileSdk = 33
-          namespace = "com.google.firebase.testing.crashlytics"
-
-          buildTypes {
-            debug {
-              configure<CrashlyticsExtension> {
-                unstrippedNativeLibsDir = arrayOf(
+      getBuildFileStringTemplate(
+        """
+        unstrippedNativeLibsDir = arrayOf(
                   "/some/absolute/string/path",
                   "/another/absolute/string/path",
                   File("/a/file/object/path"),
                   project.files("/absolute/project/file/path"),
                   "relative/path",
                   project.files("relative/project/file/path"),
+                  "build/intermediates/merged_native_libs/debug/out"
                 )
-              }
-            }
-          }
-        }
-      """
+    """
+          .trimIndent()
+      )
     )
 
-    val result = buildGradleRunner(projectDir, "-d", ":tasks", "--configuration-cache")
+    val result =
+      buildGradleRunner(
+        projectDir,
+        "generateCrashlyticsSymbolFileDebug",
+        "verifyCrashlyticsPaths",
+        "--configuration-cache"
+      )
 
     assertThat(result.output).contains("/some/absolute/string/path")
     assertThat(result.output).contains("/another/absolute/string/path")
@@ -135,12 +111,33 @@ class CrashlyticsExtensionTests {
     // Verify the relative paths were made absolute by checking the prepended slash
     assertThat(result.output).contains("/relative/path")
     assertThat(result.output).contains("/relative/project/file/path")
+
+    // Verify warning is not present when manual overrides points to mergeDebugNativeLibs output but
+    // is set up using an array.
+    // The warning is expected to be shown as a notification for users who in the past make use of a
+    // widely known workaround for flavors and native merged libraries handling.
+    // There is no real use case for passing this path in an array, but just to prove the solution
+    // only aims for single provided overrides this check is added.
+    assertThat(result.output)
+      .doesNotContain("it is safe to remove from the unstrippedNativeLibsDir")
   }
 
   @Test
   fun `set unstrippedNativeLibsDir to invalid type throws`() {
-    buildFile.writeText(
-      """
+    buildFile.writeText(getBuildFileStringTemplate("unstrippedNativeLibsDir = 42"))
+
+    val thrown =
+      Assertions.assertThrows(UnexpectedBuildFailure::class.java) {
+        buildGradleRunner(projectDir, "generateCrashlyticsSymbolFileDebug", "--configuration-cache")
+      }
+
+    assertThat(thrown)
+      .hasMessageThat()
+      .contains("Cannot convert the provided notation to a File: 42")
+  }
+
+  private fun getBuildFileStringTemplate(unstrippedNativeLibsDirArg: String): String =
+    """
         import com.google.firebase.crashlytics.buildtools.gradle.CrashlyticsExtension
 
         plugins {
@@ -156,21 +153,29 @@ class CrashlyticsExtensionTests {
           buildTypes {
             debug {
               configure<CrashlyticsExtension> {
-                unstrippedNativeLibsDir = 42
+                nativeSymbolUploadEnabled = true
+                $unstrippedNativeLibsDirArg
               }
             }
           }
         }
+
+        // Custom test task to print the configured paths at realization phase.
+        abstract class VerifyPathsTask : DefaultTask() {
+            @get:InputFiles
+            abstract val filesToVerify: ConfigurableFileCollection
+            @TaskAction
+            fun verify() {
+                filesToVerify.forEach { 
+                    println("VERIFIED_PATH=" + it.absolutePath) 
+                }
+            }
+        }
+        tasks.register<VerifyPathsTask>("verifyCrashlyticsPaths") {
+            val debugBuildType = android.buildTypes.getByName("debug")
+            val extension = debugBuildType.extensions.getByName("firebaseCrashlytics") as CrashlyticsExtension
+
+            filesToVerify.setFrom(extension.unstrippedNativeLibsDir)
+        }
       """
-    )
-
-    val thrown =
-      Assertions.assertThrows(UnexpectedBuildFailure::class.java) {
-        buildGradleRunner(projectDir, "-d", ":tasks", "--configuration-cache")
-      }
-
-    assertThat(thrown)
-      .hasMessageThat()
-      .contains("Cannot convert the provided notation to a File: 42")
-  }
 }

firebase-crashlytics-gradle/src/main/kotlin/com/google/firebase/crashlytics/buildtools/gradle/CrashlyticsVariantExtension.kt

@@ -47,7 +47,22 @@ constructor(config: VariantExtensionConfig<*>) : VariantExtension, Serializable
       // Only set the properties for non-null values.
       crashlyticsExtension.mappingFileUploadEnabled?.let(mappingFileUploadEnabled::set)
       crashlyticsExtension.nativeSymbolUploadEnabled?.let(nativeSymbolUploadEnabled::set)
-      crashlyticsExtension.unstrippedNativeLibsDir?.let { unstrippedNativeLibsOverride.from(it) }
+      crashlyticsExtension.unstrippedNativeLibsDir?.let { nativeLibsOverride ->
+        if (
+          nativeLibsOverride is String &&
+            nativeLibsOverride.contains("/intermediates/merged_native_libs/")
+        ) {
+          logger.warn(
+            """
+                  The unstrippedNativeLibsDir is manually overridden.
+                  This is unnecessary, it is safe to remove from the unstrippedNativeLibsDir 
+                  override in the CrashlyticsExtension configuration block.
+              """
+              .trimIndent()
+          )
+        }
+        unstrippedNativeLibsOverride.from(nativeLibsOverride)
+      }
       crashlyticsExtension.symbolGeneratorType?.let(symbolGeneratorType::set)
       crashlyticsExtension.breakpadBinary?.let(breakpadBinary::set)
 
@@ -59,16 +74,5 @@ constructor(config: VariantExtensionConfig<*>) : VariantExtension, Serializable
         )
       }
     }
-
-    printDebugProperties(config)
-  }
-
-  private fun printDebugProperties(config: VariantExtensionConfig<*>) {
-    logger.debug("CrashlyticsVariantExtension for variant: ${config.variant.name}")
-    logger.debug("  mappingFileUploadEnabled: ${mappingFileUploadEnabled.orNull}")
-    logger.debug("  nativeSymbolUploadEnabled: ${nativeSymbolUploadEnabled.orNull}")
-    logger.debug("  unstrippedNativeLibsOverride: ${unstrippedNativeLibsOverride.asPath}")
-    logger.debug("  symbolGeneratorType: ${symbolGeneratorType.orNull}")
-    logger.debug("  breakpadBinary: ${breakpadBinary.orNull?.asFile?.path}")
   }
 }

firebase-crashlytics-gradle/src/main/kotlin/com/google/firebase/crashlytics/buildtools/gradle/tasks/GenerateSymbolFileTask.kt

@@ -31,7 +31,6 @@ import com.google.firebase.crashlytics.buildtools.ndk.internal.csym.NdkCSymGener
 import java.io.File
 import org.gradle.api.DefaultTask
 import org.gradle.api.Project
-import org.gradle.api.UnknownTaskException
 import org.gradle.api.file.ConfigurableFileCollection
 import org.gradle.api.file.DirectoryProperty
 import org.gradle.api.file.RegularFileProperty
@@ -87,46 +86,31 @@ abstract class GenerateSymbolFileTask : DefaultTask() {
   /**
    * Sets and validates the unstripped native libs directories.
    *
-   * Validate the [unstrippedNativeLibsOverride] does not contain a directory manually set to the
-   * output of the [SingleArtifact.MERGED_NATIVE_LIBS] without include the dependency.
+   * Validate the [unstrippedNativeLibsOverride] is not empty at execution phase, with a fallback
+   * natively pointing out to [SingleArtifact.MERGED_NATIVE_LIBS] directory.
    *
    * This happens because for a while the plugin did not properly handle product flavors, so
    * customers would manually configure this to the output of the merged native libs task in a way
    * that didn't include the task dependencies.
    */
-  private fun validateUnstrippedNativeLibsDirs(
-    project: Project,
+  private fun setUnstrippedNativeLibsDirs(
     variant: Variant,
     unstrippedNativeLibsOverride: ConfigurableFileCollection,
   ) {
-    if (unstrippedNativeLibsOverride.isEmpty) {
-      unstrippedNativeLibsDirs.setFrom(variant.artifacts.get(SingleArtifact.MERGED_NATIVE_LIBS))
-      return
-    }
-
-    val mergedNativeLibsOutput = "build/intermediates/merged_native_libs/${variant.name}/out"
-    if (unstrippedNativeLibsOverride.any { it.path.contains(mergedNativeLibsOutput) }) {
-      val mergedNativeLibsTask = "merge${variant.name.capitalized()}NativeLibs"
-
-      val dependencies =
-        unstrippedNativeLibsOverride.buildDependencies
-          .getDependencies(null)
-          .mapNotNull { it?.name }
-          .toSet()
-
-      if (!dependencies.contains(mergedNativeLibsTask)) {
-        try {
-          dependsOn(project.tasks.getByPath(mergedNativeLibsTask))
-        } catch (ex: UnknownTaskException) {
-          logger.warn(
-            "The unstrippedNativeLibsDir manually overridden to output of $mergedNativeLibsTask " +
-              "task. This is not necessary, it is safe to remove $mergedNativeLibsOutput from " +
-              "the unstrippedNativeLibsDir override."
-          )
+    // Extract default provider outside the lambda to avoid closure capture,
+    // thus preventing possible Gradle serialization issues.
+    val defaultMergedNativeLibs = variant.artifacts.get(SingleArtifact.MERGED_NATIVE_LIBS)
+
+    // Setting provider as a lazy mechanism which will be invoked at execution phase.
+    this.unstrippedNativeLibsDirs.setFrom(
+      project.provider {
+        if (unstrippedNativeLibsOverride.isEmpty) {
+          defaultMergedNativeLibs
+        } else {
+          unstrippedNativeLibsOverride
         }
       }
-    }
-    unstrippedNativeLibsDirs.setFrom(unstrippedNativeLibsOverride)
+    )
   }
 
   /** Sets and validates the symbol generator type. */
@@ -181,24 +165,12 @@ abstract class GenerateSymbolFileTask : DefaultTask() {
         this.breakpadExtractionDir.set(buildDir(project, variant, "dump_syms"))
         this.symbolFileOutputDir.set(buildDir(project, variant, "nativeSymbols"))
 
-        validateUnstrippedNativeLibsDirs(
-          project,
+        setUnstrippedNativeLibsDirs(
           variant,
           crashlyticsExtension.unstrippedNativeLibsOverride,
         )
 
         validateSymbolGeneratorType(project, crashlyticsExtension.symbolGeneratorType)
-
-        printDebugProperties()
       }
   }
-
-  private fun printDebugProperties() {
-    logger.debug("GenerateSymbolFileTask:")
-    logger.debug("  unstrippedNativeLibsDirs: ${unstrippedNativeLibsDirs.asPath}")
-    logger.debug("  breakpadBinary: ${breakpadBinary.orNull?.asFile?.path}")
-    logger.debug("  symbolGeneratorType: ${symbolGeneratorType.orNull?.toString()}")
-    logger.debug("  breakpadExtractionDir: ${breakpadExtractionDir.orNull?.asFile?.path}")
-    logger.debug("  symbolFileOutputDir: ${symbolFileOutputDir.orNull?.asFile?.path}")
-  }
 }