Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Add Node cross-compilation support #14

Closed

Conversation

dennisameling
Copy link
Contributor

@dennisameling dennisameling commented May 1, 2021

Successor of #7, needed for signalapp/Signal-Desktop#3745, signalapp/Signal-Desktop#4461 and signalapp/Signal-Desktop#1636.

  • Allow cross-compiling for NodeJS on Windows, macOS and Linux
  • Bump Rust toolchain to 1.49.0 to have Apple Silicon support
  • Drop armv7 iOS builds since they are no longer supported in Rust 1.49.0. Apple has been accepting 64-bit apps since 2015 and has officially dropped support for 32-bit apps (iPhone 5s and later)
  • Update CI to build releases for all supported architectures (x64, arm64, ia32)
  • Add crossbuild packages for Ubuntu to allow for Linux cross-compilation builds

Supported architectures:

  • Windows:
    • x64
    • ia32
    • arm64
  • Linux:
    • x64
    • ia32
    • arm64
  • MacOS:
    • x64
    • arm64 (Apple Silicon)

Tests & release:

@dennisameling dennisameling force-pushed the node-cross-compilation branch 2 times, most recently from f33b81e to 674d090 Compare May 2, 2021 15:10
@dennisameling dennisameling changed the title [WIP] Add Node cross-compilation support Add Node cross-compilation support May 2, 2021
@dennisameling dennisameling marked this pull request as ready for review May 2, 2021 15:10
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.

I'm hesitant to do too much here since we'd like to move zkgroup into libsignal-client (not-now-but-hopefully-soon). Like RingRTC I think it would still make sense not to publish artifacts for platforms the official client apps don't support (yet?); making sure the build works is one thing but publishing artifacts gives the impression that they're supported.

rust-toolchain Outdated
1.49.0
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to update we may as well update to current stable.

@@ -127,22 +170,35 @@ jobs:
node-version: '${{ steps.node_version.outputs.NODE_VERSION }}'

- name: Build
env:
# Needed until hosted macos-11.0 runners become available
SDKROOT: /Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no good because they might update to 11.2. Is it sufficient to use xcrun -sdk macosx make below? xcrun automatically sets a bunch of environment variables like SDKROOT but I don't know if it'll pick the latest SDK or the host SDK.

Comment on lines 63 to 67
- name: Upload Linux x64
uses: svenstaro/upload-release-action@v1-release
with:
repo_token: ${{ secrets.GITHUB_TOKEN }}
file: ffi/node/libzkgroup.so
asset_name: libzkgroup.so
file: ffi/node/libzkgroup-x64.so
asset_name: libzkgroup-x64.so
tag: ${{ github.ref }}
overwrite: true

- name: Upload Linux ia32
uses: svenstaro/upload-release-action@v1-release
with:
repo_token: ${{ secrets.GITHUB_TOKEN }}
file: ffi/node/libzkgroup-ia32.so
asset_name: libzkgroup-ia32.so
tag: ${{ github.ref }}
overwrite: true

- name: Upload Linux arm64
uses: svenstaro/upload-release-action@v1-release
with:
repo_token: ${{ secrets.GITHUB_TOKEN }}
file: ffi/node/libzkgroup-arm64.so
asset_name: libzkgroup-arm64.so
tag: ${{ github.ref }}
overwrite: true
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 use file_glob: true for these and then we can have a single upload action for all the Linux ones.

@dennisameling
Copy link
Contributor Author

@jrose-signal thanks for reviewing this! You mentioned the following:

Like RingRTC I think it would still make sense not to publish artifacts for platforms the official client apps don't support (yet?)

So does this mean that I should take out Windows arm64/ia32, macOS arm64 and Linux arm64/ia32 from the GH Actions workflow for now? So that we'll only have x64 for now, just like before? That also means that we no longer need the macOS 11 SDK nor do we need the file_glob as you mentioned. It would indeed simplify this PR a lot, but just checking if I understand your remarks correctly! 😊

@jrose-signal
Copy link
Contributor

Yep, I think that's the way to go for now (though let's conclude our conversation on the RingRTC one too). When the Signal team decides to build for a new arch, we'll be ready (and of course you can continue to build unofficial clients), but we don't need to modify the default artifacts just yet.

@dennisameling dennisameling force-pushed the node-cross-compilation branch 2 times, most recently from 8467a3a to 2fd2316 Compare May 6, 2021 20:50
@@ -1,6 +1,13 @@
ZKGROUP_RUST_DIR=../../rust
ZKGROUP_TARGET_DIR=../../target

NODE_ARCH := $(shell node -p "process.arch" || x64)
Copy link
Contributor

Choose a reason for hiding this comment

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

|| echo x64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Just tested locally and indeed needs the echo 👍🏼

Comment on lines 7 to 9
ifdef CARGO_TARGET
CARGO_BUILDFLAGS=--target $(CARGO_TARGET)
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

CARGO_BUILD_TARGET is the standard Rust environment variable for this; might as well use it. (I'm not sure we even need the extra flags if we do.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Was looking how to get the default build target but didn't come across this env variable. Will change accordingly :) indeed don't need the extra flags then

@dennisameling dennisameling force-pushed the node-cross-compilation branch from 2fd2316 to 02c3866 Compare May 6, 2021 21:18
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.

I'm going to hold off on merging this until I have a chance to test it locally, but this looks good. Thank you for pushing this through!

@dennisameling dennisameling force-pushed the node-cross-compilation branch from 02c3866 to 8f72e97 Compare May 6, 2021 22:02
@dennisameling
Copy link
Contributor Author

Just applied your suggestions, indeed simplifies things further. Now one can simply run e.g. make libzkgroup CARGO_BUILD_TARGET=aarch64-pc-windows-msvc NODE_ARCH=arm64 to generate a Windows arm64 dll 👍🏼

@sebdanielsson
Copy link

Will this me merged soon? Can't wait for arm64 support on macOS🤤

@jrose-signal
Copy link
Contributor

Sorry, it's been busier than expected over here, but I haven't forgotten!

@jrose-signal
Copy link
Contributor

For the record, this looks good, and we'll be incorporating it in an upcoming release. I'll comment here when the tag is released!

@jrose-signal
Copy link
Contributor

Included in v0.7.3 as 2dc537e. Thank you again!

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

Successfully merging this pull request may close these issues.

3 participants