-
Notifications
You must be signed in to change notification settings - Fork 521
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
Conversation
getting closer on cross compilation support
on native arm64 buildserver these files are getting built as aarch64 edit: no longer an issue |
I found the issue, they closed the issue because they are too lazy to implement it..... |
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 ) |
oh wonderful that explains why --arch doesn't work in node-gyp... its only an option for windows, not documented anywhere this is why I hate node/npm/electron edit: workaround implemented here 1b40966 |
ebd55f0
to
4bc4bd8
Compare
@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. |
a4356c2
to
5afd34c
Compare
binaries/packages tested on arm64 and working well |
@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:
From then we can ship an early release for Linux (I won't publish to
All good, I'll handle that. |
ea263aa
to
4abbff7
Compare
91eddc1
to
1d3d156
Compare
.github/workflows/ci.yml
Outdated
permissions: | ||
contents: write | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
node: [18.14.0] | ||
node: [18.16.0] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #897 (comment)
7b66643
to
35949fa
Compare
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:
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):