Skip to content

Build ARM64 MacOS releases #4397

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

Merged
merged 9 commits into from
Jan 6, 2022
Merged

Build ARM64 MacOS releases #4397

merged 9 commits into from
Jan 6, 2022

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Dec 16, 2021

This allows them to run on both Intel and ARM hardware.

@dschuff
Copy link
Member Author

dschuff commented Dec 16, 2021

Addresses #4334
The other option is to build two different packages (reduces the overall size but means you have to care which one you get).

@d3lm
Copy link

d3lm commented Dec 16, 2021

This is amazing! Thanks so much for creating this PR 🙌

@dschuff
Copy link
Member Author

dschuff commented Dec 16, 2021

Hm, maybe don't thank me until it actually works 🤣
would it be more useful to have a single package with a universal binary, or separate package?
@kripken and Binaryen folks, do you have opinions on whether it would be better to do universal binaries or separate packages? Do we know anything about who all uses these packages?

@kripken
Copy link
Member

kripken commented Dec 16, 2021

@dschuff

do you have opinions on whether it would be better to do universal binaries or separate packages? Do we know anything about who all uses these packages?

I think wasm-pack is the biggest user of these packages that I'm aware of. ccing @ashleygwilliams @drager

@dschuff
Copy link
Member Author

dschuff commented Dec 16, 2021

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?

@d3lm
Copy link

d3lm commented Dec 17, 2021

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.

@dschuff
Copy link
Member Author

dschuff commented Jan 4, 2022

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.

@dschuff
Copy link
Member Author

dschuff commented Jan 5, 2022

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?

@sbc100
Copy link
Member

sbc100 commented Jan 5, 2022

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).

@sbc100
Copy link
Member

sbc100 commented Jan 5, 2022

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.

@dschuff
Copy link
Member Author

dschuff commented Jan 5, 2022

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.

@dschuff dschuff requested a review from sbc100 January 5, 2022 20:00
@dschuff dschuff changed the title Build MacOS releases as universal binaries Build ARM64 MacOS releases Jan 5, 2022
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

There is probably a more elegant way to avoid the duplication here.. but I'm not sure its worth spending more time on.

@@ -73,7 +73,7 @@ jobs:
if: matrix.os == 'ubuntu-latest'

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

Choose a reason for hiding this comment

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

Why this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should just leave it as the default.

@@ -82,7 +82,7 @@ jobs:
if: matrix.os == 'windows-latest'

- name: build
run: cmake --build out --config Release
run: cmake --build out --config Release -v
Copy link
Member

Choose a reason for hiding this comment

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

Debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

What does -v do when running cmake exactly? We should probably use this flag everywhere or nowhere.

@dschuff dschuff merged commit 73897b0 into main Jan 6, 2022
@dschuff dschuff deleted the arm branch January 6, 2022 01:09
@dschuff
Copy link
Member Author

dschuff commented Jan 6, 2022

Should we tag a new release now?

@sbc100
Copy link
Member

sbc100 commented Jan 6, 2022

Should we tag a new release now?

Sure!

@d3lm
Copy link

d3lm commented Jan 6, 2022

Now that this is merged, does that mean there's a native arm64 release which can be used on an M1?

@d3lm
Copy link

d3lm commented Jan 6, 2022

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.

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.

@dschuff
Copy link
Member Author

dschuff commented Jan 12, 2022

Sorry, I just realized that nobody actually tagged a release. I just tagged version_105, it should be up on https://github.com/WebAssembly/binaryen/releases shortly.

@d3lm
Copy link

d3lm commented Jan 18, 2022

Thanks a lot 🙏

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.

4 participants