swiftwasm / JavaScriptKit

Swift framework to interact with JavaScript through WebAssembly.
https://swiftpackageindex.com/swiftwasm/JavaScriptKit/main/documentation/javascriptkit
MIT License
670 stars 44 forks source link

Add 5.6 release and macOS 12 with Xcode 13.3 to CI matrix #176

Closed MaxDesiatov closed 2 years ago

github-actions[bot] commented 2 years ago

Time Change: -9,130.25ms (2149%) 🏆

Total Time: 424.75ms

ℹī¸ View Unchanged | Test name | Duration | Change | | :--- | :---: | :---: | | Serialization/Write JavaScript number directly | 210ms | +4ms (1%) | | Serialization/Write JavaScript string directly | 214.75ms | +1.25ms (0%) | | Serialization/Swift Int to JavaScript | 0ms | -2,827.25ms (0%) | | Serialization/Swift String to JavaScript | 0ms | -3,031.25ms (0%) | | Object heap/Increment and decrement RC | 0ms | -3,277ms (0%) |

performance-action

MaxDesiatov commented 2 years ago

I'm seeing consistent errors with the latest SwiftWasm 5.6 snapshot:

TypeError: Cannot read properties of undefined (reading 'buffer')

any ideas?

yonihemi commented 2 years ago

I can easily reproduce on macOS 12, not sure how the test passed. Investigating the issue, for now findings are it's coming from swift.setInstance causing Swift error:

JavaScriptKit/JSClosure.swift:138: Fatal error: The function was already released

(Guessing it's more of a function not found issue than released?)

kateinoigakukun commented 2 years ago

I guess this is due to the introduction of the reactor model in WASI and wasi-libc. See https://github.com/WebAssembly/WASI/issues/13

This means we need to build Swift executable with reactor model when using JSKit because our runtime calls some exported functions multiple times.

yonihemi commented 2 years ago

@kateinoigakukun wouldn't libc changes be in the toolchain? I now cannot get carton/JSKit (from main) to work at all (similar issues to this), but I'm using swift-wasm-5.6-SNAPSHOT-2022-03-23-a toolchain which was working fine with previous carton/JSKit. I also don't see JS wasmer version change, so can't figure it out.

kateinoigakukun commented 2 years ago

@yonihemi I recently updated wasi-sdk including wasi-libc https://github.com/swiftwasm/swift/pull/4360 and backported it to 5.6 branch. Here is a minimum patch to pass the integration tests https://github.com/swiftwasm/JavaScriptKit/compare/katei/try-reactor-model but we have to think about how to pass -mexec-model=reactor when building.

MaxDesiatov commented 2 years ago

Would passing -mexec-model=reactor in carton be sufficient, or do you prefer some other way to pass it?

kateinoigakukun commented 2 years ago

@MaxDesiatov I think it would be sufficient for most use cases, but I may overlook something... Anyway, it's OK to pass the flag in IntegrationTests/Makefile for now.

MaxDesiatov commented 2 years ago

It's a bit unclear to me how this option should be passed to swift build. I've tried all the different combinations, including -Xswiftc -Xclang-linker -mexec-model=reactor, which leads to error: Unknown option '-mexec-model', as the rest of the combinations did.

I wonder if having these flags in Package.swift, like in your branch, is a better approach?

kateinoigakukun commented 2 years ago

@MaxDesiatov -Xswiftc -Xclang-linker -Xswiftc -mexec-model=reactor is the correct combination

MaxDesiatov commented 2 years ago

Oh dear 😅

MaxDesiatov commented 2 years ago

CI is green. After testing with Tokamak, I think it's time to tag SwiftWasm 5.6.0 and then update this PR for that release. Any objections?

MaxDesiatov commented 2 years ago

Something's broken in distribute-latest-toolchain.sh I guess? For some reason wasm-5.6.0-RELEASE.pkg is unpacked to /Library/Developer/Toolchains/swift-wasm-5.6-SNAPSHOT-2022-04-01-a.xctoolchain instead of /Library/Developer/Toolchains/swift-wasm-5.6.0-RELEASE.xctoolchain as we'd expect, even though I've specified same distribution workflow parameters as I did for 5.5.0. This is the reason for latest CI failure on this PR.

I'm not 100% sure if https://github.com/swiftwasm/swift/commit/3be7ec1f82cb96c9f3ef6fe5a155e0e578132f2a had any impact, but that's the only recent change in that script that I see.

I don't have more time to look into this either today or even this weekend, sorry. Maybe next week. If anyone can pick this up in the meantime, I'd greatly appreciate it. As usual, no need to rush, I'm happy to delay 5.6.0 for whatever time it takes to make it work.

MaxDesiatov commented 2 years ago

BTW, these are the settings that I use for each release, just swapping patch and date numbers in it. I'm keeping this screenshot for reference, but I guess it's time to create a release process document and attach it there 😅 SwiftWasm-release

kateinoigakukun commented 2 years ago

@MaxDesiatov That's my bad đŸ¤Ļ I've fixed it now https://github.com/swiftwasm/swift/commit/b36d31731514e6783b05b573de48dce3df4fb8ac

MaxDesiatov commented 2 years ago

ok, I'll kick off new manual distribution then