Initial KMP Restructure #15

frenziedherring · 2022-05-15T01:35:01Z

Updated src into source sets for common/jvm/js/android/native
Moved JVM code to JVM source set
Extracted platform dependent pieces into expect/actual
Implemented required 'actual's for JS & Native
Moved AndroidLogcatLogger to android source set

- Updated src into source sets for common/jvm/js/android/native - Moved JVM code to JVM source set - Extracted platform dependent pieces into expect/actual - Implemented required 'actual's for JS & Native - Moved AndroidLogcatLogger to android source set

pyricau · 2022-05-18T19:43:50Z

Thanks a lot, this is really cool!

pyricau · 2022-05-18T19:44:36Z

gradle.properties

+POM_LICENCE_DIST=repo
+
+POM_DEVELOPER_ID=square
+POM_DEVELOPER_NAME=Square, Inc.


I'm a bit surprised you had to add all this, we've published releases just fine in the past, what's changed?

I used a skeleton KMP project to help get the Android source setup with the build files - this must have come over with it. I'll remove these extra lines.

I have no experience publishing an AAR, so I'll have to dig in to learn about the publishing process.

These have been removed and I was able to run the Android build and sample project on the emulator. The publishing plugin was re-added when I restructured in 3c402d1 so I think the AAR publishing should be OK now

build.gradle.kts

gradle/wrapper/gradle-wrapper.properties

pyricau · 2022-05-18T19:46:29Z

kotlin-js-store/yarn.lock

@@ -0,0 +1,3108 @@
+git add src comm
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1


this file should not be committed, right?

I've always committed the lock files that are generated from package managers in the past (package-lock.json on my past NPM projects) to ensure the dependencies are specific.

That being said - how did that stray git command get in there? 🤦 At a minimum I'll fix that, but definitely can remove this file if you think it's not entirely necessary

Check what other Square KMP projects do

pyricau · 2022-05-18T19:47:48Z

src/androidMain/kotlin/logcat/Throwables.kt

