-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Check MSVC arm64 variant on arm64 host #4555
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
dab2b03
Check MSVC arm64 variant too
saschanaz 938fabf
conditional selector
saschanaz c5825c0
Merge branch 'main' into msvc-aarch64
saschanaz 12fe9f7
Update msvc.py
saschanaz 102b401
Merge branch 'main' into msvc-aarch64
saschanaz f9c669d
use distutils.util.get_platform
saschanaz 374130e
Merge remote-tracking branch 'upstream/main' into msvc-aarch64
saschanaz bf9f2ac
Merge branch 'msvc-aarch64' of https://github.com/saschanaz/setuptool…
saschanaz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Added detection of ARM64 variant of MSVC -- by :user:`saschanaz` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
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).
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.
I think it should take priority, because we need arm64 variant to get that
x86_arm64compiler. 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:
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
The comment on trying that option was regarding appending (in opposition to prepending, i.e last position)
"Microsoft.VisualStudio.Component.VC.Tools.arm64"to thesuitable_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"tosuitable_components.But according to your previous comment, there would be a second problem that appears when we append
....arm64"tosuitable_componentsand 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"tosuitable_components.What I would like for us to do is to document in this PR/discussion is:
"Microsoft.VisualStudio.Component.VC.Tools.arm64"tosuitable_componentsas the last entry (like we did withWDExpress, no conditional).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.
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:
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.
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.
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.