Windows Arm64 support for emsdk.py only #1186

Blackhex · 2023-02-14T12:40:48Z

This PR add support for Visual Studio Build Tools 2022 (Preview) compilation of packages using emsdk.py on and for Windows Arm64.

Blackhex · 2023-02-14T12:42:34Z

emsdk.py

@@ -256,7 +253,8 @@ def vswhere(version):
    if not program_files:
      program_files = os.environ['ProgramFiles']
    vswhere_path = os.path.join(program_files, 'Microsoft Visual Studio', 'Installer', 'vswhere.exe')
-    output = json.loads(subprocess.check_output([vswhere_path, '-latest', '-version', '[%s.0,%s.0)' % (version, version + 1), '-requires', 'Microsoft.VisualStudio.Component.VC.Tools.x86.x64', '-property', 'installationPath', '-format', 'json']))
+    tools_arch = ('ARM64' if ARCH == 'aarch64' else 'x86.x64')
+    output = json.loads(subprocess.check_output([vswhere_path, '-latest', '-products', '*', '-prerelease', '-version', '[%s.0,%s.0)' % (version, version + 1), '-requires', 'Microsoft.VisualStudio.Component.VC.Tools.' + tools_arch, '-property', 'installationPath', '-format', 'json']))


The -products * allows detection of Build Tools, the -prerelease allows detection of Preview version of Visual Studio and Build Tools.

Perhaps put this in a comment on the line above?

sbc100

lgtm with comments addressed.

Thanks for working on this!

sbc100 · 2023-02-14T16:04:26Z

emsdk.py

+  elif ARCH == 'x86_64':
+    return 'x64'
+  else:
+    return 'x86'


How about writing this a dict so it will crash accordingly on unknown arch instead of defaulting to x86. e.g.

cmake_arch = { 'aarch64': 'arm64' ... } return cmake_arch[ARCH]

Also should ARM64 and ARM be capitalized here. I'd rather keep them all lowercase if that works?

The code style fixed and the values changed according to the source reference though the source does not mention Arm values. Experimentally was proven that it needs to be capital.

sbc100 · 2023-02-14T16:06:25Z

emsdk.py

@@ -256,7 +253,8 @@ def vswhere(version):
    if not program_files:
      program_files = os.environ['ProgramFiles']
    vswhere_path = os.path.join(program_files, 'Microsoft Visual Studio', 'Installer', 'vswhere.exe')
-    output = json.loads(subprocess.check_output([vswhere_path, '-latest', '-version', '[%s.0,%s.0)' % (version, version + 1), '-requires', 'Microsoft.VisualStudio.Component.VC.Tools.x86.x64', '-property', 'installationPath', '-format', 'json']))
+    tools_arch = ('ARM64' if ARCH == 'aarch64' else 'x86.x64')


Can arm64 be lowercase here?

Should we switch our internal ARCH=aarch64 to ARCH=arm64 maybe?

No need for brackets around this expression.

This is driven by VS component IDs so it should be capital: https://learn.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2022.

aarch64 came from Unix-like systems and the value is used both for Linux, MacOS and Windows. I'd keep it as is unless we want to distinguish between different OSes.

sbc100 · 2023-02-14T16:09:05Z

emsdk.py

+  elif hasattr(tool, 'arch') and tool.arch == 'x86_64':
+    return 'x64'
+  elif hasattr(tool, 'arch') and tool.arch == 'x86':
+    return 'x86'


How about just one if hasattr(tool, 'arch') here.

e.g.

