-
Notifications
You must be signed in to change notification settings - Fork 38
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
Infer architecture using platform.machine() #22
Comments
I ran def get_arch_name(self):
supported_archs = ('x86_64', )
archs = {
'AMD64': 'x86_64',
}
if "BLIS_ARCH" in os.environ:
return os.environ["BLIS_ARCH"]
else:
platform = platform.machine()
arch = archs.get(platform, arch)
if arch in supported_archs:
return arch
else:
return 'generic' |
If I am not mistaken this should be merged with #13 |
I think I'm opposed to this, or at least I'd like it to be behind a different environment variable like Can you just set the The problem is it's just really hard to test the logic that does the hardware detection, as basically nobody is going to have the test rig that exercises all the combinations of OS and hardware. The person submitting the patch normally can't test it, the CI can't test it, and I can't test it --- which means it's really likely to break. |
I run a project which builds Arm platform wheels for raspberry pi. It's automated to build everything by running |
Hmm okay. I see how having to do special-case stuff per library is bad. Would it help if we made it a setup arg? I think that can be put into a requirements file. The good news is that the Python packaging ecosystem has now rolled out multilinux2014 containers, so I've managed to update our wheel-building machinery to use the new format. I'm hoping this means we'll be able to build and distribute ARM wheels on pip. |
I think we can do a better job here for linux at least. I definitely don't think the default should be "x86_64", which doesn't even compile for gcc 8 currently. I think we can detect whether the compiler supports particular flags and do reasonably well here: #44 I also can't test with every possible combination and this may still break in some cases, but I think it should work for a much larger percentage of typical users who are compiling from source without requiring extra settings. I tested it for the conda-forge builds ( The current status where the default |
Thanks @adrianeboyd! I will keep an eye out when a release is made and see if the piwheels build succeeds. Others can keep track here: https://www.piwheels.org/project/blis/ |
I can build wheels without issues locally on a Raspberry Pi 4, so I hope it's all much easier now! |
I wondered if it could be possible to infer the architecture in
setup.py
by usingplatform.machine()
from the standard library?The environment variable could remain as an override option.
Example proposed change:
I noticed that
platform
is imported insetup.py
but never used, so I looked at the commit (28cfda4) that added it and found that you used to do something similar:Background: I run a Raspberry Pi package repo at piwheels.org and it was reported that the blis package had failed builds. Looking into it I saw that
BLIS_ARCH="generic"
is required (not really doable for piwheels' automation). Issue: piwheels/packages#12The text was updated successfully, but these errors were encountered: