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

Check MSVC arm64 variant on arm64 host #4555

Merged
merged 8 commits into from
Aug 26, 2024
Merged

Conversation

saschanaz
Copy link
Contributor

Summary of changes

For now this selects Hostx86/arm64 because of old distutils decision:

# A map keyed by get_platform() return values to values accepted by
# 'vcvarsall.bat'. Always cross-compile from x86 to work with the
# lighter-weight MSVC installs that do not include native 64-bit tools.
PLAT_TO_VCVARS = {
'win32': 'x86',
'win-amd64': 'x86_amd64',
'win-arm32': 'x86_arm',
'win-arm64': 'x86_arm64',
}

This works but just runs slower. Perhaps we can safely require arm64 MSVC for arm64 python installations?

Closes #4553

Pull Request Checklist

@abravalheri
Copy link
Contributor

abravalheri commented Aug 12, 2024

Hi @saschanaz, thank you very much for the contribution.

Could you please propose the change directly to the pypa/distutils repository? In the pypa/setuptools repository we don't modify the _distutils folder directly, instead we always obtain the latest version of pypa/distutils (and that overwrites any local change).

Never mind, just seeing that the change do not affect the _distutils folder. Sorry for the noise.

@@ -89,6 +89,7 @@ def _msvc14_find_vc2017():
return None, None

suitable_components = (
"Microsoft.VisualStudio.Component.VC.Tools.arm64",
Copy link
Contributor

@abravalheri abravalheri Aug 12, 2024

Choose a reason for hiding this comment

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

Personally, I would add this as the last component of the list so that it does not take priority, the reasoning is merely to take a very conservative approach.

By appending it to the list we don't change much how the code currently works only add a fallback, but if we prepend it to the list it starts to take priority and I honestly would like to avoid unintended consequences 😅 (even if it costs performance).

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 think it should take priority, because we need arm64 variant to get that x86_arm64 compiler. Otherwise it won't exist and the code will fail to get the compiler when ran from arm64 python. Initially I went further and only put arm64 variant on arm64 python, but I just did not find any actual benefit over just putting it on the existing list.

Python with other architectures still can properly select the compiler matching their architecture even if arm64 variant is selected, as somehow it can also provide paths for them, as seen below:

> py -3.12
Python 3.12.5 (tags/v3.12.5:ff3bc82, Aug  6 2024, 20:45:27) [MSC v.1940 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from setuptools._distutils import ccompiler
>>> compiler = ccompiler.new_compiler()
>>> compiler.initialize()
>>> compiler.cc
'C:\\Program Files (x86)\\Microsoft Visual Studio\\2022\\BuildTools\\VC\\Tools\\MSVC\\14.40.33807\\bin\\HostX86\\x64\\cl.exe'
> py -3.12-arm64
Python 3.12.5 (tags/v3.12.5:ff3bc82, Aug  6 2024, 20:54:36) [MSC v.1940 64 bit (ARM64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from setuptools._distutils import ccompiler
>>> compiler = ccompiler.new_compiler()
>>> compiler.initialize()
>>> compiler.cc
'C:\\Program Files (x86)\\Microsoft Visual Studio\\2022\\BuildTools\\VC\\Tools\\MSVC\\14.40.33807\\bin\\HostX86\\ARM64\\cl.exe'

Copy link
Contributor

@abravalheri abravalheri Aug 13, 2024

Choose a reason for hiding this comment

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

Thank you very much for the clarification.

From what I understood from the problem description in #4553, the exception distutils.errors.DistutilsPlatformError("Unable to find vcvarsall.bat") is common from the fact that the function _msvc14_find_vc2017() returns (_whatever_, None). So appending the value to the list would prevent that particular exception.

Did you try that option, what happens then?

Based in your last comment, I assume that there is a follow up error that justifies prepending the component to the list. It would be nice if we can have a description of this potential follow-up problem before we take a decision regarding the position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I could not understand what option do you want to try here. What specific value do you want to append to the tuple?

Meanwhile, I understand your conservative-as-possible argument, so I just made the component selector conditional so that only arm64 python will select arm64 msvc. This now should not affect x86-64 python's behavior, only arm64 python failure will be fixed.

Copy link
Contributor

@abravalheri abravalheri Aug 23, 2024

Choose a reason for hiding this comment

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

The comment on trying that option was regarding appending (in opposition to prepending, i.e last position) "Microsoft.VisualStudio.Component.VC.Tools.arm64" to the suitable_components.


I am trying to understand and document all the problems that we are dealing with.

The error trace in the linked issue suggests the root of the problem is distutils.errors.DistutilsPlatformError("Unable to find vcvarsall.bat"). I believe, this particular exception would be solved be appending "Microsoft.VisualStudio.Component.VC.Tools.arm64"to suitable_components.

But according to your previous comment, there would be a second problem that appears when we append ....arm64"to suitable_components and prevents the build from being successful.
That is why we would need the conditional approach you are suggesting or prepending "Microsoft.VisualStudio.Component.VC.Tools.arm64"to suitable_components.

What I would like for us to do is to document in this PR/discussion is:

  • Suppose we simply add "Microsoft.VisualStudio.Component.VC.Tools.arm64" to suitable_components as the last entry (like we did with WDExpress, no conditional).
  • What would be the new stacktrace and error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, my updated thought:

I think append and prepend wouldn't really change the behavior, as it's only being used to get vsvarsall, which would be identical regardless of the tool selection (it should have all information).

Then here's the thing: append/prepend would allow selecting x86.x64 on aarch64 python if arm64 variant is not installed, which causes weird linking error which is harder to understand. Here's a zstandard example:

> pip install zstandard
Collecting zstandard
  Using cached zstandard-0.23.0.tar.gz (681 kB)
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: zstandard
  Building wheel for zstandard (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Building wheel for zstandard (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [32 lines of output]
      <string>:41: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
      <string>:42: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
      Microsoft (R) C/C++ Optimizing Compiler Version 19.41.34120 for x86
      Copyright (C) Microsoft Corporation.  All rights reserved.

      tmpcts_zhd5.h
      Microsoft (R) C/C++ Optimizing Compiler Version 19.41.34120 for x86
      Copyright (C) Microsoft Corporation.  All rights reserved.

      tmp07n40syy.h
      not modified: 'build\\zstandard\\_cffi.c'
      generating build\zstandard\_cffi.c
      (already up-to-date)
      running bdist_wheel
      running build
      running build_py
      creating build\lib.win-arm64-cpython-312
      creating build\lib.win-arm64-cpython-312\zstandard
      copying zstandard\backend_cffi.py -> build\lib.win-arm64-cpython-312\zstandard
      copying zstandard\__init__.py -> build\lib.win-arm64-cpython-312\zstandard
      copying zstandard\__init__.pyi -> build\lib.win-arm64-cpython-312\zstandard
      copying zstandard\py.typed -> build\lib.win-arm64-cpython-312\zstandard
      running build_ext
      building 'zstandard.backend_c' extension
      creating build\temp.win-arm64-cpython-312
      creating build\temp.win-arm64-cpython-312\Release
      creating build\temp.win-arm64-cpython-312\Release\c-ext
      "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.41.34120\bin\HostX86\x86\cl.exe" /c /nologo /O2 /W3 /GL /DNDEBUG /MD -Ic-ext -Izstd -IC:\Users\sasch\AppData\Local\Programs\Python\Python312-arm64\include -IC:\Users\sasch\AppData\Local\Programs\Python\Python312-arm64\Include "-IC:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.41.34120\include" "-IC:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.41.34120\ATLMFC\include" "-IC:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Auxiliary\VS\include" "-IC:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\shared" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\winrt" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\cppwinrt" /Tcc-ext\backend_c.c /Fobuild\temp.win-arm64-cpython-312\Release\c-ext\backend_c.obj -DZSTD_SINGLE_FILE -DZSTDLIB_VISIBLE= -DZDICTLIB_VISIBLE= -DZSTDERRORLIB_VISIBLE=
      backend_c.c
      "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.41.34120\bin\HostX86\x86\link.exe" /nologo /INCREMENTAL:NO /LTCG /DLL /MANIFEST:EMBED,ID=2 /MANIFESTUAC:NO /LIBPATH:C:\Users\sasch\AppData\Local\Programs\Python\Python312-arm64\libs /LIBPATH:C:\Users\sasch\AppData\Local\Programs\Python\Python312-arm64 /LIBPATH:C:\Users\sasch\AppData\Local\Programs\Python\Python312-arm64\PCbuild\arm64 "/LIBPATH:C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.41.34120\ATLMFC\lib\ARM64" "/LIBPATH:C:\Program Files (x86)\Windows Kits\10\lib\10.0.22621.0\ucrt\arm64" "/LIBPATH:C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\arm64" /EXPORT:PyInit_backend_c build\temp.win-arm64-cpython-312\Release\c-ext\backend_c.obj /OUT:build\lib.win-arm64-cpython-312\zstandard\backend_c.cp312-win_arm64.pyd /IMPLIB:build\temp.win-arm64-cpython-312\Release\c-ext\backend_c.cp312-win_arm64.lib
      LINK : fatal error LNK1104: cannot open file 'MSVCRT.lib'
      error: command 'C:\\Program Files (x86)\\Microsoft Visual Studio\\2022\\BuildTools\\VC\\Tools\\MSVC\\14.41.34120\\bin\\HostX86\\x86\\link.exe' failed with exit code 1104
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for zstandard
Failed to build zstandard
ERROR: ERROR: Failed to build installable wheels for some pyproject.toml based projects (zstandard)

I would definitely prefer setuptools error (same as attached in #4553) than a mysterious linker error (which happens because we obviously cannot mix arm64 and x86 binary).

So I retreat from append/prepend and now prefer explicit single dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thank you very much @saschanaz for helping to document the behaviour.
I think the PR looks nice as it is, but I will ask the help of some people that understand better than me about these inner works for the final review.

@saschanaz saschanaz requested a review from abravalheri August 21, 2024 21:04
@saschanaz saschanaz changed the title Check MSVC arm64 variant too Check MSVC arm64 variant on arm64 host Aug 21, 2024
@abravalheri
Copy link
Contributor

@jaraco, @zooba could you please have a look on this PR?
It looks fine to me, but you guys might have a better understanding of the whole system.

@zooba
Copy link
Contributor

zooba commented Aug 23, 2024

The check should use distutils.util.get_platform() == "win-arm64" so that cross-compiling overrides work (unless you've got a setuptools-specific copy of this function now).

Otherwise, checking for that component is the right thing to do when targeting ARM64. When installed, it should include all the host platforms, so whether cross-compiling or native compiling, there'll be suitable tools there.

I see there are no changes now to the code originally quoted. If you were going to change that, you'd need to compare get_host_platform() with get_platform() to decide whether to use arm64 or else one of x86_arm64 or amd64_arm64 in order to compile. Using x86_arm64 will work consistently everywhere, but is indeed likely to be slower on an ARM64 machine (i.e. when get_host_platform() == "win-arm64").

@saschanaz
Copy link
Contributor Author

The check should use distutils.util.get_platform() == "win-arm64" so that cross-compiling overrides work (unless you've got a setuptools-specific copy of this function now).

Done!

Otherwise, checking for that component is the right thing to do when targeting ARM64. When installed, it should include all the host platforms, so whether cross-compiling or native compiling, there'll be suitable tools there.

I see there are no changes now to the code originally quoted. If you were going to change that, you'd need to compare get_host_platform() with get_platform() to decide whether to use arm64 or else one of x86_arm64 or amd64_arm64 in order to compile. Using x86_arm64 will work consistently everywhere, but is indeed likely to be slower on an ARM64 machine (i.e. when get_host_platform() == "win-arm64").

You mean in https://github.com/pypa/distutils ?

@zooba
Copy link
Contributor

zooba commented Aug 25, 2024

You mean in https://github.com/pypa/distutils ?

Yes, I believe the maintainers still prefer distutils changes to go into that repo first. (I was kind of hoping that it would all just be "setuptools" by now and slowly the distutils part becomes shims over setuptools implementation, but that's up to @abravalheri and team, not me.) It's not an essential change, but it was what you linked to in the original post.

@jaraco
Copy link
Member

jaraco commented Aug 25, 2024

Yes, I believe the maintainers still prefer distutils changes to go into that repo first. (I was kind of hoping that it would all just be "setuptools" by now and slowly the distutils part becomes shims over setuptools implementation, but that's up to @abravalheri and team, not me.)

That's correct. The strategy for integration depends on first deprecating all use of distutils interfaces so that setuptools._distutils is completely private. Once that's done, it's be easier to consolidate the behaviors and unify the test suite (the main thing that demands separate projects right now).

@saschanaz
Copy link
Contributor Author

Thanks, here's distutils PR: pypa/distutils#285

@abravalheri abravalheri merged commit 4c990b9 into pypa:main Aug 26, 2024
19 of 21 checks passed
@abravalheri
Copy link
Contributor

Thank you very much @saschanaz for working on the PR and @zooba for the review/advice.

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.

[BUG] msvc.py never checks Microsoft.VisualStudio.Component.VC.Tools.arm64 and thus fails to detect MSVC
4 participants