-
Notifications
You must be signed in to change notification settings - Fork 614
Add NodeJS_16, with Apple Silicon support #2869
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
Conversation
@MichaelHatherly This was a bit of a shot in the dark and leans entirely on your work, but the checks appear to have succeeded 🤞 . If you have a chance to give this a quick review / any feedback, would be thrilled. :) |
Following NodeJS script by MichaelHatherly, Source here: https://github.com/JuliaPackaging/Yggdrasil/pull/2739/files
Are you able to confirm locally that the built artifact for apple silicon does actually work? |
If we're going to have multiple parallel versions of NodeJS, it'd be better to have them in |
Yep, just did, it works! |
@giordano Think a common script might be overkill, given that the build script itself is 10 lines long and everything else is config / boilerplate. Folder structure is updated. Thoughts? |
I think this is excellent! NodeJSBuilder supports more platforms (see here), could we add the remaining ones here as well? |
Also, could we add NodeJS_14 as well? Would be good to have the current LTS release also supported. I would suggest that for the node 14 build we just repackage the x64 binaries for apple silicon, for node 14 only. |
Ah, didn't see that! |
@davidanthoff added the missing platforms with some caveats: i686 on Linux doesn't seem to be supported as a built node binary: https://github.com/nodesource/distributions#faq |
Ah, yes, this is actually a better reflection of what gets currently packaged in NodeJSBuilder. I think the only platform that I'm packing there that isn't packaged here in this PR yet is linux arm7? |
@davidanthoff oops, failed to push, recheck now |
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.
Fantastic, I'd say we merge!
@jeremiahpslewis if you'd like to PR similar changes, if they're needed, against my nodejs 14 branch then we can sort that one out as well? |
Gladly, it's now done, please see here: https://github.com/MichaelHatherly/Yggdrasil/pull/2ase |
Thanks. |
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.
Can we merge this? Who can actually merge this?
# The products that we will ensure are always built | ||
products = [ | ||
ExecutableProduct("node", :node), | ||
FileProduct("bin/npm", :npm), |
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.
@jeremiahpslewis, sorry for the ping on an old PR, but any reason in particular npm
isn't an ExecutableProduct? instead of FileProduct?
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.
Because that's not a binary executable, but a script which needs an interpreter (node
in this case)
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.
ah, makes sense. so usage would be run('$node $npm ...')
, right?
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.
Yes, or something like that.
Following NodeJS script by @MichaelHatherly,
Source here: https://github.com/JuliaPackaging/Yggdrasil/pull/2739/files