if hasattr(tool, 'arch'): tools_arch_map = {'aarch64': ARM64' ...} return tool_arch_map[tool.arch] // Fall back to host arch. if ARCH == 'aarch64': return 'ARM64' else: return 'x64' if tool.bitness == 64 else 'x86'

Should we call decide_cmake_host_platform for the fallback case?

The code style has changed and the values fixed according to the source references added. The host platform and target platform values has different syntax as you can see in the sources.

sbc100 · 2023-02-14T16:10:51Z

emsdk.py

-    args += ['-A', 'x64' if tool.bitness == 64 else 'x86']
-    args += ['-Thost=x64']
+    args += ['-A', decide_cmake_target_platform(tool)]
+    args += ['-Thost=' + decide_cmake_host_platform()]


Can we drop the decide_ here and just use cmake_target_platform? Or maybe use get_ if you think we need a prefix at all?

I wanted to be consistent with with decide_cmake_build_type but OK.

sbc100 · 2023-02-14T16:11:57Z

emsdk.py

@@ -1360,7 +1383,7 @@ def build_binaryen_tool(tool):
    return False

  # Make
-  success = make_build(build_root, build_type, 'x64' if tool.bitness == 64 else 'Win32')
+  success = make_build(build_root, build_type)


I guess maybe the removal of this third unused argument could be its own NFC (non-functional-change)? But its not a big deal if you want to leave it as part of this one..

You decide, I had those changes written before VS 2022 support was added upstream so when I was updating my old changes including VS 2022 support to the new upstream state, this change was some kind of leftover of the code adding the VS 2022 support.

I always prefer smaller and separate changes. So I would prefer a separate PR in this case if you have time.

sbc100 · 2023-02-15T16:52:04Z

emsdk.py

@@ -256,7 +253,9 @@ def vswhere(version):
    if not program_files:
      program_files = os.environ['ProgramFiles']
    vswhere_path = os.path.join(program_files, 'Microsoft Visual Studio', 'Installer', 'vswhere.exe')
-    output = json.loads(subprocess.check_output([vswhere_path, '-latest', '-version', '[%s.0,%s.0)' % (version, version + 1), '-requires', 'Microsoft.VisualStudio.Component.VC.Tools.x86.x64', '-property', 'installationPath', '-format', 'json']))
+    # Source: https://learn.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2022
+    tools_arch = 'ARM64' if ARCH == 'aarch64' else 'x86.x64'


If this is supposed to be uppercase why isn't the else case "X86.X64"?

I was referring to Arm64 variant only. x86.x64 should be lower case. See https://learn.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2022 for the reference.

sbc100 · 2023-02-15T16:52:40Z

emsdk.py

@@ -256,7 +253,8 @@ def vswhere(version):
    if not program_files:
      program_files = os.environ['ProgramFiles']
    vswhere_path = os.path.join(program_files, 'Microsoft Visual Studio', 'Installer', 'vswhere.exe')
-    output = json.loads(subprocess.check_output([vswhere_path, '-latest', '-version', '[%s.0,%s.0)' % (version, version + 1), '-requires', 'Microsoft.VisualStudio.Component.VC.Tools.x86.x64', '-property', 'installationPath', '-format', 'json']))
+    tools_arch = ('ARM64' if ARCH == 'aarch64' else 'x86.x64')
+    output = json.loads(subprocess.check_output([vswhere_path, '-latest', '-products', '*', '-prerelease', '-version', '[%s.0,%s.0)' % (version, version + 1), '-requires', 'Microsoft.VisualStudio.Component.VC.Tools.' + tools_arch, '-property', 'installationPath', '-format', 'json']))


Perhaps put this in a comment on the line above?

sbc100 · 2023-02-15T16:54:29Z

emsdk.py

+    if ARCH == 'aarch64':
+      return 'ARM64'
+    else:
+      return 'x64' if tool.bitness == 64 else 'Win32'


Is the return value here supposed to be uppercase?

What happens when this function is called on non-windows platforms?

According to https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2017%202022.html#platform-selection yes. This method should not be called on non-Windows platforms as it is called only when generator is "Visual Studio".

sbc100 · 2023-02-15T16:56:39Z

emsdk.py

+    else:
+      return cmake_target_platform_no_tool_arch(tool)
+  else:
+    return cmake_target_platform_no_tool_arch(tool)


I think you can remove the to final else blocks here and just return cmake_target_platform_no_tool_arch.

Also since cmake_target_platform_no_tool_arch is very simple and (after the above change) has just one calls site maybe just inline it here?

emsdk.py

sbc100 · 2023-02-15T18:50:37Z

emsdk.py

@@ -142,9 +142,6 @@ def exit_with_error(msg):
  ARCH = 'x86'
 elif machine.startswith('aarch64') or machine.lower().startswith('arm64'):
  ARCH = 'aarch64'
-  if WINDOWS:
-      errlog('No support for Windows on Arm, fallback to x64')
-      ARCH = 'x86_64'


On macOS it was useful for a while to run the x86 prebuilt binaries on arm.. does that approach also work on windows (i.e. is there binary emulation?) . If so, would this change cause a regression since today you can just install the x86 binaries and have a working installation?

You are right, as far as there are no publicly available Arm64 binaries we should install the x64 ones running in emulation. But having this code here prevents form possiblility to build the Arm64 binaries from sources using emsdk.py. For that reason, I've move this check into the install_tool() method. The code is not nice but this should be only a temporary solution.

One option is that we drop the hacks and folks can use EMSDK_ARCH=x86_64 emsdk install latest if they want to force the install the x86 binaries? Or do you think we should continue to make it automatic?

I guess the hack is better because it keeps the current behaviour. I can rember when I started with emsdk that it took me a while to figure out that the EMSDK_ARCH even exists. On the other, hand note that there is this "strange" behavior: If you have Arm64 Python installed on your system and you do emsdk.bat install latest and emsdk.bat activate latest, the x64 version of emsdk Python is installed and activated and since then, emsdk.bat install xxx installs or compiles x64 versions anyway (without the error message). You either need to force Arm64 with EMSDK_ARCH=aarch64, or use system Python with python emsdk.py install xxx. If your system Python is x64, you need to force EMSDK_ARCH=aarch64 in all cases.

Yes, the python based system detection is a bit boring, as the architecture of your python dictates which platform you have. As you mentioned, if you have an x64 python (emulated on arm64), it will say you're on an x64 cpu.

The other way to detect this under Windows is to use PROCESSOR_ARCHITECTURE env var. Same joke, from cmd.exe, it says you're on an arm64 system, and from powershell, that you're on an x64 system (because default powershell install is x64).

For this kind of case, IMHO, having an explicit option (available through help) would be the best solution. Or at least, to mention EMSDK_ARCH in help and official documentation.

sbc100 · 2023-02-15T18:59:25Z

lgtm! Thanks for working on this.

I suppose the next step would be try to build release binaries for windows/arm64 so folks don't need to build from source all the time. Do you think we are that point where that it worth the investment? How many windows/arm64 users are out there right now?

Blackhex · 2023-02-16T20:19:33Z

I suppose the next step would be try to build release binaries for windows/arm64 so folks don't need to build from source all the time. Do you think we are that point where that it worth the investment? How many windows/arm64 users are out there right now?

@sbc100 Thank you for your help with the review and merging.

We already have some experimental binaries and now we are ensuring they can pass all the emscripten repository unit tests and some useful projects can be compiled with them. You can check out our https://github.com/Windows-on-ARM-Experiments/emsdk fork. As our team's mission is an enablement of smooth developer experience on Windows Arm64, we are happy to listen any requests or suggestions in that regards.

sbc100 · 2023-02-16T20:45:22Z

Great. Once you have some binaries you are happen with we could consider adding them emscripten-releases-tags.json like we have to latest-arm64-linux:

{                                                                                   
  "aliases": {                                                                      
    "latest": "3.1.31",                                                             
    "latest-sdk": "latest",                                                         
    "latest-arm64-linux": "3.1.21",                                                 
    "latest-64bit": "latest",                                                       
    "sdk-latest-64bit": "latest",                                                   
    "latest-upstream": "latest",                                                                              
    "latest-clang-upstream": "latest",                                           
    "latest-releases-upstream": "latest"                                         
  },

Note that latest-arm64-linux is not necessarily up-to-date because we don't build the binaries with the same automatic process. We could potentially do the same for arm64-windows?

Blackhex · 2023-02-16T20:51:56Z

Why not target to build the Windows Arm64 regularly together with x64 ones? Cross-compilation is always the option.

sbc100 · 2023-02-16T21:05:33Z

Sure, we do that for macOS, so if its easy for windows we can do that too

…1186)

Blackhex commented Feb 14, 2023

View reviewed changes

Blackhex force-pushed the emsdk-vs2022-arm64 branch from 88482fb to f7ad4e1 Compare February 14, 2023 12:45

Blackhex changed the title ~~Windows Arm64 support~~ Windows Arm64 support for emsdk.py only Feb 14, 2023

Blackhex mentioned this pull request Feb 14, 2023

Support for Windows on Arm #1116

Open

sbc100 approved these changes Feb 14, 2023

View reviewed changes

Blackhex force-pushed the emsdk-vs2022-arm64 branch 4 times, most recently from 4ff3dd7 to 7149e8b Compare February 15, 2023 08:23

Blackhex requested a review from sbc100 February 15, 2023 08:27

sbc100 reviewed Feb 15, 2023

View reviewed changes

Blackhex force-pushed the emsdk-vs2022-arm64 branch 3 times, most recently from ce32e10 to 07d652c Compare February 15, 2023 18:52

sbc100 reviewed Feb 15, 2023

View reviewed changes

Blackhex requested a review from sbc100 February 15, 2023 18:55

sbc100 approved these changes Feb 15, 2023

View reviewed changes

Add support for Windows Arm64 compilation of packages using emsdk

884c7c6

Blackhex force-pushed the emsdk-vs2022-arm64 branch from 07d652c to 884c7c6 Compare February 15, 2023 19:19

sbc100 merged commit 7bcf144 into emscripten-core:main Feb 15, 2023

Blackhex deleted the emsdk-vs2022-arm64 branch February 16, 2023 08:47

Blackhex mentioned this pull request Feb 16, 2023

Remove unused build_target_platform parameter from make_build #1187

Merged

shlomif pushed a commit to shlomif/emsdk that referenced this pull request Sep 29, 2023

Add Windows Arm64 support for compiling from source (emscripten-core#…

c98ba98

…1186)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Arm64 support for emsdk.py only #1186

Windows Arm64 support for emsdk.py only #1186

Blackhex commented Feb 14, 2023 •

edited

Loading

Blackhex Feb 14, 2023

sbc100 Feb 15, 2023

sbc100 left a comment

sbc100 Feb 14, 2023

Blackhex Feb 15, 2023 •

edited

Loading

sbc100 Feb 14, 2023

Blackhex Feb 15, 2023 •

edited

Loading

sbc100 Feb 14, 2023

Blackhex Feb 15, 2023

sbc100 Feb 14, 2023

Blackhex Feb 15, 2023

sbc100 Feb 14, 2023

Blackhex Feb 15, 2023

sbc100 Feb 15, 2023

sbc100 Feb 15, 2023

Blackhex Feb 15, 2023

sbc100 Feb 15, 2023

sbc100 Feb 15, 2023

Blackhex Feb 15, 2023

sbc100 Feb 15, 2023

sbc100 Feb 15, 2023

Blackhex Feb 15, 2023 •

edited

Loading

sbc100 Feb 15, 2023

Blackhex Feb 15, 2023 •

edited

Loading

pbo-linaro Feb 16, 2023

sbc100 commented Feb 15, 2023

Blackhex commented Feb 16, 2023

sbc100 commented Feb 16, 2023

Blackhex commented Feb 16, 2023

sbc100 commented Feb 16, 2023

Windows Arm64 support for emsdk.py only #1186

Windows Arm64 support for emsdk.py only #1186

Conversation

Blackhex commented Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackhex Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackhex Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackhex Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackhex Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Feb 15, 2023

Blackhex commented Feb 16, 2023

sbc100 commented Feb 16, 2023

Blackhex commented Feb 16, 2023

sbc100 commented Feb 16, 2023

Blackhex commented Feb 14, 2023 •

edited

Loading

Blackhex Feb 15, 2023 •

edited

Loading

Blackhex Feb 15, 2023 •

edited

Loading

Blackhex Feb 15, 2023 •

edited

Loading

Blackhex Feb 15, 2023 •

edited

Loading