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

node: Add Windows ARM64 prebuilds to CI #440

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

dennisameling
Copy link
Contributor

This PR adds prebuilds for Windows ARM64 to CI. I've been building these in my fork for a few months and haven't encountered issues with it.

As discussed in #426 (comment), "discussion with the Desktop folks is leaning positive about it", so I took the liberty to create this PR 😊

Copy link
Contributor

@jrose-signal jrose-signal left a comment

Choose a reason for hiding this comment

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

Thanks for the ping! Yes, we can take this, but I'll note this workflow is just for the manually-submitted job to publish to NPM; we're still not compiling for aarch64-apple-darwin or aarch64-pc-windows-msvc on every PR. With macOS I didn't worry about it because we do compile the non-bridge crates for aarch64-apple-ios; do you feel confident that aarch64-pc-windows-msvc is sufficiently exercised by the other AArch64 platforms + AMD64 Windows?

@@ -56,7 +58,7 @@ jobs:

- run: npx prebuildify --napi -t ${{ steps.get-nvm-version.outputs.node-version }} --arch arm64
working-directory: node
if: matrix.os == 'macos-11'
if: matrix.os != 'ubuntu-latest'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a little more semantic: we can rename additional-rust-target to arm64-rust-target and then this can check whether arm64-rust-target is present at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dennisameling
Copy link
Contributor Author

do you feel confident that aarch64-pc-windows-msvc is sufficiently exercised by the other AArch64 platforms + AMD64 Windows?

I would say yes. I've been building this in my fork for a few months from a x64 host and all is good. aarch64-pc-windows-msvc is a Tier 2 with Host Tools platform, just like aarch64-apple-darwin, so it's also well-supported from the Rust side.

we're still not compiling for aarch64-apple-darwin or aarch64-pc-windows-msvc on every PR

Would you like me to add aarch64-pc-windows-msvc to the regular PR pipeline as well? Shall we simply use npx prebuildify for that or would you prefer a different approach? I'm happy to sponsor an ECS LIVA QC710 if you want to run CI tests on Windows ARM64 as well, or would you feel comfortable enough cross-compiling from x64 for now (which is the only architecture for which GH Actions has hosted runners currently)?

@jrose-signal
Copy link
Contributor

Let's leave the per-PR CI as it is for now, then. Setting up a custom GitHub Actions runner is one more thing we'd have to keep running, and without that we'd just be testing compilation, which is pretty unlikely to break just for ARM64 Windows.

Thank you for doing this!

@jrose-signal jrose-signal merged commit 1eb6cea into signalapp:main Jan 21, 2022
@jrose-signal
Copy link
Contributor

Just published v0.12.1 with an ARM64 Windows prebuild. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants