search

signalapp / ringrtc

GNU Affero General Public License v3.0
550 stars 135 forks source link

Add Electron multi-arch/cross-compilation support #12

Closed dennisameling closed 3 years ago

dennisameling commented 4 years ago

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 https://github.com/signalapp/Signal-Desktop/issues/3745 and https://github.com/signalapp/Signal-Desktop/issues/4461.

You can test this by running make electron NODEJS_ARCH=arm64. Supported archs are x64, 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 yet~ FIXED ✔️: https://github.com/signalapp/ringrtc/pull/12#issuecomment-830689652

jim-signal commented 4 years ago

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.

dennisameling commented 4 years ago

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:

denni@DESKTOP-HIGHLVU MINGW64 ~/repos/ringrtc/src/rust (add-windows-multi-arch-support)
$  npm_config_arch=x86 npm_config_target_arch=x86 npm_config_disturl=https://atom.io/download/electron npm_config_runtime=electron npm_config_target=8.2.5 npm_config_build_from_source=true npm_config_devdir=~/.electron-gyp RUSTFLAGS="-C link-arg=-s" cargo build --target i686-pc-windows-msvc --features electron --release
   Compiling neon-sys v0.4.1
   Compiling curve25519-dalek v2.1.0
   Compiling futures-executor v0.3.5
   Compiling futures v0.3.5
   Compiling x25519-dalek v0.6.0
error: failed to run custom build command for `neon-sys v0.4.1`

Caused by:
  process didn't exit successfully: `C:\Users\denni\repos\ringrtc\src\rust\target\release\build\neon-sys-52c875801be308ce\build-script-build` (exit code: 101)
--- stdout
'Skipping node-gyp installation as part of npm install.'
audited 97 packages in 1.561s
found 3 low severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details
cargo:node_root_dir=C:\\Users\\denni\\.electron-gyp\\8.2.5
cargo:node_lib_file=C:\\\\Users\\\\denni\\\\.electron-gyp\\\\8.2.5\\\\<(target_arch)\\\\node.lib

--- stderr
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', C:\Users\denni\.cargo\registry\src\github.com-1ecc6299db9ec823\neon-sys-0.4.1\build.rs:98:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

warning: build failed, waiting for other jobs to finish...
error: failed to run custom build command for `neon-sys v0.4.1`

Caused by:
  process didn't exit successfully: `C:\Users\denni\repos\ringrtc\src\rust\target\release\build\neon-sys-52c875801be308ce\build-script-build` (exit code: 101)
--- stdout
'Skipping node-gyp installation as part of npm install.'
added 97 packages from 70 contributors and audited 97 packages in 4.663s
found 3 low severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details
cargo:node_root_dir=C:\\Users\\denni\\.electron-gyp\\8.2.5
cargo:node_lib_file=C:\\\\Users\\\\denni\\\\.electron-gyp\\\\8.2.5\\\\<(target_arch)\\\\node.lib

--- stderr
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', C:\Users\denni\.cargo\registry\src\github.com-1ecc6299db9ec823\neon-sys-0.4.1\build.rs:98:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

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 🚀

jim-signal commented 3 years ago

RingRTC has been updated to use the n-api runtime for the neon bindings, so hopefully you will finally be unblocked.

dennisameling commented 3 years ago

This is fantastic news, thanks for the update! Give me a few days to look into this 😊🚀

dennisameling commented 3 years ago

First results looking good! Was able to generate an arm64 .node file alongside the x64 one. x86 led to linker errors though, will look into that later this week. Great start though!! To be continued

image

dennisameling commented 3 years ago

@jim-signal it's working! Have all three archs for Windows working now:

image

It can be built using make electron TARGET_ARCH=arm64 (x64 is the default)

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 npm test is passing 🚀

It looks like CI / Lints is failing because of the following error, do you have any tips?

Error: Unable to create clippy annotations! Reason: HttpError: Resource not accessible by integration Warning: It seems that this Action is executed from the forked repository. Warning: GitHub Actions are not allowed to create Check annotations, when executed for a forked repos. See https://github.com/actions-rs/clippy-check/issues/2 for details.

Planning to test cross-compilation on Linux and MacOS later today.

dennisameling commented 3 years ago

Linux + MacOS x64 also working after this change 🚀 Reported my findings in https://github.com/signalapp/ringrtc/pull/12#issue-486201934

dennisameling commented 3 years ago

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: https://github.com/signalapp/Signal-Desktop/issues/4461#issuecomment-782869558

Thanks in advance!

ashwaniii commented 3 years ago

Probably this is the bottleneck in Apple Silicon support of Signal. Do we have any updates?

Be-ing commented 3 years ago

@dennisameling there is now a conflict in bin/build-electron

jim-signal commented 3 years ago

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.

dennisameling commented 3 years ago

@jim-signal rebased 👍🏼 also updated the docs at BUILDING.md for the new cross-compilation support.

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 libringrtc binaries I generated: https://github.com/dennisameling/signal-ringrtc-node/tree/cross-compilation-support-new/build/darwin - it seems to work correctly on Apple Silicon now: https://github.com/signalapp/Signal-Desktop/issues/4461#issuecomment-830780121

dennisameling commented 3 years ago

Just found out it's also possible to cross-compile for Linux arm64 and ia32. Needed to do the following for that, similar to https://github.com/signalapp/zkgroup/pull/14:

Then you can run the following to cross-compile for Linux arm64 and ia32 (tested on Ubuntu 20.04 x64):

sudo apt-get update && sudo apt-get install -y crossbuild-essential-arm64 crossbuild-essential-i386
rustup target add aarch64-unknown-linux-gnu i686-unknown-linux-gnu
make electron PLATFORM=unix NODEJS_ARCH=x64
src/webrtc/src/build/linux/sysroot_scripts/install-sysroot.py --arch arm64
make electron PLATFORM=unix NODEJS_ARCH=arm64
make electron PLATFORM=unix NODEJS_ARCH=ia32

Here's the native binaries I generated through this: https://github.com/dennisameling/signal-ringrtc-node/tree/cross-compilation-support-new/build/linux

dennisameling commented 3 years ago

@jrose-signal Alright, as discussed in https://github.com/signalapp/zkgroup/pull/14#issuecomment-832348867 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 neon version bump from 0.7.0 to 0.7.1 is OK as it's needed for folks who want to cross-compile to arm64 on Linux 😊 thanks in advance!

jim-signal commented 3 years ago

This PR has been merged to this commit: 142cbdc43df92afd647bd436ba2a861340e8e9a0

Thank you!