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

enable ARM32/64 cross compilation for Linux in CI #897

Merged
merged 1 commit into from
Jul 9, 2023

Conversation

theofficialgman
Copy link

@theofficialgman theofficialgman commented Jul 7, 2023

closes #895
closes #251

reverts custom dugite commit and uses dugite 2.5.1

Tested on an arm64 buildserver and working with the following commands:

yarn
yarn build:prod
yarn run package

Also working with github actions x86_64 to build arm64 (and armhf, see actions for alternative variables to set)
simplified commands for testing that locally are (assuming you have cross compilers already installed):

export npm_config_arch=arm64
export AS=aarch64-linux-gnu-as
export STRIP=aarch64-linux-gnu-strip
export AR=aarch64-linux-gnu-ar
export CC=aarch64-linux-gnu-gcc
export CPP=aarch64-linux-gnu-cpp
export CXX=aarch64-linux-gnu-g++
export LD=aarch64-linux-gnu-ld
export FC=aarch64-linux-gnu-gfortran
export PKG_CONFIG_PATH=/usr/lib/aarch64-linux-gnu/pkgconfig
yarn
yarn build:prod
yarn run package

@theofficialgman theofficialgman marked this pull request as draft July 7, 2023 16:51
@theofficialgman
Copy link
Author

theofficialgman commented Jul 7, 2023

getting closer on cross compilation support
a couple of npm packages are still acting badly and don't appear to support cross compiling (they build as x86_64)

fs_admin.node:                         ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, BuildID[sha1]=aa032c2ad45bc848811f4cdc2ca82b19afcc4e78, stripped
desktop-trampoline/desktop-trampoline: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=4295580279c509a5e8a0fdb5312437d9a55c341e, for GNU/Linux 3.2.0, not stripped
keytar.node:                           ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, BuildID[sha1]=b32a8201f935cd58964bef1ca54216aed71abed2, stripped
desktop-notifications.node:            ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, BuildID[sha1]=eaa58f98a44a35f5cba1b1605ea3d678f56c5efd, not stripped

on native arm64 buildserver these files are getting built as aarch64

edit: no longer an issue

@theofficialgman theofficialgman changed the title Use dugite 2.5.1 Use dugite 2.5.1 and correct and enable cross compilation Jul 7, 2023
@theofficialgman
Copy link
Author

theofficialgman commented Jul 7, 2023

I found the issue, node-gyp is getting called due to there not being a binary available for the platform selected (eg: arm64) and node-gyp does not support cross compilation. it simply uses the default GCC/G++ nodejs/node-gyp#829

they closed the issue because they are too lazy to implement it.....

@theofficialgman
Copy link
Author

the way to get around this is to explicitly pass the --arch variable to node-gyp. but that requires that the problematic packages (desktop-trampoline https://github.com/desktop/desktop-trampoline/blob/734fbaeffd848ef61c0519c6ff88a4123aca54dc/package.json#L14 and desktop-notifications be corrected https://github.com/desktop/desktop-notifications/blob/41d50b6f85e1689f3edc38549613936ad05e2501/package.json#L8 )

@theofficialgman
Copy link
Author

theofficialgman commented Jul 7, 2023

oh wonderful that explains why --arch doesn't work in node-gyp... its only an option for windows, not documented anywhere
https://github.com/nodejs/node-gyp/blob/53c99ae573bd5a5435e843b7de6b2e684f4de4d3/lib/build.js#L137-L155

this is why I hate node/npm/electron

edit: workaround implemented here 1b40966

@theofficialgman
Copy link
Author

@shiftkey this PR is final. I am not sure how you want to handle uploading releases from two separate job matrices so I will let you handle that.

@theofficialgman theofficialgman changed the title Use dugite 2.5.1 and correct and enable cross compilation Use dugite 2.5.1 and correct and enable ARM cross compilation Jul 7, 2023
@theofficialgman
Copy link
Author

binaries/packages tested on arm64 and working well

@shiftkey
Copy link
Owner

shiftkey commented Jul 8, 2023

@theofficialgman thanks for digging into this - I'm going to break this up into a few branches to make the changes easier to review and test:

  • update dugite to new version and drop workarounds ✅
  • split build and publish into two separate jobs (following the pattern from dugite-native to upload artifacts)
  • update CI to add ARM support

From then we can ship an early release for Linux (I won't publish to apt or rpm feeds) with the ARM binaries available for verification. The upstream desktop package might not be available for a couple of weeks, but once they cut the release it'll be easy for us to shift these changes over and get things working. It'll also be a chance for me to tidy up the history of commits that have built up over the years...

I am not sure how you want to handle uploading releases from two separate job matrices so I will let you handle that.

All good, I'll handle that.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
permissions:
contents: write
strategy:
fail-fast: false
matrix:
node: [18.14.0]
node: [18.16.0]
Copy link
Owner

Choose a reason for hiding this comment

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

I'm curious about this version bump here - do you remember what this helped us with?

Copy link
Author

Choose a reason for hiding this comment

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

I just version bumped so that windows and linux versions were close together. they are still part of the stable (ie: no api breakage) node series

Copy link
Author

Choose a reason for hiding this comment

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

seems to be a part of the electron bump desktop#16624

Copy link
Owner

Choose a reason for hiding this comment

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

The bit that I'm curious about here was the minor version bump in this PR (from 18.14 to 18.16) - how does this help us out?

Copy link
Owner

Choose a reason for hiding this comment

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

This appears to be the only version with the linux-x64-glibc-217 version of Node. Not sure how sustainable this will be in the long-term, mostly wanted to understand the version shift here.

Copy link
Author

Choose a reason for hiding this comment

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

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
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.

pre-requisites for experimental ARM build
3 participants