Allow architecture to be overriden by CFLAGS

david-cortes / isotree

(Python, R, C/C++) Isolation Forest and variations such as SCiForest and EIF, with some additions (outlier detection + similarity + NA imputation)

https://isotree.readthedocs.io

BSD 2-Clause "Simplified" License

186 stars 38 forks source link

Allow architecture to be overriden by CFLAGS #34

Closed DenisKhrutsky closed 2 years ago

DenisKhrutsky commented 2 years ago

I tried using isotree in Python and noticed a problem when building binaries. It's currently using -march=native and does not allow it to be overridden by CFLAGS. This means that if you build a Docker image it's not guaranteed to run on another machine. Especially this applies to CI/CD pipelines where Docker images are built on AWS EC2 instances from different generations.

It would be great to allow overriding this option through CFLAGS. Currently it's not possible because CFLAGS is put before -march=native from setup script so the compiler uses the latter option.

david-cortes commented 2 years ago

Thanks, hadn't thought about it. Before merging, I think however:

It should also look for -mcpu in the flags just in case.
For -march and -mcpu, should detect whether the flag starts with it, otherwise it wouldn't pick patterns like -march=haswell
Would be good to also look at CXXFLAGS.

DenisKhrutsky commented 2 years ago

I've added -mcpu to the list as well as CXXFLAGS check. But there's no need in detecting the start of the flag, we can just check whether a flag is present in the environment variable string. And your example will be detected since -march is a part of the ... -march=haswell ... string.

david-cortes commented 2 years ago

Ypu're right, thanks.