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

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

Conversation

dennisameling
Copy link
Contributor

@dennisameling dennisameling commented Sep 13, 2020

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 are x64, ia32, arm64 (in line with NodeJS' archs), default is x64.

Tested on:

  • darwin x64 (build + yarn test)
  • darwin arm64 (build)
  • windows x64 (build + yarn test)
  • windows arm64 (build + yarn test)
  • windows ia32 (build)
  • linux x64 (build + yarn test)
  • linux arm64 (build)
  • linux ia32 (build)

MacOS (darwin) arm64 / Apple Silicon

MacOS (darwin) arm64 doesn't seem to work yet FIXED ✔️: #12 (comment)

@jim-signal
Copy link
Contributor

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
Copy link
Contributor Author

dennisameling commented Sep 19, 2020

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 🚀

@dennisameling dennisameling changed the title [WIP] Add Windows Electron multi-arch support [WIP] Add Electron multi-arch/cross-compilation support Nov 16, 2020
Makefile Outdated Show resolved Hide resolved
@jim-signal
Copy link
Contributor

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

@dennisameling
Copy link
Contributor Author

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

@dennisameling
Copy link
Contributor Author

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 dennisameling changed the title [WIP] Add Electron multi-arch/cross-compilation support Add Electron multi-arch/cross-compilation support Feb 11, 2021
@dennisameling dennisameling reopened this Feb 11, 2021
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
Copy link
Contributor Author

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

@dennisameling
Copy link
Contributor Author

@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 actions-rs/clippy-check#2 for details.

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

@dennisameling dennisameling marked this pull request as ready for review February 11, 2021 16:17
@dennisameling
Copy link
Contributor Author

Linux + MacOS x64 also working after this change 🚀 Reported my findings in #12 (comment)

@dennisameling dennisameling force-pushed the add-windows-multi-arch-support branch 2 times, most recently from bf69c09 to d62f1a3 Compare February 13, 2021 09:39
@dennisameling
Copy link
Contributor Author

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!

@ashwaniii
Copy link

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

@Be-ing
Copy link

Be-ing commented Apr 27, 2021

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

@jim-signal
Copy link
Contributor

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 dennisameling force-pushed the add-windows-multi-arch-support branch 2 times, most recently from b84b13b to 5c76fd6 Compare May 1, 2021 19:43
@dennisameling
Copy link
Contributor Author

dennisameling commented May 1, 2021

@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: signalapp/Signal-Desktop#4461 (comment)

@dennisameling dennisameling force-pushed the add-windows-multi-arch-support branch from 5c76fd6 to 2734a92 Compare May 3, 2021 13:40
@dennisameling
Copy link
Contributor Author

dennisameling commented May 3, 2021

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

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

[target.aarch64-unknown-linux-gnu]
linker = "aarch64-linux-gnu-gcc"
[target.i686-unknown-linux-gnu]
linker = "i686-linux-gnu-gcc"
Copy link
Contributor

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

Copy link
Contributor Author

@dennisameling dennisameling May 4, 2021

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?

Copy link
Contributor

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?

@dennisameling dennisameling force-pushed the add-windows-multi-arch-support branch from 2734a92 to 92b9f14 Compare May 6, 2021 20:24
@dennisameling
Copy link
Contributor Author

dennisameling commented May 6, 2021

@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 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!

Copy link
Contributor

@jim-signal jim-signal left a 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!

@jim-signal
Copy link
Contributor

This PR has been merged to this commit: 142cbdc

Thank you!

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.

5 participants