-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix building and linking OpenSSL on Windows ARM #122
Conversation
@@ -345,8 +345,8 @@ def build_extension(self, ext: Extension): | |||
if self.compiler.compiler_type == "msvc": | |||
if IS_X86 and "msvc_x86_flags" in source_files: | |||
args["extra_postargs"] += source_files["msvc_x86_flags"] | |||
if IS_X86 and "msvc_x86_libraries" in source_files: | |||
ext.libraries += source_files["msvc_x86_libraries"] | |||
if "msvc_libraries" in source_files: |
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 lines above do need the x86 in them? Only these don't?
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 could foresee a possible need to complement msvc_x86_flags
with msvc_arm_flags
in the future, and it keeps it simlar to how there are gcc_flags
, gcc_x86_flags
, gcc_arm_flags
and others.
I don't think there would be any harm always including those flags and just rename it to msvc_flags
, they seem to just have x86 relevant /arch
flags so those codepaths are not used on arm anyway.
Maybe @animetosho can let us know if they get a chance.
We we want the libraries linked regardless of architecture, so I thought it better to rename the key.
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 using unsupported /arch
flags causes MSVC to emit a warning and then ignore the issue. So you could set it on an ARM compile, but there's no reason to do so.
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.
👍
@mnightingale Any chance you have time to see if you can get cross-compile working on Github Actions to get a arm64 wheel? 😇 |
Oh wait, it's easy :) I'll do it! |
I see you've released it now, I did test with Just to note I did see from the action log:
So it still picks up the host details, however it's the same issue as discussed in this PR so is of no consequence. |
Fixes #121
Public Arm runners are coming soon so you will be able to replace the slow Linux QEMU build and add Windows Arm to the matrix.