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

Add standalone-cli build for Windows Arm64 platform #10001

Merged
merged 4 commits into from Dec 5, 2022
Merged

Add standalone-cli build for Windows Arm64 platform #10001

merged 4 commits into from Dec 5, 2022

Conversation

MJRasicci
Copy link
Contributor

This PR adds building the standalone-cli for windows targeting Am64. Updates are made to the standalone-cli tests to ensure the proper executable is used for running tests under windows when the node binary is targeting a different architecture than the host OS.

With the release of the Windows Dev Kit 2023 and Arm64 Visual Studio as well as existing support for arm64 on other platforms, it seems natural to release the standalone-cli for Windows. It's especially helpful when using the standalone-cli as part of automated builds for non-javascript projects.

Built and tested with Windows 11 22H2 Arm64 on a Windows Dev Kit 2023 with Visual Studio 17.5 Preview 1 using the bundled node.js

Change the path to the binary on Windows to use backslashes and the OS architecture instead of the architecture of the node binary.

Visual Studio 17.4, the first GA release supporting ARM64, bundles node built for ia32, as official arm64 binaries are not yet available on Windows. Using the PROCESSOR_ARCHITEW6432 environment variable, if present, ensures we run the tests using the executable supported natively by the OS.
@thecrypticace
Copy link
Contributor

thecrypticace commented Dec 5, 2022

I guess it's time for me to get a Windows dev kit haha. Regarding the PROCESSOR_ARCHITEW6432 env var, it tells us the underlying arch instead of the emulated arch (for example if inside a 32 vs 64 bit shell, I imagine this also applies to x86/64 emulation but not sure).

Is there any reason we need to use this instead of just process.arch? It feels like, to me, process.arch should always be what you want to use. The bundled version of Node for the ARM binary should be built for 64-bit ARM so there shouldn't be any disconnect between the two.

@thecrypticace thecrypticace self-assigned this Dec 5, 2022
@MJRasicci
Copy link
Contributor Author

Official node binaries for arm64 on Windows haven't been released yet. It's being actively discussed according to their workgroup agenda but in my 17.5 Preview 1 install, process.arch and even os.arch() both return 'ia32'. I haven't done a native arm64 build of node yet but since process.arch returns the target architecture that should make 83f4b2d unnecessary (though I did still need to use backslashes in order to run the tests, unsure if that was related to my terminal or not). Since 32-bit binaries aren't available for the standalone-cli, I think users would still want to run the binary supported by their platform even if they have a 32-bit node install for some reason. PROCESSOR_ARCHITEW6432 is null if the process is running on the same architecture as the OS so it falls back to process.arch in that case.

I felt the environment variable was the least invasive method to make sure the tests were able to run in this, albeit uncommon and temporary, situation. I'm sure you guys know better than me whether it makes more sense for the tailwind project to wait until official node binaries are released for arm64 windows before merging this.

@thecrypticace
Copy link
Contributor

Ah I see okay. If the official node binaries don't exist yet this it's highly likely adding this won't even work because we bundle node based on the officially provided binaries (or rather pkg does that for us). I'mma give it a try though. It might work. Will report back.

@thecrypticace
Copy link
Contributor

Looks like pkg includes a native ARM build for node. A slightly modified CLI that outputs arch information results in this:

Screen Shot 2022-12-05 at 11 31 50

On the left is an x86 shell and on the right is an ARM native shell (which shouldn't affect the process.arch variable but I was curious). So, as far as I can tell, we don't need the environment variable at all because it will never be defined anyway.

@thecrypticace
Copy link
Contributor

Okay, I did some final testing. It appears that, if running an x64 build of node, that env var isn't populated at all either. I think this only matters for 32-bit installations on Windows which we don't even build binaries for. Also these standalone CLI tests are mainly for CI. They only run in the release pipeline to make sure we didn't mess something up. All CLI interactions are otherwise covered by our standard CLI integration tests. So I'm going to simplify that check a bit.

I don't know why we have to use backslashes when I think it worked before? But yeah I'm seeing that behavior too.

@thecrypticace
Copy link
Contributor

Well, I'll merge it once GitHub actions decides to start working again instead of failing to clone stuff. Woo.

@MJRasicci
Copy link
Contributor Author

Awesome, thanks!

Also you're correct, the PROCESSOR_ARCHITEW6432 environment variable is only set in processes built for a different architecture than the operating system. This isn't an issue for the standalone-cli since pkg outputs native binaries, as you saw in your first test. This only caused problems when running npm test when a 32-bit node is running on 64-bit windows or in my case arm64 within an x86 emulation layer. Doesn't matter for the CI builds since the tests only run under linux but I wanted to make sure I could pass the tests locally when submitting the PR. Here's a screenshot that show it more clearly. Running x86 powershell vs arm64 native powershell results in different environment variables.

image

So when running the tests with process.arch it would try and run tailwindcss-windows-ia32.exe which caused the tests to fail.

@thecrypticace
Copy link
Contributor

Yeah makes sense. I think we can leave it out for now but if it becomes needed again then we can add it back. Really appreciate it.

Aside: Any reason you're using a 32-bit x86 install of Node instead of 64-bit? I'm just curious. ¯_(ツ)_/¯ I was able to emulate the x64 version of Node on ARM just fine. (which I am currently running in virtual machine :D on my macbook since I don't have a Dev Kit currently). I did notice that a few places tried to automatically download 32-bit intel versions of apps when the x64 version appears to work just fine. Seemed a bit weird to me. I figured it was more of a "we don't know what platform this is so we default to 32-bit intel"

@thecrypticace thecrypticace merged commit 9e8d37a into tailwindlabs:master Dec 5, 2022
@thecrypticace
Copy link
Contributor

Thanks again! This change will go out with the next release.

@MJRasicci
Copy link
Contributor Author

That's just the version of node that was with Visual Studio 17.5, I assume because there isn't an official arm64 build yet. It seems to run okay (maybe a bit slow but without a native arm64 build to compare performance with I only have a 10900K for reference). Overall the Snapdragon 8cx in the Windows Dev Kit has performed admirably over the last month that I've had it.

According to this windows is just using WOW64 which was made to run 32-bit apps on 64-bit windows so even if it does run 64-bit apps it probably gets better performance with a 32-bit exe.

Thanks!

@sxa
Copy link

sxa commented Dec 5, 2022

It's being actively discussed according to their nodejs/build#3105 but in my 17.5 Preview 1 install, process.arch and even os.arch() both return 'ia32'.

FYI We have had some builds for a while in the Node.js jenkins instance, and have been working through test case failures which are pretty much all done so there are no serious issues with the builds on that platform that I know of. We're working on adding it into the regular CI runs and then - pending sufficient machines being made available - we can look at moving it forward towards being a full release.

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.

None yet

3 participants