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 workflow for ARM on Ubuntu and see what breaks #895

Closed
wants to merge 73 commits into from

Conversation

shiftkey
Copy link
Owner

@shiftkey shiftkey commented Jul 3, 2023

Exploring the lift required to have CI run tests and generate binaries for ARM on Ubuntu

shiftkey and others added 30 commits July 3, 2023 12:22
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
shiftkey and others added 18 commits July 3, 2023 12:36
…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.
@shiftkey shiftkey force-pushed the enable-arm-building-for-linux branch from fa4d3a9 to 322875b Compare July 3, 2023 20:59
@shiftkey
Copy link
Owner Author

shiftkey commented Jul 4, 2023

@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...

Comment on lines +163 to +166
const targetArch = process.env.TARGET_ARCH
if (targetArch === 'arm64') {
return 'arm64'
}

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) {

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'
  }
}

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.

@shiftkey
Copy link
Owner Author

shiftkey commented Jul 8, 2023

Shifting over to #897

@shiftkey shiftkey closed this Jul 8, 2023
@shiftkey shiftkey deleted the enable-arm-building-for-linux branch August 26, 2023 15:25
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.