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

Un-break building under arm64 #3

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Conversation

rshura
Copy link

@rshura rshura commented Aug 26, 2021

The -msse option is specific to x86_64, it does not build under arm64.

The -msse option is specific to x86_64, it does not build under arm64.
@Dobatymo
Copy link
Owner

Dobatymo commented Aug 28, 2021

It would be better to check if the architecture is ARM64, otherwise the binaries which are built for linux on pypi are too specific to the github actions system. Maybe using platform.machine() == "aarch64".

@rshura
Copy link
Author

rshura commented Aug 28, 2021

I don't quite understand the objection. If the build is happening on x86_64 then -march=native will include all -msse flags that are appropriate for the hardware. If the build is happening on any other hardware architecture than x86_64, not just arm64, but anything else, then -msse4.2 will fail. I happened to trigger this on arm64, but it would be the same for anything else.

There's of course a question of what if github builds on arch X and I want to use arch Y, but that is a different issue altogether.

@Dobatymo
Copy link
Owner

Because the wheels are built with github actions arch=native will optimize for the github runner cpus. They should work on other CPUs also because the pypi wheels are mostly for the convenience of people who cannot build the wheels by themselves. sse4.2 is important for good performance on x86 64 so I included that flag.

So perhaps the ss3 flag should be behind a platform.machine().lower() in ("x86_64", "amd64") (or similar) guard instead.

@rshura
Copy link
Author

rshura commented Aug 31, 2021

I did as you suggested.

@Dobatymo
Copy link
Owner

Hi, could you add import platform on top please?

@Dobatymo Dobatymo merged commit a99495f into Dobatymo:master Aug 31, 2021
@Dobatymo
Copy link
Owner

Thanks!

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.

2 participants