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

feat: add arm64 build support #2344

Closed
wants to merge 32 commits into from
Closed

feat: add arm64 build support #2344

wants to merge 32 commits into from

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Dec 10, 2020

This will enable the build for arm64 via npm run build:arm64, fixes #2342

Most changes are made for Apple Silicon as upstream hard-coded Mac targets as x86_64, linux_arm64 should just work.

Known issues:

@gnattu
Copy link
Member Author

gnattu commented Dec 10, 2020

dmg creation bug submitted to upstream: electron-userland/electron-builder#5465

@gnattu gnattu self-assigned this Dec 12, 2020
app.js Outdated Show resolved Hide resolved
@@ -48,13 +46,16 @@ export class ResolutionConfig extends Component {
(this.props.isolateGameWindow ? this.props.webview.windowWidth : this.props.webview.width) *
this.props.zoomLevel,
),
...getMinArea(screen.getAllDisplays()),
...getMinArea(ipcRenderer.sendSync('get-all-displays')),
Copy link
Member

Choose a reason for hiding this comment

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

I think we may change to use async calls here, but this could be done later

app.js Outdated Show resolved Hide resolved
@KagamiChan
Copy link
Member

I think we still need to wait for the support from electron-builder side
in the meantime the upgrade to next version of electron is good and could be done first

"start": "electron .",
"start_debug": "electron . --dev --inspect",
"dev": "electron . --dev --inspect",
"clean": "gulp clean",
"lint:js": "eslint . --ext .es --ext .js --ext .ts --ext .tsx --ignore-path .gitignore",
"lint:css": "stylelint views/**/*.css assets/**/*.css --config .stylelintrc.css.js",
"lint:styled": "stylelint views/**/*.es --config .stylelintrc.js",
"lint": "npm run lint:js && npm run lint:css && npm run lint:styled"
"lint": "npm run lint:js && npm run lint:css && npm run lint:styled",
"postinstall": "patch-package"
Copy link
Member

@KagamiChan KagamiChan Dec 13, 2020

Choose a reason for hiding this comment

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

This npm script will run for each npm install

since we don't always build the app, there's no need to patch every time.I think we can call it before we start to build

@gnattu
Copy link
Member Author

gnattu commented Dec 14, 2020

I think we still need to wait for the support from electron-builder side

Exactly, it's why this is only a draft. I think we should wait until electron-userland/electron-builder#5426 is merged.

However, arm64 support for Windows and Linux is merged at least one year ago and still having serious issues. I think patching upstream will be necessary for a long time to enable correct building.

@gnattu
Copy link
Member Author

gnattu commented Dec 14, 2020

Now the upstream merged the PR and released a beta version, but that version breaks npm install on non-macOS platforms as it introduced a new dependency that is not compatible with Windows and Linux. The fix is already in master but we have to wait for next release to begin the work to make the arm64 builds as default targets.

Two limitations mentioned in this PR are hard to resolve at the moment:

  • We have to build linux-arm64 targets on an x86 machine, this is not a big deal for us as our CI is on x86.

  • nsis does not work for x86+arm mixed, we have to either build two nsis targets by changing the current building process(which will also affect auto update), or not providing nsis for arm64 at all until upstream figured out how to provide a proper nsis script for such configuration.

Another side note:

I have not tested to build mac-arm64 targets on macOS < 11 yet and I cannot guarantee it will work. It may fail on our CI which is currently on 10.15

@gnattu gnattu marked this pull request as ready for review December 20, 2020 11:14
@gnattu gnattu changed the title [WIP] feat: add arm64 build support feat: add arm64 build support Dec 20, 2020
@gnattu
Copy link
Member Author

gnattu commented Dec 20, 2020

CI failure is not related to the changes:

Error uploading artifact the storage: The underlying connection was closed: An unexpected error occurred on a receive.

arch:
- arm64
nsis:
artifactName: 'poi-setup-${version}-arm64.${ext}'

Choose a reason for hiding this comment

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

This adds arm64 suffix to the NSIS file name. But electron-builder also creates a latest.yml to use with electron-updater. The name is latest.yml for both x64 and arm64 so they end up overwriting each other. Any solution?

Choose a reason for hiding this comment

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

I managed to patch this. You can apply this patch to poi as well: https://github.com/webcatalog/webcatalog-app/pull/1253/files

.gitignore Outdated Show resolved Hide resolved
@KagamiChan
Copy link
Member

seems upstream electron-builder has released the support for apple silicon, we can start the code cleanup work and land the new version of electron asap

@KagamiChan
Copy link
Member

if not possible I'd prefer picking up the electron 11 support first, electron 11 and apple silicon are 2 things IMO

@KochiyaOcean
Copy link
Member

If the blocker is Linux arm64, I think we can merge it and deliver Windows arm64 &macOS arm64 first

@KagamiChan KagamiChan force-pushed the master branch 2 times, most recently from 277793c to f504e04 Compare February 17, 2021 08:46
@KagamiChan
Copy link
Member

Seems this could be closed?

@KagamiChan KagamiChan closed this Nov 20, 2021
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.

4 participants