-  printStackTrace(printWriter)
-  printWriter.flush()
-  return stringWriter.toString()
+actual fun Throwable.asLog(): String {


not super familiar with KMP: why is this in androidMain? This should work fine elsewhere, no?

I wanted to build out hierarchical source sets so that the Android source could leverage the JVM implementation since It should definitely work in the JVM. After digging into https://kotlinlang.org/docs/multiplatform-share-on-platforms.html, It looks like that isn't currently supported:

(https://kotlinlang.org/docs/multiplatform-share-on-platforms.html#configure-the-hierarchical-structure-manually)

You can have a shared source set for the following combinations of targets:

JVM + JS + Native

JVM + Native

JS + Native

JVM + JS

Native

Kotlin doesn't currently support sharing a source set for these combinations:

Several JVM targets

JVM + Android targets

Several JS targets

huh that's sad.

Probably the saddest part of this whole effort - hopefully in the future the JVM + Android can be shared

pyricau · 2022-05-18T19:48:19Z

src/androidTest/kotlin/LogcatTest.kt


 class LogcatTest {

-  @After
+  @AfterTest


Interesting, why migrate from junit to kotlin test? Is kotlin test able to run on multiple platforms?

Exactly. In fact, I discovered that the @kotlin.test.AfterTest is just a typealias to @org.junit.After for JVM

package kotlin.test public actual typealias Test = org.junit.Test public actual typealias Ignore = org.junit.Ignore public actual typealias BeforeTest = org.junit.Before public actual typealias AfterTest = org.junit.After

But using that allowed me to port the tests to JS and Native (which was super helpful in figuring out the bit that were different between the Android implementation and other platforms, especially with stack traces, Companion objects and frozen references for native, to fill in the expect/actual. Running the Gradle check target executes each of the platform's tests.

pyricau · 2022-05-18T19:49:16Z

src/commonMain/kotlin/logcat/LogcatLogger.kt

+  )
+
+  companion object {
+    // For expect functions above


above => below?

Why do these have to be defined below?

Yeah, that should be "below". They're defined as extensions on the Companion so that they can have the expect modifier. I would have preferred them inline, but including them in the Companion directly as expect complains that there's no backing field or delegate.

Modifier 'expect' is not applicable to 'member property without backing field or delegate'

I will definitely re-arrange to better co-locate the expected extensions in the file though

Rearranged to help with code readability

pyricau · 2022-05-18T19:50:06Z

src/jvmTest/kotlin/logcat/LogcatTest.kt

+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+class LogcatTest {


Why is this test duplicated in N places? Feels like the test should only be written once.

Agreed, there's some work I can to to collapse tests which don't have platform specific requirements. Primarily the ones that differ the most are the tests for Throwable, and the native tests require a TestLogger with AtomicReferences to prevent Immutability exceptions on non-frozen references.

I'll take another pass on the test sections to clean them up.

As much as possible, commonTest includes test that can run on all platforms. They are executed once for each of the platforms (JVM/JS/Native) using the same test.

- Removed commented lines from build.gradle.kts - Removed extra lines from gradle.properties - Changed Gradle back to 7.2 in gradle-wrapper.properties - Fixed yarn.lock - Reformatted code to match style - Restructured Tests - Use actual/expect to inject ITestLogcatLogger - Pull platform-agnostic tests to CommonLogcatTest.kt - Put platform-specific tests in [Platform]LogcatTest.kt

frenziedherring · 2022-05-23T02:41:20Z

@pyricau - I updated my branch based on the comments above. The biggest change was updating the test structure to prevent duplicating tests in N platforms. The common tests now leverage expect/actual to get an instance of the test logger for the platform, leaving the rest of the test intact. Let me know what you think about that. Thanks for the review!

- Moved src under /logcat - Updated .api files - Updated Gradle plugin to 7.0.0 - Removed versionName and versionCode from android block in build.gradle.kts - Re-added the publishing and KtLint checks - Fixed lint errors - Removed jcenter repo

- Clean up build.gradle.kts

- Put Coroutines dependency into Dependencies.kt - Limit coroutines to only commonTest dependency set - Downgraded errant Coroutines 1.6 to 1.5.2 - Allow Kotlin 1.5.31 and Java 8 to pass build checks - Removed test case with multiple nested Companion functions - The use case was very edge case, and the multiple nesting was making JS Throwable fallback fail. Single companion function call (no nesting) still passes though

frenziedherring · 2022-06-15T01:24:52Z

@pyricau - I downgraded some dependencies (and fixed tests that were failing) that should address Gradle failures in https://github.com/square/logcat/runs/6610273912?check_suite_focus=true. I downgraded the coroutines dependency to 1.5.2 which allowed going back to Kotlin 1.5.30 and Java 8. It's also only needed in commonTest.

swankjesse · 2022-06-28T11:05:59Z

gradle.properties

@@ -1,3 +1,7 @@
+kotlin.code.style=official


Seems off-topic.

swankjesse · 2022-06-28T11:06:48Z

gradlew

@@ -1,7 +1,7 @@
-#!/bin/sh


What's with the empty wrapper jar?!

I don't know why it says it's empty - the file is about 58k on my branch (https://github.com/frenziedherring/logcat/blob/frenziedherring/logcatx/gradle/wrapper/gradle-wrapper.jar)

logcat/build.gradle.kts

swankjesse · 2022-06-28T11:10:50Z

logcat/build.gradle.kts

+  val nativeTarget = when {
+    hostOs == "Mac OS X" -> macosX64("native")
+    hostOs == "Linux" -> linuxX64("native")
+    isMingwX64 -> mingwX64("native")


Typically we include all native targets. Also this is missing many important targets like ARM Macs and iOS.

I have added more targets that will come in the next push. I created the nativeMain and nativeTest source sets and made the other targets dependOn them. I only have a Mac, so I could not build some of the targets

logcat/gradle.properties

logcat/src/commonMain/kotlin/logcat/defaultThrowableAsLog.kt

logcat/src/commonTest/kotlin/logcat/ITestLogcatLogger.kt

swankjesse · 2022-06-28T11:22:17Z

logcat/src/jsTest/kotlin/logcat/JsLogcatTest.kt

+
+    val stackTraceStr = """
+      |RuntimeException: damn
+      |    at JsLogcatTest.Throwable_asLogMessage_has_stacktrace_logged


Is this dependent on the JS engine?

Appears to be part of the Kotlin stdlib for JS, but does use the asDynamic().stack which could be JS engine specific. FWIW, the test did pass when I added nodejs in addition to browser as targets in the build.gradle. I'll have to find time to dig in more to find more of an answer.
https://github.com/JetBrains/kotlin/tree/d8b0dbe71bbeb84ba0fbdac87801c8c6f4eed4c9/libraries/stdlib/js/src/kotlin/throwableExtensions.kt#L18 uses ExceptionTraceBuilder.dumpFullTrace/dumpSelfTrace.

Looks like the .stack is dependent on JS engine:

v8 - https://v8.dev/docs/stack-trace-api, https://nodejs.org/api/errors.html#errorstack
mdn - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack#examples

swankjesse · 2022-06-28T11:22:55Z

logcat/src/jvmMain/kotlin/logcat/Throwables.kt

+ * - No silent swallowing of UnknownHostException.
+ * - The buffer size is 256 bytes instead of the default 16 bytes.
+ */
+actual fun Throwable.asLog(): String {


Seems duplicated?

Consider a jniMain that has Android and JVM sources.

I was able to make androidMain depend on jvmMain and remove the duplication (even though based on #15 (comment), it looked like sharing Android + JVM wasn't supported), and that appears to build & pass tests. The IDE is showing unresolved references, though, so I'm working through that right now

swankjesse · 2022-06-28T11:25:11Z

logcat/src/nativeTest/kotlin/logcat/NativeLogcatTest.kt

+
+    val stackTraceStr = """
+      |kotlin.RuntimeException: damn
+      |    at 0


Can we do better than this?

kpgalligan · 2022-06-28T12:54:16Z

logcat/src/nativeMain/kotlin/logcat/extLogcatLogger.kt

+  get() = loggerReference.value
+
+actual fun LogcatLogger.Companion.install(logger: LogcatLogger) {
+  logger.freeze()


At this point, freezing should be optional and controlled by the runtime memory setting

Is this referring to the new memory management for Kotlin/Native? Is there a way to check the memory setting at runtime to optionally freeze the logger?

kpgalligan · 2022-06-28T13:27:16Z

logcat/src/nativeMain/kotlin/logcat/outerClassSimpleNameInternalOnlyDoNotUseKThxBye.kt

+  return if (this.isNullOrEmpty() || this == "Companion") null else this
+}
+
+actual fun Any.outerClassSimpleNameInternalOnlyDoNotUseKThxBye(): String {


Should do some visibility review. This gets exposed to the framework header, I'd assume unnecessarily

__attribute__((objc_subclassing_restricted)) __attribute__((swift_name("OuterClassSimpleNameInternalOnlyDoNotUseKThxByeKt"))) @interface LogcatOuterClassSimpleNameInternalOnlyDoNotUseKThxByeKt : LogcatBase + (NSString *)outerClassSimpleNameInternalOnlyDoNotUseKThxBye:(id)receiver __attribute__((swift_name("outerClassSimpleNameInternalOnlyDoNotUseKThxBye(_:)"))); @end;

Updated that particular function to be @PublishedApi internal actual. I don't see it showing up in my local build's framework header now, as far as I can tell (I'm not an iOS developer; primarily Android)

kpgalligan · 2022-06-28T13:29:30Z

logcat/src/nativeMain/kotlin/logcat/extLogcatLogger.kt

+
+actual fun LogcatLogger.Companion.install(logger: LogcatLogger) {
+  logger.freeze()
+  loggerReference.compareAndSet(NoLog, logger).also { updated ->


This might be a dumb idea, but it's been on my mind forever with Kermit and others. You can't really reset the logger, or generally speaking, you don't have any use case that would need it. So, if you could configure the logger up front, possibly at build time (dev vs release), you could avoid every logging call going through an atomic reference. All global reference loggers have the same issue, and it seems avoidable.

Interesting. Are you thinking that you'd swap an impl JAR, i.e. both jars have the same class, one is a no-op and the other the real impl?

Or something else?

kpgalligan · 2022-06-28T13:42:08Z

logcat/src/commonMain/kotlin/logcat/PrintLogger.kt

@@ -3,11 +3,15 @@ package logcat
 /**
 * A [LogcatLogger] that always logs and delegates to [println] concatenating
 * the tag and message, separated by a space. Alternative to [AndroidLogcatLogger]
- * when running on a JVM.
+ * when running on a JVM/JS/Native.
 */
 object PrintLogger : LogcatLogger {


It looks like the "other platforms" only get println. Consider iOS and JS specific loggers (touchlab/Kermit#183, https://github.com/touchlab/Kermit/blob/main/kermit/src/jsMain/kotlin/co/touchlab/kermit/ConsoleWriter.kt).

Lots of good KMP stuff to learn from Kermit, for sure. Unfortunately a few things I wish were different (lack of auto tag and requiring devs to make a decision about severity instead of making that optional. See here https://github.com/square/logcat#motivations)

Well, I mean, I'm not saying "use Kermit", I'm just saying if it's multiplatform, I'd add some platform-specific loggers. The first link is to a thread where we took inspiration from Napier, which is another KMP logger. I'll definitely take a look at the motivations, as we're sort of redesigning Kermit right now. Tagging is tricky, for sure.

Certainly seems like we could make a JS specific logger that uses the console API. I'll give it a try.

- Create source set hierarchy - Removed println & printStackTrace - Made outerClassSimpleNameInternalOnlyDoNotUseKThxBye.kt internal with @PublishedApi across `actual`s to keep it out of headers - androidMain depends on jvmMain, removing duplication

frenziedherring added 2 commits May 14, 2022 21:03

Initial KMP Restructure

3024247

- Updated src into source sets for common/jvm/js/android/native - Moved JVM code to JVM source set - Extracted platform dependent pieces into expect/actual - Implemented required 'actual's for JS & Native - Moved AndroidLogcatLogger to android source set

Added missing gradle.properties & added trailing file newlines

2be2e89

frenziedherring requested a review from pyricau as a code owner May 15, 2022 01:35

pyricau reviewed May 18, 2022

View reviewed changes

build.gradle.kts Outdated Show resolved Hide resolved

pyricau reviewed May 18, 2022

View reviewed changes

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved

pyricau reviewed May 18, 2022

View reviewed changes

frenziedherring marked this pull request as draft May 25, 2022 00:42

frenziedherring added 3 commits May 24, 2022 22:38

Fixed build scripts

3c402d1

- Moved src under /logcat - Updated .api files - Updated Gradle plugin to 7.0.0 - Removed versionName and versionCode from android block in build.gradle.kts - Re-added the publishing and KtLint checks - Fixed lint errors - Removed jcenter repo

- Move dependencies to Dependencies.kt

164953f

- Clean up build.gradle.kts

frenziedherring marked this pull request as ready for review June 15, 2022 03:28

swankjesse suggested changes Jun 28, 2022

View reviewed changes

kpgalligan reviewed Jun 28, 2022

View reviewed changes

frenziedherring added 5 commits July 7, 2022 21:58

- Add more build targets

956c29a

- Create source set hierarchy - Removed println & printStackTrace - Made outerClassSimpleNameInternalOnlyDoNotUseKThxBye.kt internal with @PublishedApi across `actual`s to keep it out of headers - androidMain depends on jvmMain, removing duplication

- Changed ITestLogcatLogger.kt to TestLogcatLogger.kt

27aaca0

- Removed unnecessary gradle.properties lines

f3ca17b

- Removed parens from test function name

18633d1

Fixed test assertions (function name changed in trace)

eabc219

Initial KMP Restructure #15

Are you sure you want to change the base?

Initial KMP Restructure #15

Conversation

frenziedherring commented May 15, 2022

pyricau commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyricau May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenziedherring commented May 23, 2022

frenziedherring commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenziedherring Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyricau May 18, 2022 •

edited

Loading

frenziedherring Jul 6, 2022 •

edited

Loading