-
-
Notifications
You must be signed in to change notification settings - Fork 135
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 Electron multi-arch/cross-compilation support #12
Add Electron multi-arch/cross-compilation support #12
Conversation
Hi @dennisameling, thanks for working through this. Yes, as you know, we currently use neon for the FFI from Javascript to Rust. I think as they mention, there were some difficulties with 32-bit support. Also, unfortunately, we don't use their 'N-API', although it is on our roadmap. It seems like an update is coming soon from them though... We will keep monitoring for that. |
Neon 0.4.1 was released yesterday, which brings us a bit closer to cross-compilation support, but it still doesn't work with the NAN-based runtime unfortunately:
Let's just wait for the Neon team to release the N-API backend :) They are currently working on preparing a transition plan for current NAN users 🚀 |
4dd28c8
to
c356f3e
Compare
b4d7e1e
to
eacf0d9
Compare
RingRTC has been updated to use the n-api runtime for the neon bindings, so hopefully you will finally be unblocked. |
This is fantastic news, thanks for the update! Give me a few days to look into this 😊🚀 |
000b998
to
5e05c38
Compare
bin/build-electron
Outdated
else | ||
npm_config_arch=x64 npm_config_target_arch=x64 npm_config_disturl=https://atom.io/download/electron npm_config_runtime=electron npm_config_target=11.2.0 npm_config_build_from_source=true npm_config_devdir=~/.electron-gyp RUSTFLAGS="-C link-arg=-s" cargo build --features electron --release | ||
npm_config_arch=${TARGET_ARCH} npm_config_target_arch=${TARGET_ARCH} npm_config_disturl=https://atom.io/download/electron npm_config_runtime=electron npm_config_target=11.2.0 npm_config_build_from_source=true npm_config_devdir=~/.electron-gyp RUSTFLAGS="-C link-arg=-s -C target-feature=+crt-static" cargo build --target ${CARGO_TARGET} --features electron --release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add target-feature=+crt-static
, because the ia32
build was throwing linker errors for msvcrt
. According to a Rust RFC it is supported to add target-feature=+crt-static
: https://github.com/rust-lang/rfcs/blob/master/text/1721-crt-static.md#customizing-linkage-to-the-c-runtime
@jim-signal it's working! Have all three archs for Windows working now: It can be built using Have created an example branch for testing here: https://github.com/dennisameling/signal-ringrtc-node/tree/cross-compilation-support. Tested on x64 and arm64, in both cases It looks like CI / Lints is failing because of the following error, do you have any tips?
Planning to test cross-compilation on Linux and MacOS later today. |
Linux + MacOS x64 also working after this change 🚀 Reported my findings in #12 (comment) |
bf69c09
to
d62f1a3
Compare
Hi @jim-signal, is there anything I can help with to push this over the finish line? This is one of the final bits I need to get a working build on Apple Silicon, and support for that platform is in growing demand: signalapp/Signal-Desktop#4461 (comment) Thanks in advance! |
Probably this is the bottleneck in Apple Silicon support of Signal. |
@dennisameling there is now a conflict in bin/build-electron |
Hi @dennisameling If you can rebase on the latest, and squash your changes to one commit, we can cherry-pick it for a release. Sorry for the current delays. |
b84b13b
to
5c76fd6
Compare
@jim-signal rebased 👍🏼 also updated the docs at And some very good news: I was also able to build for Apple Silicon now, presumably thanks to the recent updates in Signal's WebRTC fork 🎉 This is the final missing piece to being able to build Signal Desktop for Apple Silicon. Here's the native |
5c76fd6
to
2734a92
Compare
Just found out it's also possible to cross-compile for Linux arm64 and ia32. Needed to do the following for that, similar to signalapp/zkgroup#14:
Then you can run the following to cross-compile for Linux arm64 and ia32 (tested on Ubuntu 20.04 x64):
Here's the native binaries I generated through this: https://github.com/dennisameling/signal-ringrtc-node/tree/cross-compilation-support-new/build/linux |
.cargo/config.toml
Outdated
[target.aarch64-unknown-linux-gnu] | ||
linker = "aarch64-linux-gnu-gcc" | ||
[target.i686-unknown-linux-gnu] | ||
linker = "i686-linux-gnu-gcc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can I ask why these are necessary? Shouldn't cross-compiling always find an appropriate cross-linker? (And if you're not cross-compiling, shouldn't you use the default linker?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, cross-compiling from Linux x64 to ia32/arm64 doesn't work because it will use the x64 linker. Also see https://wiki.pine64.org/index.php?title=Cross-compiling&mobileaction=toggle_view_desktop#cargo. This only applies to Linux, on Windows and macOS this is not needed.
Your comment about not cross-compiling makes sense, so what I could do is add some logic to bin/build-electron
that if the toolchain is x86_64-unknown-linux
(the logic is already there), and the target is aarch64
or i686
, it uses said linkers. What do you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my hesitancy comes from writing down specific linkers anywhere in a user-invoked build process. If I use gold as my cross-linker rather than gcc, the build's going to fail here. What do you think about moving the setting to an environment variable in the GitHub workflow, rather than having it be a default in the build scripts?
2734a92
to
92b9f14
Compare
@jrose-signal Alright, as discussed in signalapp/zkgroup#14 (comment) we will just do the absolute minimum work now for cross-compilation scenarios, so that folks at least can cross-compile themselves until the Signal team is ready for it. I just updated the PR accordingly and rebased. Hope the tiny |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennisameling This PR looks good. We are trying to incorporate it in an upcoming 2.10.0 release. Will let you know when it is taken and close it here. Thanks!
This PR has been merged to this commit: 142cbdc Thank you! |
Now that ringrtc has switched to
neon
's N-API backend, we can cross-compile to different architectures for Electron 🎉 This PR adds support for building ia32 and arm64. Please note that this still only supports building from a x64 host.This work is related to signalapp/Signal-Desktop#3745 and signalapp/Signal-Desktop#4461.
You can test this by running
make electron NODEJS_ARCH=arm64
. Supported archs arex64
,ia32
,arm64
(in line with NodeJS' archs), default is x64.Tested on:
MacOS (darwin) arm64 / Apple Silicon
MacOS (darwin) arm64 doesn't seem to work yetFIXED ✔️: #12 (comment)