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

Fetch Native Windows on ARM Node builds for Node 20+ #1567

Merged
merged 2 commits into from
May 13, 2024

Conversation

iNViTiON
Copy link
Contributor

@iNViTiON iNViTiON commented Nov 6, 2023

Relate #1270

Info

  • With the release of Node v20, there are now pre-built binaries available for Windows on ARM devices.
  • Where possible, we should fetch and use those binaries, so that users get the best possible experience.
  • However, for older versions of Node, we will need to continue to fetch the x64 binaries and rely on emulator.

Changes

  • Updated the constants section in tool/node/mod.rs to include the constant values for Windows on ARM architecture
  • Ensured that the parsing of the files metadata in the Node index will check for both the native and x64 versions of Node.
  • Customized archive_filename on Windows on ARM builds to check the Node major version and fetch either a native or x64 build of Node depending on whether it is greater than or equal to 20.
  • Change test_node_archive_basename and test_node_archive_filename from version 1.2.3 to 21.2.3 to allow Windows on ARM and Apple Silicon to fetch correct version without constraint

Tested

  • Our test suite currently doesn't execute on a Windows on ARM machine, so we don't have a way to automatically test these changes.
  • I run a full test with cargo test --all and everything pass. Also confirm this properly fetches and run Node 20+ and 19-.

@rwjblue
Copy link
Contributor

rwjblue commented Nov 14, 2023

I think the release builds are also generated from this repo, so I think you'd need to tweak https://github.com/volta-cli/volta/blob/main/.github/workflows/release.yml to add a Windows + ARM section (in order to generate the required artifacts for a release).

@iNViTiON
Copy link
Contributor Author

I think the release builds are also generated from this repo, so I think you'd need to tweak https://github.com/volta-cli/volta/blob/main/.github/workflows/release.yml to add a Windows + ARM section (in order to generate the required artifacts for a release).

I believe running Volta x64 with an emulator and using native ARM for NodeJS might work well, as Volta doesn't require heavy CPU usage.

Should we go ahead and release this first, then work on building a native ARM version of Volta for Windows later?

@iNViTiON
Copy link
Contributor Author

iNViTiON commented Jan 14, 2024

I resolved the conflict with the main branch, by rebasing. In commit 1f4585a, all platforms and architectures were tested with version v16.x, the cutoff for Apple Silicon. I updated this to v20.x, the cutoff for Windows on ARM.

Is it advisable to segregate x64, Apple Silicon, and Windows on ARM for these specific version cutoffs? @rwjblue @charlespierce

@Fydon
Copy link

Fydon commented Feb 22, 2024

As ARM64 support for Linux is now available, could the target_os requirement in crates/volta-core/src/tool/node/metadata.rs be removed? This is assuming that this code is required for all operating systems that support ARM64.

@iNViTiON
Copy link
Contributor Author

As ARM64 support for Linux is now available, could the target_os requirement in crates/volta-core/src/tool/node/metadata.rs be removed? This is assuming that this code is required for all operating systems that support ARM64.

I'm still unsure about Linux's implementation for the ARM architecture. However, isn't macOS continuing to utilize target_os in metadata.rs?

@Fydon
Copy link

Fydon commented Feb 27, 2024

The way I read your changes they are adding Windows to target_os in metadata.rs. If a target_os check for Linux is added to metadata.rs as well, it may be equivalent to removing target_os as a check, as I believed that the only supported operating systems are Linux, macOS and Windows. I'm assume that just having a target_arch check for aarch64 is possible.

I don't fully understand how Volta fits together, but I see that there are ARM64 builds (shown as ARMv8) listed on the NodeJS download page, which I assume means that there are native ARM64 builds for Linux. I assume part of your changes is to obtain ARM64 builds for Windows, in addition to what is already being done for macOS and so might also be relevant for Linux.

@iNViTiON
Copy link
Contributor Author

The way I read your changes they are adding Windows to target_os in metadata.rs. If a target_os check for Linux is added to metadata.rs as well, it may be equivalent to removing target_os as a check, as I believed that the only supported operating systems are Linux, macOS and Windows. I'm assume that just having a target_arch check for aarch64 is possible.

I don't fully understand how Volta fits together, but I see that there are ARM64 builds (shown as ARMv8) listed on the NodeJS download page, which I assume means that there are native ARM64 builds for Linux. I assume part of your changes is to obtain ARM64 builds for Windows, in addition to what is already being done for macOS and so might also be relevant for Linux.

I suggest we keep the OS check since each OS has Node.js native ARM support in different versions with different file format.

Copy link
Contributor

@charlespierce charlespierce left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making these updates!

@charlespierce
Copy link
Contributor

For some reason I'm not seeing the CI checks (nor any way for me to approve them to run). I'm going to close and re-open to see if that gets GitHub working 🤷🏻

@charlespierce
Copy link
Contributor

The CI failures I think are related to GitHub actions transitioning macos-latest to use an M1 machine, so they're running on an architecture that we don't have fixtures for. I'll create a separate PR to update the fixtures and then that will need to be merged / rebased into this to get CI passing.

@iNViTiON
Copy link
Contributor Author

The CI failures I think are related to GitHub actions transitioning macos-latest to use an M1 machine, so they're running on an architecture that we don't have fixtures for. I'll create a separate PR to update the fixtures and then that will need to be merged / rebased into this to get CI passing.

Thanks a lot. Can you ping me again when I need to do the rebase?

@charlespierce
Copy link
Contributor

@iNViTiON The CI should be working again, so if you get a moment can you rebase the changes? Thanks again!

@iNViTiON
Copy link
Contributor Author

@iNViTiON The CI should be working again, so if you get a moment can you rebase the changes? Thanks again!

I use Sync fork button on my forked repo. Is it okay or should I rebase my fork instead?

@charlespierce
Copy link
Contributor

No need to actually rebase, merging the changes in should be sufficient!

@charlespierce charlespierce merged commit 08219c5 into volta-cli:main May 13, 2024
11 checks passed
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