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

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

Merged
merged 2 commits into from Nov 18, 2023
Merged

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

merged 2 commits into from Nov 18, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 18, 2023

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

NixOS/nixpkgs#268178

@bodono
Copy link
Owner

bodono commented Nov 18, 2023

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.

meson.build Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@enzbus
Copy link
Contributor

enzbus commented Nov 18, 2023

This mesonbuild/meson#6405 (comment) confirms OP statement. Probably all find_library can be switched. Does it pass the CI?

@enzbus
Copy link
Contributor

enzbus commented Nov 18, 2023

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
Copy link
Owner

bodono commented Nov 18, 2023

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
Copy link
Contributor

enzbus commented Nov 18, 2023

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

NixOS/nixpkgs#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
Copy link
Contributor

enzbus commented Nov 18, 2023

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

@ghost
Copy link
Author

ghost commented Nov 18, 2023

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.

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
@enzbus
Copy link
Contributor

enzbus commented Nov 18, 2023

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
Copy link
Author

ghost commented Nov 18, 2023

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
Copy link
Author

ghost commented Nov 18, 2023

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
Copy link
Owner

bodono commented Nov 18, 2023

All checks pass! Is this ready to merge?

@ghost ghost changed the title meson build: find libs with dependancy rather than find_library meson build: fall back to dependancy if find_library fails Nov 18, 2023
@ghost
Copy link
Author

ghost commented Nov 18, 2023

All checks pass! Is this ready to merge?

yes

@bodono bodono merged commit dd17e2e into bodono:master Nov 18, 2023
47 checks passed
@ghost ghost deleted the meson-use-dependancy branch November 18, 2023 22:54
@ghost
Copy link
Author

ghost commented Nov 18, 2023

woot! thanks!

@ghost
Copy link
Author

ghost commented Nov 19, 2023

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.

OpenMathLib/OpenBLAS#3791

@bodono
Copy link
Owner

bodono commented Nov 19, 2023

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

@ghost
Copy link
Author

ghost commented Nov 19, 2023

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
Copy link
Author

ghost commented Nov 20, 2023

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
Copy link
Contributor

enzbus commented Nov 20, 2023

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
Copy link
Author

ghost commented Nov 20, 2023

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
Copy link
Contributor

enzbus commented Nov 20, 2023

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

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