Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
MaxDesiatov merged 9 commits into
mainswiftwasm/JavaScriptKit:mainfrom
macos-12/xcode-13.3swiftwasm/JavaScriptKit:macos-12/xcode-13.3Copy head branch name to clipboard
Apr 1, 2022
Merged

Add 5.6 release and macOS 12 with Xcode 13.3 to CI matrix#176
MaxDesiatov merged 9 commits into
mainswiftwasm/JavaScriptKit:mainfrom
macos-12/xcode-13.3swiftwasm/JavaScriptKit:macos-12/xcode-13.3Copy head branch name to clipboard

Conversation

@MaxDesiatov

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actions Bot commented Mar 31, 2022

Copy link
Copy Markdown

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 MaxDesiatov marked this pull request as ready for review March 31, 2022 16:57
@MaxDesiatov MaxDesiatov requested a review from a team March 31, 2022 17:00
@MaxDesiatov MaxDesiatov changed the title Add macOS 12 with Xcode 13.3 to CI matrix Add SwiftWasm 5.6 snapshot and macOS 12 with Xcode 13.3 to CI matrix Mar 31, 2022
@MaxDesiatov MaxDesiatov changed the title Add SwiftWasm 5.6 snapshot and macOS 12 with Xcode 13.3 to CI matrix Add 5.6 snapshot and macOS 12 with Xcode 13.3 to CI matrix Mar 31, 2022
@MaxDesiatov

MaxDesiatov commented Mar 31, 2022

Copy link
Copy Markdown
Member Author

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

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

any ideas?

@yonihemi

yonihemi commented Apr 1, 2022

Copy link
Copy Markdown
Member

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

kateinoigakukun commented Apr 1, 2022

Copy link
Copy Markdown
Member

I guess this is due to the introduction of the reactor model in WASI and wasi-libc. See WebAssembly/WASI#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

yonihemi commented Apr 1, 2022

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

@yonihemi I recently updated wasi-sdk including wasi-libc swiftwasm/swift#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

MaxDesiatov commented Apr 1, 2022

Copy link
Copy Markdown
Member Author

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

@kateinoigakukun

kateinoigakukun commented Apr 1, 2022

Copy link
Copy Markdown
Member

@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

MaxDesiatov commented Apr 1, 2022

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

@MaxDesiatov

Copy link
Copy Markdown
Member Author

Oh dear 😅

@MaxDesiatov

MaxDesiatov commented Apr 1, 2022

Copy link
Copy Markdown
Member Author

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

MaxDesiatov commented Apr 1, 2022

Copy link
Copy Markdown
Member Author

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 swiftwasm/swift@3be7ec1 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 MaxDesiatov changed the title Add 5.6 snapshot and macOS 12 with Xcode 13.3 to CI matrix Add 5.6 release and macOS 12 with Xcode 13.3 to CI matrix Apr 1, 2022
@MaxDesiatov

MaxDesiatov commented Apr 1, 2022

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

@MaxDesiatov That's my bad 🤦 I've fixed it now swiftwasm/swift@b36d317

@MaxDesiatov

Copy link
Copy Markdown
Member Author

ok, I'll kick off new manual distribution then

@MaxDesiatov MaxDesiatov merged commit 0a38d70 into main Apr 1, 2022
@MaxDesiatov MaxDesiatov deleted the macos-12/xcode-13.3 branch April 1, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.