bodono / scs-python

Python interface for SCS
MIT License
41 stars 33 forks source link

meson build: fall back to dependancy if find_library fails #78

Closed ghost closed 11 months ago

ghost commented 11 months ago

find_library only does a basic search for libs where dependency can use cmake and pkg-config. this is helpful on systems like nix where libraries are located over may uniquely named directories

https://github.com/NixOS/nixpkgs/pull/268178

bodono commented 11 months ago

Thanks for doing this! It looks good to me, but I don't know anything about meson. Paging @enzbus (who is the expert here) to take a look.

enzbus commented 11 months ago

This https://github.com/mesonbuild/meson/issues/6405#issuecomment-570772914 confirms OP statement. Probably all find_library can be switched. Does it pass the CI?

enzbus commented 11 months ago

It seems it fails on linux, we could try to do apt install libopenblas-dev since it doesn't pick the conda blas. Weird, looks like find_library is more robust.

bodono commented 11 months ago

It seems it fails on linux, we could try to do apt install libopenblas-dev since it doesn't pick the conda blas. Weird, looks like find_library is more robust.

Hmmmmm, that's strange. Not sure what the solution is here.

enzbus commented 11 months ago

find_library only does a basic search for libs where dependency can use cmake and pkg-config. this is helpful on systems like nix where libraries are located over may uniquely named directories

https://github.com/NixOS/nixpkgs/pull/268178

@a-n-n-a-l-e-e do you have a clear usecase for this? it breaks our CI, you're right that what you're pointing is probably a more canonical way to do it but I'm not sure it's worth breaking what we have. This meson.build was adapted from numpy and scipy's.

enzbus commented 11 months ago

Also, you probably know this but this is only for building pip packages. scs proper has a separate build system, and repository.

ghost commented 11 months ago

It seems it fails on linux, we could try to do apt install libopenblas-dev since it doesn't pick the conda blas. Weird, looks like find_library is more robust.

does seem that way -- i would've expect it to fall back on find_library, seems like the openblas.pc. scipy seems to use blas = dependency(['openblas', 'OpenBLAS']).

the use case i have to to work better on nix where the libraries are in unique directories.

enzbus commented 11 months ago

If the CI fails again (let's see) you could instead of replacing the find_library(), add an extra if else clause to also do dependency().

ghost commented 11 months ago

If the CI fails again (let's see) you could instead of replacing the find_library(), add an extra if else clause to also do dependency()

looking at the docs dependancy defaults to auto detect, which is pkg-config + cmake + OSX thing. but there is a way to get it to also use system, which i think is the same as find_library https://mesonbuild.com/Dependencies.html#dependency-detection-method

let me see if i can get something that works. I appreciate you trying the CI.

ghost commented 11 months ago

If the CI fails again (let's see) you could instead of replacing the find_library(), add an extra if else clause to also do dependency().

yeah -- this seems like the best option. unfortunately, the method: arg for dependency only takes a string, not a list of options. will update soon -- but looks kind of gross. sorry.

bodono commented 11 months ago

All checks pass! Is this ready to merge?

ghost commented 11 months ago

All checks pass! Is this ready to merge?

yes

ghost commented 11 months ago

woot! thanks!

ghost commented 11 months ago

so -- it seems that openblas 0.3.22 renamed the pkg-config file from openblas.pc -> openblas64.pc. so when checking for the openblas module probably want to have dependancy(['openblas64', 'openblas', ...], ...). looking at the CI logs i can't tell what version of openblas it is building with, but if version 0.3.22 or later, could've been why my initial change was failing.

https://github.com/OpenMathLib/OpenBLAS/pull/3791

bodono commented 11 months ago

I presume openblas64 is using 64 bit integers? If so when linking against it we need to pass the compilation flag BLAS64 = 1.

ghost commented 11 months ago

I presume openblas64 is using 64 bit integers? If so when linking against it we need to pass the compilation flag BLAS64 = 1.

thanks, i missed this. I don't think ILP64 is what we want. the nixos openblas build recently changed from LP64 to ILP64. investigating...

ghost commented 11 months ago

i was confused and thought the 64 indicated pointer width, then further confused with ILP64 that the 8 byte integers don't match the c. nixos didn't recently change from LP64 to ILP64, it is just that the meta data in the pkg-config file now indicates the 8 byte integer width (nixos provides both libraries, and i just happened to be working on fixing the build of a package that was erroneously using the ILP64 version).

for nixos -- the python-scs uses openblas LP64 (except for darwin, as that uses accelerate). that matches what numpy is using, unless there is a compelling reason to build for ILP64 matching numpy is simpler.

enzbus commented 11 months ago

Feel free to try with openblas64 via dependency, if it passes the CI we can drop the initial find_library clauses which would simplify the code a little.

ghost commented 11 months ago

I don't think there is any pkg-config or cmake configuration data in the CI -- at least from looking at the files in the openblas-devel fedora package. in contrast with debian, ubuntu and nix which provide the configuration files to pkg-config and cmake

lib/.../cmake/openblas/OpenBLASConfig.cmake
lib/.../cmake/openblas/OpenBLASConfigVersion.cmake
lib/.../pkgconfig/blas-openblas.pc
lib/.../pkgconfig/lapack-openblas.pc
lib/.../pkgconfig/openblas.pc

i was mistaken to suggest openblas64.pc or that is for ILP64. LP64 uses openblas.pc

i will keep my eye out for some way that the two functions can be combined. i feel like i made a bit of a mess out of that file.

enzbus commented 11 months ago

OK so we keep the current (master branch) version, let us know if things work on your side.