misc: Set up Kotlin/Native build #1174

lauzadis · 2024-11-06T19:51:29Z

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…efactor-plugin

…irectory

…efactor-plugin

0marperez · 2024-11-13T17:46:49Z

runtime/crt-util/common/src/aws/smithy/kotlin/runtime/crt/ReadChannelBodyStream.kt

+import aws.smithy.kotlin.runtime.io.SdkBuffer
+
+/**
+ * write as much of [outgoing] to [dest] as possible


Nit: Capitalization

0marperez · 2024-11-13T17:52:14Z

.../test/aws/smithy/kotlin/runtime/http/interceptors/RequestCompressionInterceptorTestNative.kt

@@ -5,5 +5,5 @@
 package aws.smithy.kotlin.runtime.http.interceptors

 internal actual fun decompressGzipBytes(bytes: ByteArray): ByteArray {
-    TODO("Not yet implemented")
+    TODO("Not yet implemented. Can we write a pure Kotlin implementation?")


Nit: Leftover comment

About the comment itself, I was hoping Okio had already done it but it looks like not yet.

This is not leftover. It's something we should try to do while implementing Native GZIP / before launching Kotlin/Native support

ianbotsf · 2024-11-13T16:44:14Z

.github/workflows/ci.yml

+        run: |
+          ./gradlew apiCheck
+          ./gradlew -P"aws.sdk.kotlin.crt.disableCrossCompile"=true build
+          ./gradlew -P"aws.sdk.kotlin.crt.disableCrossCompile"=true allTests


Nit: Double quotes for the property names seem unnecessary here.

./gradlew -Paws.sdk.kotlin.crt.disableCrossCompile=true build ./gradlew -Paws.sdk.kotlin.crt.disableCrossCompile=true allTests

I forgot exactly why, but it is necessary on Windows, I'll remove them and see what fails (or if the issue has been fixed since then)

* What went wrong: Task '.sdk.kotlin.crt.disableCrossCompile=true' not found in root project 'smithy-kotlin' and its subprojects.

ianbotsf · 2024-11-13T16:46:50Z

.github/workflows/ci.yml

+        run: |
+          # FIXME K2. Re-enable warnings as errors after this warning is removed: https://youtrack.jetbrains.com/issue/KT-68532
+          # echo "kotlinWarningsAsErrors=true" >> $GITHUB_WORKSPACE/local.properties              
+          ./gradlew apiCheck
+          ./gradlew -Paws.sdk.kotlin.crt.disableCrossCompile=true build
+          ./gradlew -Paws.sdk.kotlin.crt.disableCrossCompile=true allTests


Question: Why run allTests again after build? Doesn't build already include allTests as a prerequisite? (same question in other blocks)

Yes I think you're right, this was a leftover copy-paste where only allTests was running

ianbotsf · 2024-11-13T16:49:20Z

gradle/libs.versions.toml


-aws-kotlin-repo-tools-version = "0.4.13"
+aws-kotlin-repo-tools-version = "0.4.15-kn"



Question: Does this mean we have to maintain parallel releases of the repo tools until we're ready to fully GA Kotlin/Native support? What's unsafe/unready about the K/N support in repo tools now that prevents it from being merged to main and used for all smithy-kotlin branches including main?

The main difference so far is all the targets which get enabled: awslabs/aws-kotlin-repo-tools@main...kn-main

I think we might be able to move this to main and drop the parallel versioning strategy.

This parallel release strategy is something that's been planned from the beginning. I can't say for sure but there may be more changes forthcoming which will require us to have this split.

ianbotsf · 2024-11-13T18:27:25Z

...nt-engine-crt/jvmAndNative/test/aws/smithy/kotlin/runtime/http/engine/crt/AsyncStressTest.kt

+// FIXME This test implements [TestWithLocalServer] which is JVM-only.
 class AsyncStressTest : TestWithLocalServer() {


Question: Are we tracking these K/N-specific FIXMEs/TODOs somehow? I'm worried that some may slip through the cracks as we fill out Native support.

Not currently. I was imagining we'd spot them during the kn-main -> main merge

ianbotsf · 2024-11-13T18:33:24Z

.../test/aws/smithy/kotlin/runtime/http/interceptors/RequestCompressionInterceptorTestNative.kt

 internal actual fun decompressGzipBytes(bytes: ByteArray): ByteArray {
-    TODO("Not yet implemented")
+    TODO("Not yet implemented. Can we write a pure Kotlin implementation?")
 }


Comment: We can but we probably shouldn't for performance and correctness reasons. We should look for existing platform implementations where possible. For Native that probably means binding to something like zlib or CRT.

ianbotsf · 2024-11-13T18:38:48Z

runtime/runtime-core/jvm/src/aws/smithy/kotlin/runtime/time/InstantJVM.kt

@@ -26,6 +26,7 @@ import kotlin.time.toKotlinDuration
 import java.time.Duration as jtDuration
 import java.time.Instant as jtInstant

+// FIXME Consider making this multiplatform (`common`) using kotlinx.datetime


Comment: Early in the SDK design, we rejected kotlinx-datetime as a backing impl because of concerns about support and long-term stability. These concerns might be addressed once kotlinx-datetime is promoted to Beta or once Instant and Clock are moved to stdlib.

ianbotsf · 2024-11-13T18:42:17Z

settings.gradle.kts

+// Set up aws-crt-kotlin as a composite build
+val localAwsCrtKotlin = file("../aws-crt-kotlin")
+if (localAwsCrtKotlin.exists()) {
+    println("Including aws-crt-kotlin as a composite build")
+    includeBuild(localAwsCrtKotlin)
+} else {
+    println("Could not find aws-crt-kotlin in a sibling directory, not including it")
+}
+


Suggestion: This seems brittle. I'd rather see a more flexible solution like we have for aws-sdk-kotlin, ideally extracted into the repo tools if that's possible.

What's brittle about it besides not supporting a local.properties file? Does anyone even use that configuration option? I didn't see any use of it in our CI / other tooling

ianbotsf · 2024-11-13T18:43:32Z

settings.gradle.kts

+// Set up aws-crt-kotlin as a composite build
+val localAwsCrtKotlin = file("../aws-crt-kotlin")
+if (localAwsCrtKotlin.exists()) {
+    println("Including aws-crt-kotlin as a composite build")
+    includeBuild(localAwsCrtKotlin)
+} else {
+    println("Could not find aws-crt-kotlin in a sibling directory, not including it")
+}
+


Question: This sets up a composite build with aws-crt-kotlin for smithy-kotlin but what if I'm building aws-sdk-kotlin and I'm already setting compositeProjects=../smithy-kotlin,../aws-crt-kotlin? Does the CRT project get built twice? Does it fail?

I don't configure compositeProjects at all and my aws-sdk-kotlin build gets resolved correctly with all three projects included

ianbotsf · 2024-11-13T18:45:27Z

...ttp-client-engines/test-suite/jvm/src/aws/smithy/kotlin/runtime/http/test/suite/Downloads.kt

+            // FIXME This is the only thing keeping this directory in `jvm`, could be `jvmAndNative`
            get("/gzipped") {
                val uncompressed = ByteArray(1024) { it.toByte() }
                val compressed = ByteArrayOutputStream().use { baStream ->


Question: Don't we already know we need KMP GZip support and already have expect/actual declarations for it? Can we use those here and then move this to the jvmAndNative source set? Or do other blockers remain?

Maybe we can use our expect/actual declarations, I'll take a look. Right now this is using java.util.zip.GZIPOutputStream which is definitely JVM-only

Moved to jvmAndNative by using our existing GzipSdkSource class

This reverts commit f8135f6.

lauzadis added 30 commits October 24, 2024 10:10

Latest

5dc2a7c

Depend on -kn plugin

6115504

Move jvm files to jvmAndNative where appropriate

4e6cc64

Remove commented block

83087c5

ktlint

a5b1be0

ktlint

cbdfb3f

Merge branch 'main' of github.com:smithy-lang/smithy-kotlin into kn-r…

1e09b66

…efactor-plugin

Revert to non-SNAPSHOT version

f3a1ec6

Re-enable warnings as errors

29e00d0

Remove K/N targets setup from build script

8c3d77a

Replace all-platforms test jvmTest with allTests

81e8f3b

Ignore *.cinteropLibraries.json

5e03508

WarningsAsErrors

256fef5

WarningsAsErrors

a2ad3ef

WarningsAsErrors

bc6b343

Create a setup-build script to checkout aws-crt-kotlin as a sibling d…

a5fa0da

…irectory

Fix path

f71be44

Depend on 0.4.15-kn

0e17527

CI

fb048b3

downstream ..

1c10cc7

CI

f6eb3c0

Make it a path

cd4b31d

Add debugging steps

3512d81

More debug

eae7a9c

More debug

8efb8e8

More debug

bccbbac

Fixed, remove debug

bab02b1

Add an else println

15488ac

add with path smithy-kotlin to all checkouts

96cfd7d

Also update setup-build location

713222d

lauzadis added 16 commits November 7, 2024 16:41

working-directory: ./smithy-kotlin

fae778c

Disable KMP builds on JVM suite

cc273a2

Fix JVM build

161b57a

Fix aws-signing-crt

5d8a1f5

Test macos suite

42b0453

Disable KMP for protocol-tests and downstream

2e9fe71

Merge branch 'main' of github.com:smithy-lang/smithy-kotlin into kn-r…

8651038

…efactor-plugin

Add linux platform

7cd3ed9

Add windows platform

0fbb759

Remove apiCheck

72cf8ad

Revert to windows-2022

f67dda7

working-directory

69d57ee

CI

dea452e

jvmTest -> jvmAndNativeTest

628ff9e

Remove OCI_EXE

734c830

Change CI names

57d0b70

lauzadis marked this pull request as ready for review November 13, 2024 15:29

lauzadis requested a review from a team as a code owner November 13, 2024 15:29

0marperez approved these changes Nov 13, 2024

View reviewed changes

ianbotsf approved these changes Nov 13, 2024

View reviewed changes

lauzadis added 7 commits November 13, 2024 14:17

Remove allTests

dacca68

Remove quotes on Windows

f8135f6

Move ktor test suite to jvmAndNative

9795627

Fix erroneous diff

779173b

Revert "Remove quotes on Windows"

6f0dd35

This reverts commit f8135f6.

Support local.properties compositeProjects

19fd715

Cache K/N toolchain

fa0b7c0

lauzadis merged commit 9d9faf3 into kn-main Nov 14, 2024
7 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: Set up Kotlin/Native build #1174

misc: Set up Kotlin/Native build #1174

lauzadis commented Nov 6, 2024

0marperez Nov 13, 2024

0marperez Nov 13, 2024

0marperez Nov 13, 2024

lauzadis Nov 13, 2024

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024

lauzadis Nov 13, 2024

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024

ianbotsf Nov 13, 2024

ianbotsf Nov 13, 2024

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024 •

edited

Loading

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024

ianbotsf Nov 13, 2024

lauzadis Nov 13, 2024

lauzadis Nov 13, 2024


		aws-kotlin-repo-tools-version = "0.4.13"
		aws-kotlin-repo-tools-version = "0.4.15-kn"

		// FIXME This test implements [TestWithLocalServer] which is JVM-only.
		class AsyncStressTest : TestWithLocalServer() {

misc: Set up Kotlin/Native build #1174

misc: Set up Kotlin/Native build #1174

Conversation

lauzadis commented Nov 6, 2024

Issue #

Description of changes

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

lauzadis Nov 13, 2024 • 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

lauzadis Nov 13, 2024 •

edited

Loading