Merge pull request #517 from bugsnag/release/v7.5.0

Release v7.5.0

CHANGELOG.md

@@ -1,3 +1,10 @@
+## 7.5.0 (2023-04-20)
+
+### Enhancements
+
+* Determine the NDK version being used based on the `package.xml` files within the NDK directories, with a fallback to the directory-name.
+  [#514](https://github.com/bugsnag/bugsnag-android-gradle-plugin/pull/514)
+
 ## 7.4.1 (2023-02-22)
 
 ### Bug Fixes

gradle.properties

@@ -6,7 +6,7 @@ POM_NAME=Bugsnag Android Gradle Plugin
 POM_ARTIFACT_ID=bugsnag-android-gradle-plugin
 POM_PACKAGING=jar
 GROUP=com.bugsnag
-VERSION_NAME=7.4.1
+VERSION_NAME=7.5.0
 POM_DESCRIPTION=Gradle plugin to automatically upload ProGuard mapping files to Bugsnag.
 POM_URL=https://github.com/bugsnag/bugsnag-android-gradle-plugin/
 POM_SCM_URL=https://github.com/bugsnag/bugsnag-android-gradle-plugin/

src/main/kotlin/com/bugsnag/android/gradle/BugsnagPluginExtension.kt

@@ -85,7 +85,8 @@ open class BugsnagPluginExtension @Inject constructor(objects: ObjectFactory) {
     val objdumpPaths: MapProperty<String, String> = objects.mapProperty<String, String>()
         .convention(emptyMap())
 
-    val useLegacyNdkSymbolUpload: Property<Boolean> = objects.property<Boolean>().convention(true)
+    val useLegacyNdkSymbolUpload: Property<Boolean> = objects.property<Boolean>()
+        .convention(NULL_BOOLEAN)
 
     // exposes sourceControl as a nested object on the extension,
     // see https://docs.gradle.org/current/userguide/custom_gradle_types.html#nested_objects

src/main/kotlin/com/bugsnag/android/gradle/internal/NdkPackageXmlParser.kt

@@ -0,0 +1,61 @@
+package com.bugsnag.android.gradle.internal
+
+import org.gradle.util.VersionNumber
+import org.w3c.dom.Document
+import org.w3c.dom.Node
+import java.io.File
+import javax.xml.parsers.DocumentBuilderFactory
+
+internal object NdkPackageXmlParser {
+    private const val PACKAGE_XML_FILENAME = "package.xml"
+    private const val TAG_REVISION = "revision"
+    private const val TAG_MAJOR = "major"
+    private const val TAG_MINOR = "minor"
+    private const val TAG_MICRO = "micro"
+
+    internal fun loadVersionFromPackageXml(ndkDir: File): VersionNumber {
+        val packageFile = File(ndkDir, PACKAGE_XML_FILENAME)
+        var versionNumber: VersionNumber? = null
+        if (packageFile.canRead()) {
+            versionNumber = loadPackageXml(packageFile) { doc ->
+                val revision = doc.getElementsByTagName(TAG_REVISION).item(0)
+                val major = revision.getChildValue(TAG_MAJOR)
+                val minor = revision.getChildValue(TAG_MINOR)
+                val micro = revision.getChildValue(TAG_MICRO)
+
+                VersionNumber(
+                    major?.toIntOrNull() ?: 0,
+                    minor?.toIntOrNull() ?: 0,
+                    micro?.toIntOrNull() ?: 0,
+                    null
+                )
+            }
+        }
+
+        return versionNumber ?: VersionNumber.parse(ndkDir.name) // fallback to trying to parse the dir name
+    }
+
+    private inline fun <T> loadPackageXml(packageFile: File, action: (Document) -> T): T? {
+        return packageFile.inputStream().buffered().use { stream ->
+            @Suppress("TooGenericExceptionCaught", "SwallowedException")
+            try {
+                val builder = DocumentBuilderFactory.newInstance().newDocumentBuilder()
+                val document = builder.parse(stream)
+                action(document)
+            } catch (ex: Exception) {
+                null
+            }
+        }
+    }
+
+    private fun Node.getChildValue(elementName: String): String? {
+        val children = childNodes
+        repeat(children.length) { index ->
+            if (children.item(index).nodeName == elementName) {
+                return children.item(index).textContent
+            }
+        }
+
+        return null
+    }
+}

src/main/kotlin/com/bugsnag/android/gradle/internal/NdkToolchain.kt

@@ -12,10 +12,10 @@ import org.gradle.api.logging.Logger
 import org.gradle.api.logging.Logging
 import org.gradle.api.provider.MapProperty
 import org.gradle.api.provider.Property
+import org.gradle.api.provider.Provider
 import org.gradle.api.tasks.Input
 import org.gradle.api.tasks.Internal
 import org.gradle.api.tasks.Optional
-import org.gradle.api.tasks.StopExecutionException
 import org.gradle.util.VersionNumber
 import java.io.File
 
@@ -26,6 +26,7 @@ abstract class NdkToolchain {
     abstract val baseDir: DirectoryProperty
 
     @get:Input
+    @get:Optional
     abstract val useLegacyNdkSymbolUpload: Property<Boolean>
 
     @get:Input
@@ -41,6 +42,15 @@ abstract class NdkToolchain {
     private val logger: Logger = Logging.getLogger(this::class.java)
 
     fun preferredMappingTool(): MappingTool {
+        val forceNdkSymbolTool = useLegacyNdkSymbolUpload.orNull
+        if (forceNdkSymbolTool != null) {
+            return if (forceNdkSymbolTool == true) MappingTool.OBJDUMP else MappingTool.OBJCOPY
+        }
+
+        return detectMappingTool()
+    }
+
+    private fun detectMappingTool(): MappingTool {
         var legacyUploadRequired = bugsnagNdkVersion.orNull
             ?.let { VersionNumber.parse(it) }
             ?.let { it < MIN_BUGSNAG_ANDROID_VERSION }
@@ -56,37 +66,13 @@ abstract class NdkToolchain {
             legacyUploadRequired = false
         }
 
-        if (!useLegacyNdkSymbolUpload.get() && legacyUploadRequired) {
-            throw StopExecutionException(
-                "Your Bugsnag SDK configured for variant ${variantName.get()} does not support the new NDK " +
-                    "symbols upload mechanism. Please set legacyNDKSymbolsUpload or upgrade your " +
-                    "Bugsnag SDK. See https://docs.bugsnag.com/api/ndk-symbol-mapping-upload/ for details."
-            )
-        }
-
-        // useLegacyNdkSymbolUpload force overrides any defaults or options
-        if (useLegacyNdkSymbolUpload.get()) {
-            return MappingTool.OBJDUMP
-        }
-
         val ndkVersion = version.get()
         return when {
-            ndkVersion >= MIN_NDK_OBJCOPY_VERSION -> MappingTool.OBJCOPY
+            !legacyUploadRequired && ndkVersion >= MIN_NDK_OBJCOPY_VERSION -> MappingTool.OBJCOPY
             else -> MappingTool.OBJDUMP
         }
     }
 
-    /**
-     * Set all the fields of this `NdkToolchain` based on the given [other] `NdkToolchain`
-     */
-    fun configureWith(other: NdkToolchain) {
-        baseDir.set(other.baseDir)
-        overrides.set(other.overrides)
-        useLegacyNdkSymbolUpload.set(other.useLegacyNdkSymbolUpload)
-        bugsnagNdkVersion.set(other.bugsnagNdkVersion)
-        variantName.set(other.variantName)
-    }
-
     private fun executableName(cmdName: String): String {
         return if (osName?.startsWith("windows") == true) "$cmdName.exe" else cmdName
     }
@@ -174,7 +160,7 @@ abstract class NdkToolchain {
             bugsnag: BugsnagPluginExtension,
             variant: ApkVariant
         ): NdkToolchain {
-            val useLegacyNdkSymbolUpload = bugsnag.useLegacyNdkSymbolUpload.get()
+            val useLegacyNdkSymbolUpload = bugsnag.useLegacyNdkSymbolUpload.orNull
             val overrides = bugsnag.objdumpPaths.map { it.mapKeys { (abi, _) -> Abi.findByName(abi)!! } }
 
             val ndkToolchain = project.objects.newInstance<NdkToolchain>()
@@ -183,8 +169,13 @@ abstract class NdkToolchain {
             ndkToolchain.overrides.set(overrides)
 
             // we disable the bugsnag-android version check if Unity is enabled otherwise we end up with mutation errors
-            if (!BugsnagGenerateUnitySoMappingTask
-                .isUnityLibraryUploadEnabled(bugsnag, project.extensions.findByType(BaseExtension::class.java)!!)
+            // we also disable the check if 'useLegacyNdkSymbolUpload' has been set
+            if (
+                useLegacyNdkSymbolUpload == null ||
+                !BugsnagGenerateUnitySoMappingTask.isUnityLibraryUploadEnabled(
+                    bugsnag,
+                    project.extensions.findByType(BaseExtension::class.java)!!
+                )
             ) {
                 ndkToolchain.bugsnagNdkVersion.set(project.provider { getBugsnagAndroidNDKVersion(variant) })
             }
@@ -195,5 +186,9 @@ abstract class NdkToolchain {
     }
 }
 
-private val NdkToolchain.version get() = baseDir.map { VersionNumber.parse(it.asFile.name) }
+private val NdkToolchain.version: Provider<VersionNumber>
+    get() = baseDir.map {
+        NdkPackageXmlParser.loadVersionFromPackageXml(it.asFile)
+    }
+
 private val NdkToolchain.isLLVMPreferred get() = version.map { it >= NdkToolchain.MIN_NDK_LLVM_VERSION }

src/test/kotlin/com/bugsnag/android/gradle/PluginExtensionTest.kt

@@ -75,6 +75,7 @@ class PluginExtensionTest {
             assertNull(sourceControl.revision.orNull)
             assertNull(sourceControl.provider.orNull)
             assertNull(nodeModulesDir.orNull)
+            assertNull(useLegacyNdkSymbolUpload.orNull)
         }
 
         // ndk/unity upload defaults to false
@@ -112,6 +113,7 @@ class PluginExtensionTest {
             objdumpPaths.set(mapOf(Pair("armeabi-v7a", "/test/foo")))
             sharedObjectPaths.set(listOf(File("/test/bar")))
             nodeModulesDir.set(File("/test/foo/node_modules"))
+            useLegacyNdkSymbolUpload.set(true)
 
             sourceControl.repository.set("https://github.com")
             sourceControl.revision.set("d0e98fc")
@@ -136,6 +138,8 @@ class PluginExtensionTest {
             assertEquals(mapOf(Pair("armeabi-v7a", "/test/foo")), objdumpPaths.get())
             assertEquals(listOf(File("/test/bar")), sharedObjectPaths.get())
             assertEquals(File("/test/foo/node_modules"), nodeModulesDir.get())
+            assertTrue(useLegacyNdkSymbolUpload.get())
+
             assertEquals("https://github.com", sourceControl.repository.get())
             assertEquals("d0e98fc", sourceControl.revision.get())
             assertEquals("github", sourceControl.provider.get())

src/test/kotlin/com/bugsnag/android/gradle/internal/NdkToolchainTest.kt

@@ -7,6 +7,7 @@ import org.gradle.api.file.DirectoryProperty
 import org.gradle.api.internal.provider.DefaultProvider
 import org.gradle.api.provider.MapProperty
 import org.gradle.api.provider.Property
+import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotEquals
 import org.junit.Assert.assertTrue
@@ -17,21 +18,31 @@ import java.io.File
 import org.mockito.Mockito.`when` as whenMock
 
 class NdkToolchainTest {
+    private var ndkDir: File? = null
+
+    @After
+    fun cleanupNdkDir() {
+        ndkDir?.deleteRecursively()
+    }
+
     @Test
     fun ndk19() {
-        val toolchain = TestNdkToolchainImpl(File("/19.2.5345600/"), true)
+        val ndkDir = setupFakeNdk(19, 2, 5345600)
+        val toolchain = TestNdkToolchainImpl(ndkDir, true)
         assertEquals(NdkToolchain.MappingTool.OBJDUMP, toolchain.preferredMappingTool())
     }
 
     @Test
     fun ndk21Legacy() {
-        val toolchain = TestNdkToolchainImpl(File("/21.1.6352462/"), true)
+        val ndkDir = setupFakeNdk(21, 1, 6352462)
+        val toolchain = TestNdkToolchainImpl(ndkDir, true)
         assertEquals(NdkToolchain.MappingTool.OBJDUMP, toolchain.preferredMappingTool())
     }
 
     @Test
     fun ndk21() {
-        val toolchain = TestNdkToolchainImpl(File("/21.1.6352462/"), false)
+        val ndkDir = setupFakeNdk(21, 1, 6352462)
+        val toolchain = TestNdkToolchainImpl(ndkDir, false)
         assertEquals(NdkToolchain.MappingTool.OBJCOPY, toolchain.preferredMappingTool())
 
         val objcopyPath = toolchain.objcopyForAbi(Abi.ARM64_V8A).toString()
@@ -43,7 +54,8 @@ class NdkToolchainTest {
 
     @Test
     fun ndk23() {
-        val toolchain = TestNdkToolchainImpl(File("/23.0.7599858/"), false)
+        val ndkDir = setupFakeNdk(23, 0, 7599858)
+        val toolchain = TestNdkToolchainImpl(ndkDir, false)
         assertEquals(NdkToolchain.MappingTool.OBJCOPY, toolchain.preferredMappingTool())
 
         val objcopyPath = toolchain.objcopyForAbi(Abi.ARM64_V8A).toString()
@@ -55,8 +67,9 @@ class NdkToolchainTest {
 
     @Test
     fun objcopyOverrides() {
+        val ndkDir = setupFakeNdk(23, 0, 7599858)
         val toolchain = TestNdkToolchainImpl(
-            File("/23.0.7599858/"), false, mapOf(Abi.ARM64_V8A to "arm64-objcopy")
+            ndkDir, false, mapOf(Abi.ARM64_V8A to "arm64-objcopy")
         )
 
         assertEquals(NdkToolchain.MappingTool.OBJCOPY, toolchain.preferredMappingTool())
@@ -69,8 +82,9 @@ class NdkToolchainTest {
 
     @Test
     fun objdumpOverrides() {
+        val ndkDir = setupFakeNdk(19, 2, 5345600)
         val toolchain = TestNdkToolchainImpl(
-            File("/19.2.5345600/"), true, mapOf(Abi.ARM64_V8A to "arm64-objcopy")
+            ndkDir, true, mapOf(Abi.ARM64_V8A to "arm64-objcopy")
         )
 
         assertEquals(NdkToolchain.MappingTool.OBJDUMP, toolchain.preferredMappingTool())
@@ -80,11 +94,35 @@ class NdkToolchainTest {
         assertNotEquals("arm64-objcopy", toolchain.objdumpForAbi(Abi.X86_64).toString())
         assertNotEquals("arm64-objcopy", toolchain.objdumpForAbi(Abi.ARMEABI).toString())
     }
+
+    fun setupFakeNdk(major: Int, minor: Int, micro: Int): File {
+        val newNdkDir = File(System.getProperty("java.io.tmpdir"), "ndk$major.$minor.$micro")
+        newNdkDir.mkdirs()
+
+        val packageXml = File(newNdkDir, "package.xml")
+        packageXml.writeText(
+            """
+            <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+            <ns2:repository xmlns:ns2="http://schemas.android.com/repository/android/common/02" xmlns:ns3="http://schemas.android.com/repository/android/common/01" xmlns:ns4="http://schemas.android.com/repository/android/generic/01" xmlns:ns5="http://schemas.android.com/repository/android/generic/02" xmlns:ns6="http://schemas.android.com/sdk/android/repo/addon2/01" xmlns:ns7="http://schemas.android.com/sdk/android/repo/addon2/02" xmlns:ns8="http://schemas.android.com/sdk/android/repo/repository2/01" xmlns:ns9="http://schemas.android.com/sdk/android/repo/repository2/02" xmlns:ns10="http://schemas.android.com/sdk/android/repo/sys-img2/02" xmlns:ns11="http://schemas.android.com/sdk/android/repo/sys-img2/01">
+            <license id="android-sdk-license" type="text"/>
+            <localPackage path="ndk;23.1.7779620" obsolete="false"><type-details xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="ns5:genericDetailsType"/>
+                <revision><major>$major</major><minor>$minor</minor><micro>$micro</micro></revision>
+                <display-name>NDK (Side by side) $major.$minor.$micro</display-name>
+                <uses-license ref="android-sdk-license"/>
+                <dependencies><dependency path="patcher;v4"/></dependencies>
+            </localPackage>
+            </ns2:repository>
+            """.trimIndent()
+        )
+
+        ndkDir = newNdkDir
+        return newNdkDir
+    }
 }
 
 private class TestNdkToolchainImpl(
     baseDir: File,
-    useLegacyNdkSymbolUpload: Boolean,
+    useLegacyNdkSymbolUpload: Boolean?,
     overrides: Map<Abi, String> = emptyMap()
 ) : NdkToolchain() {
     override val baseDir: DirectoryProperty = Mockito.mock(DirectoryProperty::class.java)
@@ -104,7 +142,7 @@ private class TestNdkToolchainImpl(
                     (it.arguments.first() as Transformer<out Any, in Directory>).transform(baseDirectory)
                 }
             }
-        whenMock(this.useLegacyNdkSymbolUpload.get()).thenReturn(useLegacyNdkSymbolUpload)
+        whenMock(this.useLegacyNdkSymbolUpload.orNull).thenReturn(useLegacyNdkSymbolUpload)
         whenMock(this.overrides.get()).thenReturn(overrides)
     }
 }