Add `arrow-kt` example #3886

0xnm · 2024-11-01T21:57:48Z

This PR partially addresses #3670: it contains working example, but doesn't contain any docs.

This example contains compilation and test execution for all the modules of https://github.com/arrow-kt/arrow for the JVM and JS (Node) targets, except arrow-optics-compose module (it requires better Android support by Mill).

Full example run takes 6 minutes on my machine, so it is extracted into a dedicated CI job.

The following Gradle plugins are not added to the example build script, because their respective tooling is Gradle-dependent (no raw-jar or CLI):

https://github.com/xvik/gradle-animalsniffer-plugin (although maybe it is possible to use options from https://www.mojohaus.org/animal-sniffer/)
https://github.com/Kotlin/kotlinx-knit
https://github.com/Kotlin/binary-compatibility-validator
https://github.com/diffplug/spotless

There is also no publication support in this example (requires Kotlin Multiplatform publishing support, see #3867)

I faced the following issues while writing this example:

There is quite a lot of duplication for JVM/JS configuration, because there is no support of Kotlin Multiplatform hierarchies
Each Maven or Module dependency should be with JS/JVM/etc. target qualifier, which also quite explodes amount of code to be written. This is also a question of Kotlin Multiplatform hierarchies support and resolution.
Until KMP hierarchies and target resolution support is not implemented, adding a new compilation target will bring quite a lot of new code in the build script. And there is a lot of targets in Arrow: https://github.com/arrow-kt/arrow-gradle-config/blob/97ba7b5eab810a336cf4070eb717f05533d208a8/arrow-gradle-config-kotlin/src/main/kotlin/io.arrow-kt.arrow-gradle-config-kotlin.gradle.kts#L34-L69
There is no possibility to disable tests for the particular module (so I had to use a hack with overriding compile / test tasks) - this is because tests is dedicated module, but if comes as a part of the trait, we cannot add / remove it dynamically. Having such control is handy when certain multiplatform modules have a common trait for the main compilation unit, but some of them may have no tests.

Regarding docs: I think it is too early to write any comparison with Gradle, because clearly new functionality will be added to the Kotlin support in Mill which will affect overall execution time and also the comparison should be done not only for JVM targets, but for Kotlin/JS and Kotlin/Native targets as well. And ideally it should be a defined methodology for such testing (for example, certain Gradle plugins should be removed from the Arrow Gradle build script, to have the same set as Mill build script; compiler execution time should be excluded (it may be different because of the different flags passed to it under the hood)).

So I propose to open a dedicated bounty for writing such doc.

lihaoyi · 2024-11-01T22:31:34Z

Thanks, will take a look.

I think splitting the bounty between example and doc is fine. Having an example gives us something to start off that we can critique and improve, and already gives a "realistic" example we can include in the documentation for educational purposes. Having it improved enough to be a meaningful comparison can come in follow ups

lihaoyi · 2024-11-02T01:11:18Z

example/thirdparty/arrow/build.mill

@@ -0,0 +1,590 @@
+package build


CC @lefou @lolgab in case you guys want to review this as well. We've never had a realistic kotlin multiplatform build before so this will be the flagship example.

lihaoyi · 2024-11-02T02:07:25Z

@0xnm I pushed a small update to tidy it up a bit; tests remain passing on my laptop, hopefully will continue passing in CI.

I'll probably take a few more passes trying to tidy this up over the next few days

lihaoyi · 2024-11-02T06:36:42Z

I opened separate bounties specifically for Spotless and AnimalSniffer since those seem to keep coming up in our examples

knit and binary-compatibility-validator we can just punt on for now, unless there is repeated demand

lefou

Since KotlinModule mix-in at least JavaModule, task like languageVersion and apiVersion should be explicitly prefixing the language they refer to. Otherwise user may wonder which api is meant when they see that task. Is it Java API, or Test API or Kotlin API or domain API? There is also a risk of name clash.

kotlinlib/src/mill/kotlinlib/KotlinModule.scala

lefou · 2024-11-03T15:36:20Z

example/thirdparty/arrow/build.mill

+        override def compile: T[CompilationResult] =
+          if (!modulesWithTestingEnabled.contains(multiplatformRoot))
+            Task { CompilationResult(T.dest, PathRef(T.dest)) }
+          else super.compile
+        override def test(args: String*): Command[(String, Seq[TestResult])] =
+          if (!modulesWithTestingEnabled.contains(multiplatformRoot))
+            Task.Command { ("", Seq.empty[TestResult]) }
+          else super.test(args: _*)


These are a bit strange. Can we at least add a comment, what and more important why it is happening here?

lefou · 2024-11-03T15:36:44Z

example/thirdparty/arrow/build.mill

+        override def compile: T[CompilationResult] =
+          if (!modulesWithTestingEnabled.contains(multiplatformRoot))
+            Task { CompilationResult(T.dest, PathRef(T.dest)) }
+          else super.compile
+        override def test(args: String*): Command[(String, Seq[TestResult])] =
+          if (!modulesWithTestingEnabled.contains(multiplatformRoot))
+            Task.Command { ("", Seq.empty[TestResult]) }
+          else super.test(args: _*)


Same here, please add a comment why that is needed. Also, in the other places.

lefou · 2024-11-03T15:38:59Z

example/thirdparty/arrow/build.mill

+  val modulesWithTestingEnabled = Set[CoursierModule](
+// these modules don't have hand-written tests
+//    `arrow-libs`.core.`arrow-annotations`,
+//    `arrow-libs`.core.`arrow-cache4k`,
+//    `arrow-libs`.core.`arrow-core-high-arity`,
+
+    `arrow-libs`.core.`arrow-atomic`,
+    `arrow-libs`.core.`arrow-autoclose`,
+    `arrow-libs`.core.`arrow-core-retrofit`,
+    `arrow-libs`.core.`arrow-core-serialization`,
+    `arrow-libs`.core.`arrow-core`,
+    `arrow-libs`.core.`arrow-eval`,
+    `arrow-libs`.core.`arrow-functions`,
+    `arrow-libs`.fx.`arrow-collectors`,
+    `arrow-libs`.fx.`arrow-fx-coroutines`,
+    `arrow-libs`.fx.`arrow-fx-stm`,
+// Cannot test this one - it relies on the module dependency in the .jar format, while Mill provides .class files
+//    `arrow-libs`.optics.`arrow-optics-ksp-plugin`,
+    `arrow-libs`.optics.`arrow-optics-reflect`,
+    `arrow-libs`.optics.`arrow-optics`,
+
+    `arrow-libs`.resilience.`arrow-resilience`
+  )


After I better understand why that is needed at all, I'd like to move this into it's own support trait, so we can just mix in all the customized compile and test tasks, so the actual domain setup is free of these quirks.

lefou · 2024-11-03T17:01:56Z

kotlinlib/src/mill/kotlinlib/KotlinModule.scala

@@ -326,15 +326,15 @@ trait KotlinModule extends JavaModule { outer =>
  def kotlincOptions: T[Seq[String]] = Task {
    val options = Seq.newBuilder[String]
    options += "-no-stdlib"
-    val kotlinLanguageVersion = languageVersion()
-    if (!kotlinLanguageVersion.isBlank) {
+    val kotlinkotlinLanguageVersion = kotlinLanguageVersion()


One kotlin too much

lihaoyi · 2024-11-03T17:18:10Z

I pushed some more changes to try and DRY up the build further. I'm hitting some failures that I suspect are flakiness, but not sure yet. Still looking into it

0xnm · 2024-11-03T18:21:08Z

JS tests may be flaky, because of NPM (sometimes if fails to download the artifact) and also because some tests may time out sometimes (no idea why).

For the common part of the NPM-related JS tooling (mocha, source-map) it is probably better to use the common dest/task in the future, to avoid downloading it for every module.

0xnm · 2024-11-03T18:31:32Z

example/thirdparty/arrow/build.mill

@@ -133,11 +127,11 @@ object `package` extends RootModule {
      s"-Xcommon-sources=${files.mkString(",")}"
    }

-    trait ArrowPlatformModule extends KotlinModule {
+    trait ArrowPlatformModule extends KotlinMavenModule {  outer =>


KotlinMavenModule implies that layout is src/main/... while for multiplatform setup it is usually src/commonMain/... + src/<target>Main/.... What is the advantage of applying it?

Good question. I think it's pretty arbitrary, but it feels closer than the raw src/ folder that KotlinModule provides, and at least it puts the src/main/resources and src/test/resources in the right place. I also ended up using it in ArrowJvmModule which has the exact correct layout, and so thought it would be nice to be consistent even if it doesn't quite fit

lihaoyi · 2024-11-07T00:27:02Z

I think this looks pretty good, going to merge it for now and close out the bounty. We can continue to improve upon it in master and I can try using it to do a comparative study between Mill and Gradle-Kt

0xnm added 2 commits November 1, 2024 23:32

Improvements for Kotlin compilation

a964985

Add arrow-kt example

de9b76d

0xnm force-pushed the add-arrow-kt-example branch from 88ce6e0 to de9b76d Compare November 1, 2024 22:32

0xnm marked this pull request as ready for review November 1, 2024 23:03

lihaoyi reviewed Nov 2, 2024

View reviewed changes

lihaoyi added 2 commits November 1, 2024 18:41

make jvm and js defined per-module

4db2f27

fold [test]{Jvm,Js}* members into normal members of concrete objects

9c3a434

Improve Dokka documentation generation

166172a

0xnm force-pushed the add-arrow-kt-example branch from 0337126 to 166172a Compare November 2, 2024 15:24

lefou reviewed Nov 3, 2024

View reviewed changes

lihaoyi added 2 commits November 3, 2024 08:44

wip

437e1da

cleanups

db33129

lefou reviewed Nov 3, 2024

View reviewed changes

.

d53c1ad

PlatformKotlinModule

e1bee75

0xnm commented Nov 3, 2024

View reviewed changes

Check if source set entry exists for Dokka generation

367e594

lihaoyi mentioned this pull request Nov 5, 2024

Junit5 test discovery reports spurious test runs for non-test classes (500USD bounty) #3910

Closed

lihaoyi merged commit 1a5605a into com-lihaoyi:main Nov 7, 2024
25 checks passed

lefou added this to the 0.12.2 milestone Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `arrow-kt` example #3886

Add `arrow-kt` example #3886

0xnm commented Nov 1, 2024

lihaoyi commented Nov 1, 2024

lihaoyi Nov 2, 2024

lihaoyi commented Nov 2, 2024

lihaoyi commented Nov 2, 2024 •

edited

Loading

lefou left a comment

lefou Nov 3, 2024

lefou Nov 3, 2024 •

edited

Loading

lefou Nov 3, 2024

lefou Nov 3, 2024

lihaoyi commented Nov 3, 2024

0xnm commented Nov 3, 2024

0xnm Nov 3, 2024

lihaoyi Nov 3, 2024

lihaoyi commented Nov 7, 2024

Add arrow-kt example #3886

Add arrow-kt example #3886

Conversation

0xnm commented Nov 1, 2024

lihaoyi commented Nov 1, 2024

lihaoyi Nov 2, 2024

Choose a reason for hiding this comment

lihaoyi commented Nov 2, 2024

lihaoyi commented Nov 2, 2024 • edited Loading

lefou left a comment

Choose a reason for hiding this comment

lefou Nov 3, 2024

Choose a reason for hiding this comment

lefou Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

lefou Nov 3, 2024

Choose a reason for hiding this comment

lefou Nov 3, 2024

Choose a reason for hiding this comment

lihaoyi commented Nov 3, 2024

0xnm commented Nov 3, 2024

0xnm Nov 3, 2024

Choose a reason for hiding this comment

lihaoyi Nov 3, 2024

Choose a reason for hiding this comment

lihaoyi commented Nov 7, 2024

Add `arrow-kt` example #3886

Add `arrow-kt` example #3886

lihaoyi commented Nov 2, 2024 •

edited

Loading

lefou Nov 3, 2024 •

edited

Loading