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 5.6 release and macOS 12 with Xcode 13.3 to CI matrix #176

Merged
merged 9 commits into from
Apr 1, 2022

Conversation

MaxDesiatov
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Mar 31, 2022

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
Copy link
Contributor Author

MaxDesiatov commented Mar 31, 2022

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

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

any ideas?

@yonihemi
Copy link
Member

yonihemi commented Apr 1, 2022

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
Copy link
Member

kateinoigakukun commented Apr 1, 2022

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
Copy link
Member

yonihemi commented Apr 1, 2022

@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
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
Copy link
Contributor Author

MaxDesiatov commented Apr 1, 2022

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

@kateinoigakukun
Copy link
Member

kateinoigakukun commented Apr 1, 2022

@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
Copy link
Contributor Author

MaxDesiatov commented Apr 1, 2022

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
Member

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

@MaxDesiatov
Copy link
Contributor Author

Oh dear 😅

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Apr 1, 2022

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
Copy link
Contributor Author

MaxDesiatov commented Apr 1, 2022

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
Copy link
Contributor Author

MaxDesiatov commented Apr 1, 2022

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
Member

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

@MaxDesiatov
Copy link
Contributor 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