megazord: changelog, address comments from review

CHANGES_UNRELEASED.md

@@ -3,3 +3,38 @@
 # Unreleased Changes
 
 [Full Changelog](https://github.com/mozilla/application-services/compare/v0.34.0...master)
+
+# General
+
+## Megazords
+
+The long-awaited android megazord changes have arrived. This has a large number
+of changes, many of them breaking:
+([#1103](https://github.com/mozilla/application-services/pull/1103))
+
+- Consumers who depend on network features of application-services, but
+  which were not using a megazord, will no longer be able to use a legacy
+  HTTP stack by default.
+
+- Consumers who depend on network features and *do* use a megazord, can no
+  longer initialize HTTP in the same call as the megazord.
+
+- Both of these cases should import the `org.mozilla.appservices:httpconfig`
+  package, and call `RustHttpConfig.setClient(lazy { /* client to use */ })`
+  before calling functions which make HTTP requests.
+
+- For custom megazord users, the name of your megazord is now always
+  `mozilla.appservices.Megazord`. You no longer need to load it by reflection,
+  since the swapped-out version always has the same name as your custom version.
+
+- The reference-browser megazord has effectively been replaced by the
+  full-megazord, which is also the megazord used by default
+
+- The steps to swap-out a custom megazord have changed. The specific steps are
+  slightly different in various cases, and we will file PRs to help make the
+  transition.
+
+- Substitution builds once again work, except for running unit tests against
+  Rust code.
+
+

build.gradle

@@ -4,6 +4,7 @@ buildscript {
     ext.kotlin_version = '1.3.10'
     ext.android_components_version = '0.50.0'
     ext.jna_version = '5.2.0'
+    ext.android_gradle_plugin_version = '3.3.2'
 
     ext.build = [
         compileSdkVersion: 27,
@@ -19,7 +20,7 @@ buildscript {
         }
     }
     dependencies {
-        classpath "com.android.tools.build:gradle:3.3.2"
+        classpath "com.android.tools.build:gradle:$android_gradle_plugin_version"
         classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
 
         // Publish.

components/fxa-client/android/build.gradle

@@ -24,6 +24,9 @@ android {
 
     sourceSets {
         test.resources.srcDirs += "$buildDir/rustJniLibs/desktop"
+        // Add the full-megazord's build directory to our resource path so that
+        // we can actually find it during tests. (Unfortunately, each project
+        // has their own build dir)
         test.resources.srcDirs += "${project(':full-megazord').buildDir}/rustJniLibs/desktop"
 
         main {

components/logins/android/build.gradle

@@ -23,6 +23,9 @@ android {
 
     sourceSets {
         test.resources.srcDirs += "$buildDir/rustJniLibs/desktop"
+        // Add the full-megazord's build directory to our resource path so that
+        // we can actually find it during tests. (Unfortunately, each project
+        // has their own build dir)
         test.resources.srcDirs += "${project(':full-megazord').buildDir}/rustJniLibs/desktop"
     }
 }
@@ -46,7 +49,7 @@ configurations {
 
 dependencies {
     // Part of the public API.
-    api project(':sync15-library')
+    api project(':sync15')
 
     jnaForTest "net.java.dev.jna:jna:$jna_version@jar"
     implementation "net.java.dev.jna:jna:$jna_version@aar"

components/push/android/build.gradle

@@ -23,6 +23,9 @@ android {
 
     sourceSets {
         test.resources.srcDirs += "$buildDir/rustJniLibs/desktop"
+        // Add the full-megazord's build directory to our resource path so that
+        // we can actually find it during tests. (Unfortunately, each project
+        // has their own build dir)
         test.resources.srcDirs += "${project(':full-megazord').buildDir}/rustJniLibs/desktop"
     }
 }

components/rc_log/android/build.gradle

@@ -23,6 +23,9 @@ android {
 
     sourceSets {
         test.resources.srcDirs += "$buildDir/rustJniLibs/desktop"
+        // Add the full-megazord's build directory to our resource path so that
+        // we can actually find it during tests. (Unfortunately, each project
+        // has their own build dir)
         test.resources.srcDirs += "${project(':full-megazord').buildDir}/rustJniLibs/desktop"
     }
 }

components/rc_log/android/src/test/java/mozilla/appservices/rustlog/LogTest.kt

@@ -117,8 +117,11 @@ class LogTest {
         assertEquals(logs.size, 6)
 
         // We called `enable` again, so we expect to have used another thread
-        // XXX why is this 1?
-        // assertEquals(threadIds.size, 2)
+
+        // TODO: changing to indirect binding has chnged how JNA allocates threads
+        // for our callbacks, and has this next line fail. We should change it back
+        // once things are back to normal. Ditto for commented out lines below labeled
+        // assertEquals(threadIds.size, 2) // INDIRECT
 
         RustLogAdapter.disable()
 
@@ -136,8 +139,7 @@ class LogTest {
         assert(!RustLogAdapter.isEnabled)
 
         // new log callback, new thread.
-        // XXX why is this 1?
-        // assertEquals(threadIds.size, 3)
+        // assertEquals(threadIds.size, 3) // INDIRECT
 
         // Check behavior of 'disable by throw'
         RustLogAdapter.enable { level, tagStr, msgStr ->
@@ -156,8 +158,7 @@ class LogTest {
         assert(!RustLogAdapter.isEnabled)
 
         // new log callback, new thread.
-        // XXX why is this 1?
-        // assertEquals(threadIds.size, 4)
+        // assertEquals(threadIds.size, 4) // INDIRECT
 
         // Clean up
         RustLogAdapter.disable()
@@ -167,7 +168,7 @@ class LogTest {
             Thread.sleep(10)
             System.gc()
         }
-        // XXX this should be 0
+        // assertEquals(threads.size, 0) // INDIRECT
         assertEquals(threads.size, 1)
     }
 }

components/support/android/README.md

@@ -0,0 +1,10 @@
+# The `native_support` kotlin library.
+
+This essentially is where we put shared android code which meets both of the following requirements:
+
+- We'd like to reuse it in multiple projects.
+- It only is sensible for inclusion in packages that already have a transitive dependency on JNA.
+
+This is a bit subtle, but is mostly done to avoid additional special cases we'd need in the publish pipeline to handle this case sanely. (It also avoids adding that dependency if it's not needed).
+
+In particular, this means pure-kotlin modules (sync15, for example), should not depend on this.

components/support/android/src/main/java/mozilla/appservices/support/native/Helpers.kt

@@ -93,7 +93,7 @@ class MultipleMegazordsPresent(
 
 internal const val FULL_MEGAZORD_LIBRARY: String = "megazord"
 
-internal fun doMegazordCheck(componentName: String, componentVersion: String): String {
+internal fun lookupMegazordLibrary(componentName: String, componentVersion: String): String {
     val mzLibrary = System.getProperty("mozilla.appservices.megazord.library")
     Log.d("RustNativeSupport", "lib configured: ${mzLibrary ?: "none"}")
     if (mzLibrary == null) {
@@ -135,11 +135,11 @@ internal fun doMegazordCheck(componentName: String, componentVersion: String): S
  * It should not be called by consumers.
  */
 @Synchronized
-fun megazordCheck(componentName: String, componentVersion: String): String {
-    Log.d("RustNativeSupport", "megazordCheck($componentName, $componentVersion")
+fun findMegazordLibraryName(componentName: String, componentVersion: String): String {
+    Log.d("RustNativeSupport", "findMegazordLibraryName($componentName, $componentVersion")
     val mzLibraryUsed = System.getProperty("mozilla.appservices.megazord.library.used")
     Log.d("RustNativeSupport", "lib in use: ${mzLibraryUsed ?: "none"}")
-    val mzLibraryDetermined = doMegazordCheck(componentName, componentVersion)
+    val mzLibraryDetermined = lookupMegazordLibrary(componentName, componentVersion)
     Log.d("RustNativeSupport", "settled on $mzLibraryDetermined")
 
     // If we've already initialized the megazord, that means we've probably already loaded bindings
@@ -163,7 +163,9 @@ fun megazordCheck(componentName: String, componentVersion: String): String {
 }
 
 /**
- * Contains all the boilerplate for loading a
+ * Contains all the boilerplate for loading a library binding from the megazord,
+ * locating it if necessary, safety-checking versions, and setting up a fallback
+ * if loading fails.
  *
  * Indirect as in, we aren't using JNA direct mapping. Eventually we'd
  * like to (it's faster), but that's a problem for another day.
@@ -172,7 +174,7 @@ inline fun <reified Lib : Library> loadIndirect(
     componentName: String,
     componentVersion: String
 ): Lib {
-    val mzLibrary = megazordCheck(componentName, componentVersion)
+    val mzLibrary = findMegazordLibraryName(componentName, componentVersion)
     return try {
         Native.load<Lib>(mzLibrary, Lib::class.java)
     } catch (e: UnsatisfiedLinkError) {

components/sync15/android/src/main/java/mozilla/appservices/sync15/SyncTelemetryPing.kt

@@ -7,7 +7,6 @@ package mozilla.appservices.sync15
 import org.json.JSONArray
 import org.json.JSONException
 import org.json.JSONObject
-import org.json.JSONException
 
 /**
  * This file defines Kotlin data classes for unpacking the Sync telemetry ping,

components/viaduct/android/build.gradle

@@ -24,6 +24,10 @@ android {
 
     sourceSets {
         test.resources.srcDirs += "$buildDir/rustJniLibs/desktop"
+
+        // Add the full-megazord's build directory to our resource path so that
+        // we can actually find it during tests. (Unfortunately, each project
+        // has their own build dir)
         test.resources.srcDirs += "${project(':full-megazord').buildDir}/rustJniLibs/desktop"
 
         main {

megazords/fenix/android/build.gradle

@@ -33,7 +33,6 @@ android {
 afterEvaluate {
     android.sourceSets.debug.jniLibs.srcDirs = android.sourceSets.main.jniLibs.srcDirs
     android.sourceSets.release.jniLibs.srcDirs = android.sourceSets.main.jniLibs.srcDirs
-//    android.sourceSets.main.jniLibs.srcDirs = []
 }
 
 configurations {

megazords/full/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "megazord"
 version = "0.1.0"
-authors = ["Thom Chiovoloni <tchiovoloni@mozilla.com>"]
+authors = ["application-services <application-services@mozilla.com>"]
 edition = "2018"
 license = "MPL-2.0"
 

megazords/full/android/build.gradle

@@ -33,7 +33,6 @@ android {
 afterEvaluate {
     android.sourceSets.debug.jniLibs.srcDirs = android.sourceSets.main.jniLibs.srcDirs
     android.sourceSets.release.jniLibs.srcDirs = android.sourceSets.main.jniLibs.srcDirs
-//    android.sourceSets.main.jniLibs.srcDirs = []
 }
 
 configurations {

megazords/full/src/lib.rs

@@ -20,12 +20,13 @@ pub use viaduct;
 /// optional for the default (full) megazord.
 ///
 /// This function exists so that the `native_support` code can ensure that we
-/// still check tthat the version of the functions in the megazord library and
+/// still check that the version of the functions in the megazord library and
 /// the version of the code loading them is identical.
 ///
 /// Critically, that means this function (unlike our other functions) must be
 /// ABI stable! It needs to take no arguments, and return either null, or a
-/// NUL-terminated C string.
+/// NUL-terminated C string. Failure to do this will result in memory unsafety
+/// when an old version of the megazord loader loads a newer library!
 ///
 /// If we ever need to change that (which seems unlikely, since we could encode
 /// whatever we want in a string if it came to it), we must change the functions

publish.gradle

@@ -17,7 +17,7 @@ def libVcsUrl = properties.libVcsUrl
 // with.
 //
 // It's only used for megazords, for which it's required. Passing it in for a
-// non-megazord is allowed, but will trigger a warning, but is allowed.
+// non-megazord is allowed, but will trigger a warning.
 ext.configurePublish = { jnaForTestConfiguration = null ->
     def theGroupId = rootProject.ext.library.groupId
     def theArtifactId = project.ext.artifactId