Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add K/Wasm support #914

Merged
merged 40 commits into from
Dec 6, 2023
Merged

Add K/Wasm support #914

merged 40 commits into from
Dec 6, 2023

Conversation

eymar
Copy link
Collaborator

@eymar eymar commented Nov 24, 2023

This PR:

  • updates kotlin to 1.9.21
  • removes watchosX86() target - it's not available since kotlin 1.9.20

@eymar eymar changed the title Wasm main sync 1.5.10 Add K/Wasm support Nov 24, 2023
@@ -35,6 +35,19 @@ class AndroidXPlugin : Plugin<Project> {
"from" to "$supportRoot/buildSrc/apply/applyAndroidXImplPlugin.gradle"
)
)

project.configurations.all {
// TODO: remove these HACKS when possible
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not supposed to be merged into jb-main branch.
Should be removed when all required libs are properly built and published

@@ -138,10 +138,20 @@ class AndroidXComposeImplPlugin : Plugin<Project> {
}

project.tasks.withType(KotlinJsCompile::class.java).configureEach { compile ->
val isWasm = compile.kotlinOptions.freeCompilerArgs.contains("-Xwasm")
Copy link
Collaborator Author

@eymar eymar Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently KotlinJsCompile task type is reused for k/wasm compilations.

For the nearest release we'll keep decoys enabled for k/js, but not for k/wasm. In the future it will be disabled for both.

project.rootProject.resolveProject(":compose:compiler:compiler")
}
)
project.dependencies.add(COMPILER_PLUGIN_CONFIGURATION, "org.jetbrains.compose.compiler:compiler:1.5.4")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To build the compose-core libs with our published compose compiler plugin, rather than from the plugin built from the sources.
This way we have a single source of truth.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's move this version to gradle.properties

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -125,6 +126,7 @@ class ComposeIrGenerationExtension(

val mangler = when {
pluginContext.platform.isJs() -> JsManglerIr
pluginContext.platform.isWasm() -> JsManglerIr
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, the changes in compose/compiler can be ignored. When merging from upstream we can take their sources.
We have a separate branch with our patches for building and publishing the compose compiler plugin.

@@ -193,6 +193,6 @@ private fun Any?.integralValue(): Long = when (this) {

private fun Any?.toStringForAssert(): String = when {
this == null -> toString()
isIntegralBoxedPrimitive() -> "${this::class.qualifiedName}<$this>"
isIntegralBoxedPrimitive() -> "${this::class.simpleName}<$this>"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qualifiedName is a reflection API not available for web targets.

@@ -112,20 +112,3 @@ apple {
}
}
}

// TODO: Workaround, see https://youtrack.jetbrains.com/issue/KT-55751
val myAttribute = Attribute.of("myOwnAttribute", String::class.java)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dima has checked that this workaround is not needed anymore with kotlin 1.9.2x

/**
* Returns if the registry is setup to receive registrations from [SkiaRootForTest]s
*/
val isSetUp: Boolean
get() = SkiaRootForTest.onRootCreatedCallback == ::onRootCreated
get() = SkiaRootForTest.onRootCreatedCallback == _onRootCreated
Copy link
Collaborator Author

@eymar eymar Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @MatkovIvan JFYI: unfortunately, not all targets reuse the same reference here.
Apparently, k/wasm creates a new instance every time and the check is always false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just store isSetUp as a boolean flag then

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@eymar eymar requested a review from igordmn November 29, 2023 16:36
.gitignore Outdated
@@ -39,3 +39,5 @@ __pycache__

# Don't ignore src files under build package
!**/src/**/build
/jbdeps/android-sdk/linux/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? We already have jbdeps/.gitignore

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Right, for some reason it didn't ignore the files for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the change

@@ -32,7 +32,7 @@ allprojects {
kotlinOptions {
jvmTarget = "17"
freeCompilerArgs += [
"-Werror",
// "-Werror", // commented in JB-fork
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not comment this, and fix the warnings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

project.rootProject.resolveProject(":compose:compiler:compiler")
}
)
project.dependencies.add(COMPILER_PLUGIN_CONFIGURATION, "org.jetbrains.compose.compiler:compiler:1.5.4")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's move this version to gradle.properties

actual fun get(): Int = delegate.value
actual fun set(value: Int) {
delegate.value = value
}
actual fun add(amount: Int): Int = delegate.addAndGet(amount)
}

@OptIn(ExperimentalNativeApi::class)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep in mind that we can't release Compose for iOS if at that time it is still be experimental. It doesn't add binary compatibility guarantees, which we need for stable Compose targets.


@OptIn(DelicateCoroutinesApi::class)
actual suspend fun testWithTimeout(timeoutMs: Long, block: suspend CoroutineScope.() -> Unit) {
GlobalScope.launch {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GlobalScope.launch {
coroutineScope {
block()
}

Will this work? An async call is fragile.

Copy link
Collaborator Author

@eymar eymar Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not implemented properly at all (I added this dumb implementation a few month ago and forgot about it). It ignores timeoutMs. I'll update it separately.
It's used in our tests only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COMPOSE-662

*/

// TODO: in a browser we have only 1 thread and it's actually a UI thread.
// But returning `true` here breaks `setContent` - `waitForIdle` is not called, but seems to be necessary.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create an issue to investigate this. It shouldn't work this way.

Copy link
Collaborator Author

@eymar eymar Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the comment - COMPOSE-661

languageVersion = "1.5"
apiVersion = "1.5"
languageVersion = "1.8"
apiVersion = "1.8"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kotlin 1.9.21 shows a warning for 1.5

@eymar eymar marked this pull request as ready for review December 6, 2023 15:18
@eymar eymar merged commit 3d70ec3 into jb-main Dec 6, 2023
1 check passed
@eymar eymar deleted the wasm-main-sync-1.5.10 branch December 6, 2023 16:22
mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
This PR:
- updates kotlin to 1.9.21
- removes watchosX86() target - it's not available since kotlin 1.9.20

---------

Co-authored-by: Oleksandr Karpovich <oleksandr.karpovich@jetbrains.com>
eymar added a commit to JetBrains/compose-multiplatform that referenced this pull request Dec 8, 2023
This a comanion PR to
JetBrains/compose-multiplatform-core#914

Resources lib doesn't contain k/wasm target yet. It will be added in a
separate PR

---------

Co-authored-by: Oleksandr Karpovich <oleksandr.karpovich@jetbrains.com>
igordmn pushed a commit that referenced this pull request Jan 30, 2024
This PR:
- updates kotlin to 1.9.21
- removes watchosX86() target - it's not available since kotlin 1.9.20

---------

Co-authored-by: Oleksandr Karpovich <oleksandr.karpovich@jetbrains.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants