Wasm/WASI target implementation #366

igoriakovlev · 2024-03-11T19:19:40Z

This is Wasm/WASI target implementation. It based on native version with hardcoded tzdb.

Extracted native implementation into intermediate sourceset.
Added WASI implementation
Added timezones-full database project

Before building timezones project you should run generateWasmWasiZoneInfo task to download and convert timezones db (nodejs and npm should be installed in the system). We decided to put the result of this task into the repository but for review it is not generated yet.

igoriakovlev · 2024-03-11T19:23:30Z

timezones/full/build.gradle.kts

+    sourceSets {
+        commonMain {
+            dependencies {
+                compileOnly(project(":kotlinx-datetime"))


Applying timezones without datetime lead to broken compilation (or exception on usage times zone when PL is enabled). This helps us to avoid version collisions on gradle side and circullar dependency while testing.

igoriakovlev · 2024-03-11T19:24:04Z

core/wasmWasi/src/internal/TimeZonesInitializer.kt

+
+@OptIn(InternalDateTimeApi::class)
+private fun zoneDataByName(name: String): ByteArray =
+    timeZonesProvider?.zoneDataByName(name) ?: error("TimeZones are not supported")


We need to decide how to lead user to use timezones dependency in this case

I think this should be an IllegalTimeZoneException (which is documented in the KDoc of TimeZone.of), with the text like "WASI does not support time zones automatically; please add a dependency on kotlinx-datetime-timezones-full"

Also, there should be a clear difference between a zone not being found because it doesn't exist and it not being found because the dependency wasn't provided, though with the current pure-Kotlin TimeZone.kt, it won't work, so we'll need to reorganize the code a bit.

You would like to add it in common TimeZone.of KDoc or in make a separate KDoc for platform-specific TimeZone.of?

igoriakovlev · 2024-03-11T19:25:01Z

core/wasmWasi/src/internal/TimeZonesInitializer.kt

+ */
+@InternalDateTimeApi
+public fun initializeTimeZonesProvider(provider: TimeZonesProvider) {
+    check(timeZonesProvider != provider) { "TimeZone database redeclaration" }


We need to decide what to do if more that one timezone library were added

But this issue can't occur currently, when there's only one artifact with the time zones, right? If we decide to publish partial timezone data, we could do it in a way that resolves conflicts on the build system level. For example, kotlinx-datetime-timezone-basic + kotlinx-datetime-timezones-full, with full depending on basic. So I'm not sure if we will ever encounter such conflicts.

I have no idea how to to resolve such conflicts for different flavors of timezones libraries without a special gradle plugin. We, with gradle plugin team, do not see the way for now, how to get one dependency with gradle resolution, the only way I see now is to allow redeclaration in runtime with a special "priority" level. But how to manage this level's needs a consideration.

Here's a scheme:

kotlinx-datetime-timezones-full -> kotlinx-datetime-timezones-basic -> kotlinx-datetime

A -> B means "A depends on B". kotlinx-datetime-timezones-full only provides new data, the bulk of the data is in kotlinx-datetime-timezones-basic. Then, kotlinx-datetime-timezones-basic could have another thing like initializeFullTimeZonesProvider, which kotlinx-datetime-timezones-full would call. Do you see any problems with this approach?

Yes, for now we are not decided yet what could -basic be but a first thoughts was about to remove some information from timezones (like make only future implementation or +-5 years). But not amount timezones. So the information in timezones then will be not additive.

In any case, we'll be able to work around this somehow. This is a technical thing invisible to the users, so we can safely do anything we want here.

igoriakovlev · 2024-03-12T14:03:23Z

The build fails because of none of TZ binaries in the PR.

dkhalanskyjb

Also, not to forget: when we decide on the versions, artifact names, etc., we'll need to update the README.

core/build.gradle.kts

core/native/src/internal/ThreadLocal.kt

buildSrc/src/main/kotlin/zoneInfosResourcesGenerator.kt

core/nativeAndWasmWasi/src/internal/ThreadLocal.kt

dkhalanskyjb · 2024-03-19T10:51:36Z

core/wasmWasi/src/internal/Platform.kt

+}
+
+internal actual fun currentSystemDefaultZone(): Pair<String, TimeZoneRules?> =
+    throw UnsupportedOperationException("WASI platform does not support system timezone obtaining")


Since we expect this exception to realistically occur, we must document it in TimeZone.currentSystemDefault()'s KDoc.

You would like to add it in common TimeZone KDoc or in make a separate KDoc for platform-specific currentSystemDefaultZone?

The common one, because when writing TimeZone.currentSystemDefault() in common code, you should still be aware that this may fail.

After an internal discussion, we decided that it makes more sense to return the UTC time zone by default. The reason is, because TimeZone.currentSystemDefault() is not easily mockable (see #17), sometimes, there are calls to currentSystemDefault() deep inside the existing code, and to have them throw exceptions now would be problematic. If/when we replace currentSystemDefault() with a version that supports dependency injection better, that version will throw on WASI.

This should still be documented, and it should be in the common code.

So, you suggest to set currentSystemDefault into the UTC and document it, right?
Should we also need to put UTC into availableTimeZoneIds?

timezones/full/build.gradle.kts

dkhalanskyjb · 2024-03-19T14:27:42Z

gradle.properties

 org.gradle.java.installations.fromEnv=JDK_8

 group=org.jetbrains.kotlinx
 version=0.6.0-RC.2
 versionSuffix=SNAPSHOT

+timezonesMajorVersion=1.0
+tzdbVersion=2024a


Note (we'll have to discuss this internally): SemVer doesn't allow letters in the initial components, so 1.0.2024a is an invalid version number. We could stick to this anyway, or we could use the less natural 1.0.2024-a (note the dash).

In the end, this isn't a SemVer anyway, so dashes aren't needed, 2024a is ok.

dkhalanskyjb · 2024-03-19T14:34:55Z

timezones/full/build.gradle.kts

+import org.jetbrains.kotlin.gradle.targets.js.npm.NpmResolverPlugin
+
+plugins {
+    kotlin("multiplatform")


It's a good call to have timezonesMajorVersion so we could describe the compatibility between kotlinx-datetime and the timezones package. It's easy to imagine people sticking to stable versions of the library but still upgrading the time zones.

But the Kotlin version is also part of the compatibility: there are no compatibility guarantees when someone uses the library compiled with the compiler version newer than the one used by the project itself, so to upgrade the timezone database correctly, you will need to upgrade Kotlin, which I think is strange.

I propose this scheme: kotlinx-datetime-timezones-full is compiled with the oldest compiler that works, and when some incompatibility arises between the compiler used for kotlinx-datetime and the one for kotlinx-datetime-timezones-full, only then to upgrade the compiler here, while simultaneously upgrading the timezonesMajorVersion.

Thoughts?

The only thing here is that we have no possibility (in current approach) to stick a range of major versions of timezones to specific range of datetime versions in gradle. So for now it could be only documented as compatibility table.

After a discussion, here's the plan we arrived at:

Initially, while WASI is allowed to remain binary-incompatible and the library itself is unstable, we want to version our timezone database artifacts like 2024a-spi.0.6.0, where 2024a is the timezone database version and 0.6.0 is the library version. The compatibility table isn't needed, because it's self-evident if the artifacts are compatible.

When both the library and WASI reach stability, we'll have 2028c-spi.1, 2029d-spi.2, etc., where 1 and 2 is the version of the protocol by which the timezone artifact registers itself in the datetime library. Yes, these versions will need to be described in a compatibility table.

When we are completely satisfied with the protocol, we may drop the versioning entirely and only have things like 2036f as our timezone database versions.

dkhalanskyjb · 2024-03-19T15:02:09Z

gradle.properties

 org.gradle.java.installations.fromEnv=JDK_8

 group=org.jetbrains.kotlinx
 version=0.6.0-RC.2
 versionSuffix=SNAPSHOT

+timezonesMajorVersion=1.0
+tzdbVersion=2024a


This may be out of this PR's scope, but it's possible to automatically detect that a new version of tzdb was published: https://github.com/ThreeTen/threetenbp/blob/main/.github/workflows/tzdbupdate.yml If you feel like implementing this, it would be nice; if not, we'll just do it later ourselves.

core/wasmWasi/src/internal/TimeZonesInitializer.kt

gradle.properties

buildSrc/src/main/kotlin/zoneInfosResourcesGenerator.kt

settings.gradle.kts

igoriakovlev · 2024-05-21T14:14:48Z

I have rebased onto the master and merged all the AMEND commits by @dkhalanskyjb request.

dkhalanskyjb · 2024-05-23T12:19:33Z

README.md

+    sourceSets {
+        val wasmWasiMain by getting {
+            dependencies {
+                implementation("kotlinx-datetime-zoneinfo", "2024a-spi.0.6.0-RC.2")


Suggested change

implementation("kotlinx-datetime-zoneinfo", "2024a-spi.0.6.0-RC.2")

implementation("kotlinx-datetime-zoneinfo", "2024a-spi.0.6.0")

dkhalanskyjb · 2024-05-23T12:44:02Z

core/common/test/InstantTest.kt

-            val diff = instant1.periodUntil(instant2, TimeZone.currentSystemDefault())
-            val instant3 = instant1.plus(diff, TimeZone.currentSystemDefault())
+            val diff = instant1.periodUntil(instant2, TimeZone.UTC)
+            val instant3 = instant1.plus(diff, TimeZone.UTC)


No longer needed, right?

core/common/src/TimeZone.kt

core/build.gradle.kts

timezones/full/build.gradle.kts

buildSrc/src/main/kotlin/TzdbTasks/TzdbTasks.kt

ilya-g · 2024-08-07T10:22:47Z

core/wasmWasi/src/internal/TimeZonesInitializer.kt

+This is internal API which is not intended to use on user-side.
+ */
+@InternalDateTimeApi
+public interface TimeZonesProvider {


Suppressing such errors is not advisable by the compiler team and can stop working in K2

timezones/full/build.gradle.kts

.gitignore

buildSrc/src/main/kotlin/TzdbTasks/TzdbTasks.kt

timezones/full/build.gradle.kts

ilya-g

We'll wait for 0.6.1 to be released from master before merging this

igoriakovlev · 2024-08-09T12:04:48Z

I sqashed commits so PR is ready to merge. All tests passed. Thank you a lot for the review!

dkhalanskyjb · 2024-09-02T13:59:14Z

If this is good to go, could you rebase it on top of master so that we can merge it?

Co-authored-by: Dmitry Khalanskiy <[email protected]>

igoriakovlev · 2024-09-02T16:09:57Z

If this is good to go, could you rebase it on top of master so that we can merge it?

It is done.

dkhalanskyjb · 2024-09-05T14:27:18Z

Great job, thanks!

igoriakovlev requested review from ilya-g and dkhalanskyjb March 11, 2024 19:19

igoriakovlev commented Mar 11, 2024

View reviewed changes

dkhalanskyjb reviewed Mar 19, 2024

View reviewed changes

dkhalanskyjb reviewed Mar 20, 2024

View reviewed changes

core/wasmWasi/src/internal/TimeZonesInitializer.kt Outdated Show resolved Hide resolved

dkhalanskyjb reviewed Mar 20, 2024

View reviewed changes

gradle.properties Outdated Show resolved Hide resolved

dkhalanskyjb reviewed Mar 20, 2024

View reviewed changes

buildSrc/src/main/kotlin/zoneInfosResourcesGenerator.kt Show resolved Hide resolved

buildSrc/src/main/kotlin/zoneInfosResourcesGenerator.kt Show resolved Hide resolved

buildSrc/src/main/kotlin/zoneInfosResourcesGenerator.kt Outdated Show resolved Hide resolved

dkhalanskyjb added this to the 0.7.0 milestone Apr 12, 2024

dkhalanskyjb reviewed May 13, 2024

View reviewed changes

settings.gradle.kts Outdated Show resolved Hide resolved

igoriakovlev requested a review from dkhalanskyjb May 13, 2024 13:46

igoriakovlev force-pushed the wasi branch from fbfec15 to 97be51a Compare May 21, 2024 14:13

dkhalanskyjb approved these changes May 23, 2024

View reviewed changes

lppedd mentioned this pull request Jul 14, 2024

Support WasmWasi #406

Closed

igoriakovlev force-pushed the wasi branch from f5ca20c to b6f80fd Compare July 25, 2024 15:27

whyoleg mentioned this pull request Aug 4, 2024

Commonize server modules and support JS and WasmJS targets ktorio/ktor#4192

Merged

ilya-g requested changes Aug 7, 2024

View reviewed changes

igoriakovlev requested a review from ilya-g August 7, 2024 12:36

ilya-g requested changes Aug 8, 2024

View reviewed changes

igoriakovlev force-pushed the wasi branch from 97b3507 to 020c70d Compare August 8, 2024 10:27

ilya-g approved these changes Aug 8, 2024

View reviewed changes

igoriakovlev force-pushed the wasi branch 2 times, most recently from 4425dba to f4fcea0 Compare August 9, 2024 09:59

FenstonSingel mentioned this pull request Aug 9, 2024

Update dependencies and clean up build scripts #423

Open

igoriakovlev and others added 6 commits September 2, 2024 18:05

Move native implementation into commonKotlin shared sourceset

65075c7

Add Wasm WASI target

cea3246

Add WASI timezones implementation

1233b92

Co-authored-by: Dmitry Khalanskiy <[email protected]>

Add full timezones version 2024a

571106d

Update README.md

efc8227

Increase GC memory allowance to 2gb to escape memory overflow failure

61f1e60

igoriakovlev force-pushed the wasi branch from 1ac9273 to 61f1e60 Compare September 2, 2024 16:09

dkhalanskyjb merged commit 30ab530 into master Sep 5, 2024
1 check passed

dkhalanskyjb deleted the wasi branch September 5, 2024 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wasm/WASI target implementation #366

Wasm/WASI target implementation #366

igoriakovlev commented Mar 11, 2024

igoriakovlev Mar 11, 2024

igoriakovlev Mar 11, 2024

dkhalanskyjb Mar 19, 2024

igoriakovlev Mar 19, 2024

igoriakovlev Mar 11, 2024

dkhalanskyjb Mar 19, 2024

igoriakovlev Mar 19, 2024

dkhalanskyjb Mar 20, 2024

igoriakovlev Mar 20, 2024

dkhalanskyjb May 13, 2024

igoriakovlev commented Mar 12, 2024

dkhalanskyjb left a comment

dkhalanskyjb Mar 19, 2024

igoriakovlev Mar 19, 2024

dkhalanskyjb Mar 20, 2024

dkhalanskyjb May 13, 2024

igoriakovlev May 13, 2024

dkhalanskyjb May 13, 2024

dkhalanskyjb Mar 19, 2024

dkhalanskyjb May 13, 2024

dkhalanskyjb Mar 19, 2024

igoriakovlev Mar 20, 2024

dkhalanskyjb May 13, 2024

dkhalanskyjb Mar 19, 2024

igoriakovlev commented May 21, 2024

dkhalanskyjb May 23, 2024

dkhalanskyjb May 23, 2024

ilya-g Aug 7, 2024 •

edited

Loading

ilya-g left a comment

igoriakovlev commented Aug 9, 2024

dkhalanskyjb commented Sep 2, 2024

igoriakovlev commented Sep 2, 2024

dkhalanskyjb commented Sep 5, 2024

	implementation("kotlinx-datetime-zoneinfo", "2024a-spi.0.6.0-RC.2")
	implementation("kotlinx-datetime-zoneinfo", "2024a-spi.0.6.0")

Wasm/WASI target implementation #366

Wasm/WASI target implementation #366

Conversation

igoriakovlev commented Mar 11, 2024

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

igoriakovlev commented Mar 12, 2024

dkhalanskyjb left a 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

igoriakovlev commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

ilya-g left a comment

Choose a reason for hiding this comment

igoriakovlev commented Aug 9, 2024

dkhalanskyjb commented Sep 2, 2024

igoriakovlev commented Sep 2, 2024

dkhalanskyjb commented Sep 5, 2024

ilya-g Aug 7, 2024 •

edited

Loading