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

Jump to bottom

Build and package ARM releases #3896

Closed
wants to merge 2 commits into from
Closed

Build and package ARM releases #3896

wants to merge 2 commits into from

Conversation

ab77
Copy link
Contributor

@ab77 ab77 commented Dec 1, 2022 •
edited

@ab77

This comment was marked as outdated.

Sign in to view
@ab77

This comment was marked as outdated.

Sign in to view
@aethernet
Copy link
Contributor

aethernet commented Dec 2, 2022 •
edited

@ab77 i had no issue building direct-io for arm64 on my m1 with node-gyp, are we trying to cross-compile here ?

@ab77
Copy link
Contributor Author

ab77 commented Dec 2, 2022

@ab77 i had no issue building direct-io for arm64 on my m1 with node-gyp, are we trying to cross-compile here ?

Yes, cross compiling. No native ARM hardware available in this context.

@thedocbwarren
Copy link

Does this build with an DMG now? I've been able to build the code but no DMG is made. I can publish a community build if a DMG is built.

@ab77
Copy link
Contributor Author

ab77 commented Dec 5, 2022

Does this build with an DMG now? I've been able to build the code but no DMG is made. I can publish a community build if a DMG is built.

unfortunately not, see #3896 (comment)

@ab77 ab77 force-pushed the ab77/arm-builds branch 8 times, most recently from 5077e19 to a8b4a8b Compare December 28, 2022 19:05
@mcraa
Copy link
Contributor

mcraa commented Dec 28, 2022

We had the ext2fs error in previous PRs and should be solved on master already, I don't know why is it coming back here.
The node-gyp error on windows is new for me, found quite some issues mentioning it for example
nodejs/node-gyp#1371

@ab77 ab77 requested review from aethernet and mcraa December 29, 2022 01:05
mcraa
mcraa reviewed Dec 29, 2022
View reviewed changes
package.json Show resolved Hide resolved
@ab77
Copy link
Contributor Author

ab77 commented Dec 30, 2022

We had the ext2fs error in previous PRs and should be solved on master already, I don't know why is it coming back here.

workaround by setting strict: false. The real fix is to maybe figure out why a replacement isn't being made?

The node-gyp error on windows is new for me, found quite some issues mentioning it for example nodejs/node-gyp#1371

Maybe because we are trying to bundle drivelist v9.2.x, which is ~ 2-4 years old and build/re-build under node 16?

@mcraa
Copy link
Contributor

mcraa commented Dec 30, 2022

We had the ext2fs error in previous PRs and should be solved on master already, I don't know why is it coming back here.

workaround by setting strict: false. The real fix is to maybe figure out why a replacement isn't being made?

to remove the reliance on node stuff like binding and the node_modules folder which are not available in the packaged electron environment

The node-gyp error on windows is new for me, found quite some issues mentioning it for example nodejs/node-gyp#1371

Maybe because we are trying to bundle drivelist v9.2.x, which is ~ 2-4 years old and build/re-build under node 16?

then it should fail on the other two platforms too

@ab77
Copy link
Contributor Author

ab77 commented Dec 31, 2022

then it should fail on the other two platforms too

Sure, it should, but it doesn't. Maybe Windows is special. Maybe trying to build code years out of date with modern tools is the problem. Maybe both - I have no idea. I am ignoring this issue for now and hoping by the time I cycle back to it, all the dependencies are bumped up and it just builds.

@ab77
Copy link
Contributor Author

ab77 commented Dec 31, 2022 •
edited

to remove the reliance on node stuff like binding and the node_modules folder which are not available in the packaged electron environment

I am afraid I don't understand what that means. One problem I found is that fetchWasm() throws an error with with string-replace-loader { strict: true }. I've not dug into it, but from reading the docs, this would happen if no replacement is made, so to catch no-ops I am guessing.

@ab77 ab77 force-pushed the ab77/arm-builds branch 2 times, most recently from e86f12b to 5422397 Compare January 3, 2023 17:25
mcraa
mcraa reviewed Jan 3, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@ab77 ab77 force-pushed the ab77/arm-builds branch 12 times, most recently from 5812262 to 0b8447b Compare January 4, 2023 20:26
mcraa
mcraa reviewed Jan 5, 2023
View reviewed changes
package.json Outdated
@@ -21,17 +21,18 @@
"lint-css": "prettier --write lib/**/*.css",
"lint-ts": "balena-lint --fix --typescript typings lib tests scripts/clean-shrinkwrap.ts webpack.config.ts",
"lint": "npm run lint-ts && npm run lint-css",
"postinstall": "electron-rebuild -t prod,dev,optional",
"preinstall": "bash scripts/ci/preinstall.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

Add cross-env or similar instead of bash.
On line 17 flowzone-preinstall-linux still has electron-builder.yml but that file is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add cross-env or similar instead of bash.
https://github.com/kentcdodds/cross-env is archived and the package is not gettign any support. Do we really need to use it? My preference would be not to include depracated packages if at all possible, give we already have a ton of those. what does cross-env give us over bash, if bash is standardised across platforms?

On line 17 flowzone-preinstall-linux still has electron-builder.yml but that file is removed.
Cheers, fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is why I wrote "cross-env or similar"
however from the author:

So I'm declaring cross-env as finished. No new features will be added (and old features will not likely be removed). I don't plan to do any more than fix security/critical bugs and keep the Node.js support up-to-date.

It still looks like a safe bet.
bash is not available out of the box on many platforms, with a package like this we don't need to write if windows then cmd if linux then bash but only if not zsh, etc and so when a new contributor tries to run npm i it will just run even if they don't have bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am 100% against this mate, mostly for ideological reasons of not introducing any more deprecated packages. Surely we can have a maintainer guide that says "you must install {{shell}} to work on this project".

@ab77 ab77 force-pushed the ab77/arm-builds branch 8 times, most recently from 562d564 to c62ffbe Compare January 5, 2023 20:50
@ab77 ab77 requested a review from mcraa January 5, 2023 23:16
@ab77 ab77 marked this pull request as ready for review January 6, 2023 00:05
@ab77 ab77 marked this pull request as draft January 6, 2023 01:21
@dfunckt
Copy link
Member

dfunckt commented Dec 22, 2023

Superseded by #4132

@dfunckt dfunckt closed this Dec 22, 2023
@dfunckt dfunckt deleted the ab77/arm-builds branch December 22, 2023 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aethernet aethernet Awaiting requested review from aethernet

@mcraa mcraa Awaiting requested review from mcraa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ab77 @aethernet @thedocbwarren @mcraa @dfunckt