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 support for Apple ARM64 (aka Apple Silicon) #2500

Merged
merged 7 commits into from
Feb 26, 2021

Conversation

Ch00k
Copy link
Contributor

@Ch00k Ch00k commented Feb 22, 2021

This adds support for building the app on Apple Silicon.

The frontend change is the most annoying part here. Due to the fact that grpc-tools refuses to support ARM architecture the management interface proto files need to be pre-built on a non-ARM platform. Then these proto files can be used during the main build. I added a helper script build_mi_proto.sh to help in pre-building the proto files.

On the backend only bumping of a couple of versions is needed. For pfctl-rs I created an issue requesting a new release. For rust-tun I made a PR that bumps ioctl-sys. UPD: Both have been released on crates.io

Due to the nature of changes in Cargo.toml this PR should probably not be merged as is. Nevertheless I think it gives a good overview of the changes required to support Apple Silicon (and probably the rest of the ARM platforms, too).

I tested this by building a package on with ./build.sh --dev-build on an M1 Mac Mini.


This change is Reviewable

Copy link
Member

@faern faern 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 this! We were actually looking at this internally, but had no code ready yet. We'll take a look at this and compare with what we had and see what fits together.

talpid-core/Cargo.toml Outdated Show resolved Hide resolved
@faern faern requested a review from raksooo February 22, 2021 19:56
@faern
Copy link
Member

faern commented Feb 23, 2021

Thanks a bunch for this! We will look at it during the week and come back with a more thorough review.

Do you know if this can be cross compiled from an Intel mac, or does it have to be built on an M1? Maybe the other way around works? The main question is whether or not we need two build servers or can produce all artefacts on a single machine.

@Ch00k
Copy link
Contributor Author

Ch00k commented Feb 23, 2021

It is probably possible, but I have not looked into it unfortunately (my target was to build the app on Apple Silicon for Apple Silicon). Quick search shows thatelectron-builder seems to support universal builds as per electron-userland/electron-builder#5481 and https://github.com/electron/universal.
Not sure if Rust and Go code can be cross-compiled though.

@Ch00k
Copy link
Contributor Author

Ch00k commented Feb 23, 2021

Building amd64 on arm64 could theoretically be possible with Rosetta 🤔

Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Thanks, great work! The code looks nice and it seems to run pretty well. I've left a few comments that would be nice if they were fixed before merging.

This is a great step in the directions of releasing M1 native/universal builds but there are a few things that needs to be resolved before we can release it to customers. The remaining things are:

  • OpenVPN and Shadowsocks x86 binaries are still bundled and those protocols can therefore not be used in the M1 build.
  • We don't have a M1 build-server yet (only devices for testing). We either have to cross-compile it or set up a new build-server.
  • We would probably like to produce a universal binary before releasing it to customers.

We'll continue working on those remaining issues!

build_mi_proto.sh Outdated Show resolved Hide resolved
gui/package.json Show resolved Hide resolved
README.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@faern
Copy link
Member

faern commented Feb 25, 2021

@Ch00k Just to clarify the above. We don't expect you to solve OpenVPN or universal binaries etc in this PR. We can merge this after the smaller code attached comments have been fixed. Then we'll continue from there and integrate it into our real build process.

README.md Outdated Show resolved Hide resolved
@raksooo
Copy link
Member

raksooo commented Feb 25, 2021

When testing this out I found that it fails to run the build-proto.sh script on Linux and Windows which makes the entire build fail. This is a result of moving grpc-tools to optionalDependencies. I've been looking into this and might know how to solve it but will have to do some more testing on an M1 mac as well, which I'll do tomorrow.

@Ch00k
Copy link
Contributor Author

Ch00k commented Feb 25, 2021

@raksooo weird, I thought I tested the build on Linux, and it worked for me. I'll test it again. Can't test on Windows though because I don't have access to a Windows box.

UPD Ack, it does fail on Linux indeed :(

[15:42:38] 'build-proto' errored after 31 ms
[15:42:38] Error: Command failed: bash ./scripts/build-proto.sh
./scripts/build-proto.sh: line 37: /home/ay/projects/personal/mullvadvpn-app/gui/node_modules/.bin/grpc_tools_node_protoc: No such file or directory

UPD 2 But then how come I am able to build the proto files on Linux to use them on M1 Mac afterwards? I am running the build_mi_proto.sh, which in its turn runs npm ci, which is what the build.sh does too. But build.sh then fails to find grpc_tools_node_protoc. I am confused... 🤔

UPD 3 Stupid me :) Of course, build.sh has this:

# Add `--no-optional` flag when running on non-macOS environments because `npm ci` attempts to
# install optional dependencies that aren't even available on other platforms.
NPM_CI_ARGS=""
if [ "$(uname -s)" != "Darwin" ]; then
    NPM_CI_ARGS+="--no-optional"
fi

As per the doc optionalDependencies are used when you want to continue with the build even if a dependency installation fails (which is our case with grpc-tools and nseventmonitor). So perhaps we could let npm handle it and remove the entire --no-optional condition ?

Sorry for all this written thought process. I am learning Node while working on this PR 😄

@raksooo
Copy link
Member

raksooo commented Feb 26, 2021

@Ch00k Yeah, the comment makes it sound like the use of the --no-optional argument was a workaround for a bug in npm or something. I did some testing and it seems to work as expected without --no-optional on Linux and Windows. I'll have to do some more testing but I agree that removing it probably is the best approach.

@raksooo
Copy link
Member

raksooo commented Feb 26, 2021

@Ch00k Done testing without --no-optional. Please go ahead and remove it 🙂

.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉 Thanks for fixing the comments, and thanks for the contribution! I'll merge this later today.

@Ch00k
Copy link
Contributor Author

Ch00k commented Feb 26, 2021

I just built the app end-to-end one more time, just to make sure everything still works. It does.

@raksooo raksooo merged commit ae3dfbc into mullvad:master Feb 26, 2021
@raksooo
Copy link
Member

raksooo commented Feb 26, 2021

👍 Thanks for rebasing. I've updated package-lock.json as well.

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

Successfully merging this pull request may close these issues.

3 participants