Conversation
|
Addresses #4334 |
|
This is amazing! Thanks so much for creating this PR 🙌 |
|
Hm, maybe don't thank me until it actually works 🤣 |
I think wasm-pack is the biggest user of these packages that I'm aware of. ccing @ashleygwilliams @drager |
|
Also: how much do we trust the mac SDK and our ability to avoid accidentally writing ARM-incompatible code? Enough to keep the CI build single-arch (since the ARM version can't be tested anyway) and avoid taking 2x the time to compile on the mac builder? |
|
Are there any downsides of a universal binary? Does that even work 🤔? Otherwise, compiling a separate arm64 for MacOS is fine I think. wasm-pack can then just download the right binary for the platform. |
|
Universal binaries should work just fine; it's been a supported feature on macOS for a long time (previously used for 32 and 64-bit x86). The main downside (compared to having a package with just a single architecture) is that the binary is 2x as big. The advantage is that it's simpler to build and distribute, and potentially to download. |
|
Here's a hacky version with a separate package. @sbc100 do you know of a way to test this on Github Actions without actually creating a release tag? |
I believe under the current setup all you need to do is create a dummy tag on this revision.. and the action will run creating a preview release (which you can then discard). |
|
I don't think there is any need for a universal binary here. wasm-pack seems like its already setup to download and arch-specific, and not universal binary anyway. |
|
OK, I think this is doing what we want now. The only thing I don't really like about it is that I've basically just duplicated the whole archive step. That's not the worst thing, it isn't complex. But I don't know if there's a better way. |
sbc100
left a comment
There was a problem hiding this comment.
There is probably a more elegant way to avoid the duplication here.. but I'm not sure its worth spending more time on.
|
|
||
| - name: cmake (macos) | ||
| run: cmake -S . -B out -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=out/install | ||
| run: cmake -S . -B out -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=out/install -DCMAKE_OSX_ARCHITECTURES=x86_64 |
There was a problem hiding this comment.
Technically the explicit architecture isn't needed, since x86_64 is the default (for now....). But making it explicit seems more understandable, and eventually I guess it will no longer be the default? If this is distracting I can take it out.
There was a problem hiding this comment.
I think we should just leave it as the default.
|
|
||
| - name: build | ||
| run: cmake --build out --config Release | ||
| run: cmake --build out --config Release -v |
There was a problem hiding this comment.
Yes, but I actually prefer having automated build logs be as verbose as possible. It doesn't really cost anything and anytime you have to debug any issue, usually the first thing you have to do is reproduce it with verbose to see the flags.
There was a problem hiding this comment.
What does -v do when running cmake exactly? We should probably use this flag everywhere or nowhere.
|
Should we tag a new release now? |
Sure! |
|
Now that this is merged, does that mean there's a native arm64 release which can be used on an M1? |
Yes, that's right. wasm-pack looks for an arch specific version and if there's an arm64 then I can submit a PR to wasm-pack to download that instead of the x86 version. |
|
Sorry, I just realized that nobody actually tagged a release. I just tagged |
|
Thanks a lot 🙏 |
This allows them to run on both Intel and ARM hardware.