feat: Apple Silicon support #312
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #312 +/- ##
===========================================
+ Coverage 93.17% 93.33% +0.16%
===========================================
Files 43 43
Lines 908 930 +22
Branches 95 97 +2
===========================================
+ Hits 846 868 +22
Misses 41 41
Partials 21 21
Continue to review full report at Codecov.
|
|
There are probably more ways in which this PR could be cleaned up and/or improved. However, I think it would be a good idea to solicit review at this point before putting too much more work into it, to see if this is going in a reasonable direction. |
|
Hi @johnwchadwick , thanks for this! I will try to review it this weekend |
johnwchadwick
force-pushed
the
m1-support
branch
from
71c9587
to
90df103
Compare
johnwchadwick
changed the title
johnwchadwick
force-pushed
the
m1-support
branch
2 times, most recently
from
ca685b5
to
a768f97
Compare
binding.gyp
Outdated
| 'OTHER_LDFLAGS': [ | ||
| # HACK: -framework CoreFoundation appears twice, but CoreFoundation is a singleton | ||
| # because it doesn't start with a -. We need to remove one of the instances of | ||
| # -framework CoreFoundation or GYP will break our args. | ||
| '-static', | ||
| '<!@(<(curl_config_bin) --static-libs | sed "s/-framework CoreFoundation//")', | ||
| ], |
There was a problem hiding this comment.
Thanks for fixing this! I have stolen your workaround and merged it into a different PR as I had to fix the CI pipelines. 😄
There was a problem hiding this comment.
Excellent, I shall rebase.
scripts/ci/build-openssl.sh
Outdated
|
|
||
| build_arch x86_64 | ||
| make distclean || true; | ||
| build_arch arm64 |
There was a problem hiding this comment.
This line here is not working for me on Node.js 8 and 10, I believe it would also give an error with Node.js 12 but I have not tested it.
It seems that the OpenSSL version shipped with those Node.js versions does not have the build target darwin64-arm64-cc . Could you take a look at that?
To make things easier I have enabled more tests to run in the CI for PRs, so if you rebase your changes on top of master develop you should get a build matrix that tests against multiple versions of Node.js and Electron on macOS.
As this changes the build steps for the libcurl dependencies, please make sure you bump the versioning for the libcurl-deps cache in the build-lint-test and build-and-release workflows (you can find all of them by searching for libcurl-deps-cache inside https://github.com/JCMais/node-libcurl/tree/develop/.github/workflows).
There was a problem hiding this comment.
I see. I don't think Node.js 8 or 10 support Apple Silicon; I probably need to adjust this to not attempt to build a universal binary when it isn't possible. That having been said, I've been testing on Node 12, so at least on new enough Node 12 releases it should be OK.
There was a problem hiding this comment.
Awesome! Btw I meant develop above, not master, but it seems you got it. Sorry for that haha 😓
johnwchadwick
force-pushed
the
m1-support
branch
6 times, most recently
from
dd7ca2d
to
88985f6
Compare
|
fyi I just pushed a new commit to |
johnwchadwick
force-pushed
the
m1-support
branch
from
88985f6
to
3df2b8c
Compare
|
I think I've made most of the necessary changes now. I am not sure if openssl will wind up being the only limiting factor for universal builds, but now it is only attempted (by default) to do a universal build if openssl is >= 1.1.1i. I think this is a reasonable heuristic, but there might be other factors that should limit universal builds. That said, it sure seems like tests pass in CI with Node.js 10, which appears to be picked up as universal, so maybe some patch releases of node 10 also work. |
|
While universal build support is nice to have, I'm currently second-guessing if it is the right approach for the pre-built packages. For Electron, it appears electron-builder will lipo the native addons on its own, and as far as I can tell, node-pre-gyp is just not aware of universal builds at all. This isn't too big of a deal, since most of the same things apply for cross-compiling that apply for universal builds, so realistically switching between the two approaches shouldn't be too hard. |
johnwchadwick
force-pushed
the
m1-support
branch
from
3bc7b1f
to
8265d18
Compare
|
OK, scratch that. It's a bit hard for me to test, but I think I've finally landed on the best solution we can realistically get. It is possible to cross compile each target individually, but this ended up being a PITA, since the non-universal binaries can only be tested in the case where target_arch == host_arch. After a long spike, I stashed the effort to build separately on ARM64 and have instead simply modified the universal build to publish two separate packages, using |
|
@johnwchadwick I was not able to review the recent changes this weekend, but I plan to do so this following week/weekend. Regarding your comment about publishing the binary twice, that seems fine to me. |
|
Going to merge this and then create a pre-release version the pre-release version will be created this later week, as I had some errors in the CI during the weekend, I am going to fix them all during the week. |
|
it took a while, but the pre-release version has been finally released: |
The goal of this PR is to get node-libcurl building properly for Apple silicon. Universal builds seem like they would be ideal, so that is my current goal. Unfortunately, this adds a decent amount of complexity to the build scripts and will necessarily increase build times :( In addition, some workaround may be needed for node-pre-gyp to be able to fetch the universal binaries.
We're working on this in the process of trying to resolve Kong/insomnia#2964.