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

dkhalanskyjb · 2024-05-13T10:17:30Z

settings.gradle.kts

@@ -16,6 +16,8 @@ rootProject.name = "Kotlin-DateTime-library"

 include(":core")
 project(":core").name = "kotlinx-datetime"
+include(":timezones/full")
+project(":timezones/full").name = "kotlinx-datetime-timezones-full"


The artifact name we decided on is kotlinx-datetime-zoneinfo. At least for now, without full.

And if we decide to make an intermediate timezones db?

When we decide to do it, we'll know from the reported use cases what exactly people want, and then we'll choose the names appropriately.

-zoneinfo, not -timezones.

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

Co-authored-by: Dmitry Khalanskiy <52952525+dkhalanskyjb@users.noreply.github.com>

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

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

igoriakovlev added 4 commits May 21, 2024 16:07

Move native implementation into pureKotlin shared sourceset

d08526f

Add Wasm WASI target

797b328

Add WASI timezones implementation

fb16070

Update README.md

97be51a

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

igoriakovlev and others added 3 commits May 23, 2024 18:28

Update core/common/src/TimeZone.kt

3963ba8

Co-authored-by: Dmitry Khalanskiy <52952525+dkhalanskyjb@users.noreply.github.com>

Update core/common/src/TimeZone.kt

3ce7681

Co-authored-by: Dmitry Khalanskiy <52952525+dkhalanskyjb@users.noreply.github.com>

AMEND

f5ca20c

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

dkhalanskyjb May 13, 2024

igoriakovlev May 13, 2024

dkhalanskyjb May 13, 2024

dkhalanskyjb May 13, 2024

igoriakovlev commented May 21, 2024

dkhalanskyjb May 23, 2024

dkhalanskyjb May 23, 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

Are you sure you want to change the base?

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

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