-
Notifications
You must be signed in to change notification settings - Fork 522
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 workflow for ARM on Ubuntu and see what breaks #895
Conversation
Updated the "About GitHub Desktop" model to remove the button to check for updates (since it didn't do anything on Linux) and replace with a link to the linux releases page
Reduce errors produced in terminal from debian package installations by testing for existence of symlink prior to executing unlink
There already exists a function that will convert a tilde path to an absolute path. It was originally used for this purpose, but the functionality was removed during a commit that changed which function was used to validate git repositories. This reinstates that functionality and allows us to type a ~/ tilde path to get our home directories when typing in a path.
* drop explicit window icon on Linux * drop old icon used in app window * regenerate smaller icons from new 256px source
…ectory (#837) * add patch-package so we can patch a node_modules package * generate patch to disable build-id contents for RPM package * move patch-package command to post-install script * regenerate patch file
* Bump electron-installer-redhat from 3.3.0 to 3.4.0 Dependabot couldn't find the original pull request head commit, 6b63cfc. * regenerate patch file --------- Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Brendan Forster <github@brendanforster.com>
* patch dugite with forked package that contains Linux-specific build fixes * update script to point to new package * upgrade to version that more closely matches next upstream release
Dependabot couldn't find the original pull request head commit, 5a508da. Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Update linux.ts Fix #885 for VSCode, VSCodium, and VSCode Insiders * Use full path for Flatpak binaries Adds full path, as previous method didn't function properly.
fa4d3a9
to
322875b
Compare
@theofficialgman I know you've spent a lot of time in this area recently, so I'm curious if you have any thoughts on things that I should be trying out here before I try and include an ARM-based package in a release generated from CI. Also, thanks for contributing to the Git packaging parts - this wouldn't have been possible without your sustained effort there... |
const targetArch = process.env.TARGET_ARCH | ||
if (targetArch === 'arm64') { | ||
return 'arm64' | ||
} |
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.
you shouldn't need to do this. see the above lines for setting npm_config_arch
the function should already return with that having been set
function getArchitecture() { | ||
switch (process.arch) { | ||
function getArchitecture(targetArch?: string) { | ||
switch (targetArch) { |
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 should probably be changed to be more like getDistArchitecture
. there is no need to set TARGET_ARCH
and npm_config_arch
when they both convey the same information. TARGET_ARCH
is a variable internal to the dugite-native CI buildscripts. nothing is expecting that here.
basically these variables ( npm_config_arch
and npm_config_target_arch
) are names recognized by node-gyp which is used to build native node modules (and used by electron) as well as other node programs. technically both should be set for highest compatibility
https://www.electronjs.org/docs/latest/tutorial/using-native-node-modules#using-npm https://github.com/nodejs/node-gyp
ie:
function getArchitecture() {
// If a specific npm_config_arch is set, we use that one instead of the OS arch (to support cross compilation)
if (process.env.npm_config_arch === 'arm64' ) {
return '--arm64'
}
if (process.env.npm_config_arch === 'x64' ) {
return '--x64'
}
if ( process.env.npm_config_arch === 'armv7l' ) {
return '--armv7l'
}
// If no specific npm_config_arch is set, use current process.arch
switch (process.arch) {
case 'arm64':
return '--arm64'
case 'arm':
return '--armv7l'
default:
return '--x64'
}
}
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.
although tenically you don't need to set npm_config_arch
or npm_config_target_arch
at all if you just call electron-builder
with the appropriate --arm64, --armv7l, --x64 (which is what this script does) since electron-builder sets these variables for you based on the --arm64, --armv7l, --x64 input
https://github.com/electron-userland/electron-builder/blob/c1cc2e9c24784d05e14ca292adc2452e9c0237f6/packages/app-builder-lib/src/util/yarn.ts#L43-L48
the problem is I think (from remembering your scripts from memory) that you build with yarn directly and only use electron-builder to package the application basically. so you would need to set npm_config_arch
or npm_config_target_arch
yourself for any native packages that yarn (which invokes npm) builds.
ea263aa
to
4abbff7
Compare
Shifting over to #897 |
Exploring the lift required to have CI run tests and generate binaries for ARM on Ubuntu