RubixML / Tensor

A library and extension that provides objects for scientific computing in PHP.
https://rubixml.com
MIT License
223 stars 27 forks source link

check for openblas headers #11

Closed remicollet closed 3 years ago

remicollet commented 3 years ago

At least because not found when installed in /usr/include/openblas

github-actions[bot] commented 3 years ago

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

remicollet commented 3 years ago

Notice if ! test "x-lopenblas -llapacke -lgfortran -lpthread" = "x"; then

This test doesn't make sense.... always true

PHP_EVAL_LIBLINE(-lopenblas -llapacke -lgfortran -lpthread, TENSOR_SHARED_LIBADD)

It seems only -lopenblas is required

IIUC, you have blas + lapack OR openblas...

remicollet commented 3 years ago

From a quick test, with only -lopenblas

$ php -n -d extension=json -d extension=modules/tensor.so --ri tensor

tensor

Tensor is a library and extension that provides objects for scientific computing in PHP.
tensor => enabled
Author => Andrew DalPino
Version => 2.1.3
Build Date => Feb 15 2021 09:54:31
Powered by Zephir => Version 0.12.20-$Id$

$ ldd modules/tensor.so 
    linux-vdso.so.1 (0x00007ffdac769000)
    libopenblas.so.0 => /lib64/libopenblas.so.0 (0x00007f0dca02c000)
    libc.so.6 => /lib64/libc.so.6 (0x00007f0dc9e61000)
    libm.so.6 => /lib64/libm.so.6 (0x00007f0dc9d1b000)
    libgfortran.so.5 => /lib64/libgfortran.so.5 (0x00007f0dc9a60000)
    /lib64/ld-linux-x86-64.so.2 (0x00007f0dcc5b1000)
    libquadmath.so.0 => /lib64/libquadmath.so.0 (0x00007f0dc9a16000)
    libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f0dc99fb000)

P.S. PHP is built with --enable-rtld-now, so all used symbols are there.

andrewdalpino commented 3 years ago

Thanks @remicollet but this file is autogenerated by Zephir I believe

https://zephir-lang.com/

From that standpoint, is there anything we can do to fix this?

remicollet commented 3 years ago

Thanks @remicollet but this file is autogenerated by Zephir I believe

I have huge doubt about this... in all case, this failed is really wrong

andrewdalpino commented 3 years ago

Thanks @remicollet but this file is autogenerated by Zephir I believe

I have huge doubt about this... in all case, this failed is really wrong

Hmm, well indeed Zephir does modify this file

But perhaps the Zephir parser/compiler won't overwrite our changes

If you don't feel like testing it yourself, I'll be happy to myself as soon as I have some time

remicollet commented 3 years ago

FYI, using this patch (and also this one) I was able to build this extension on Fedora 31 to 33, RHEL 7 and 8, for PHP 7.2, 7.3 and 7.4

So it is now available, among other extensions in my repository https://twitter.com/RemiRepository/status/1361256662359822337 https://blog.remirepo.net/pages/PECL-extensions-RPM-status#c9433

andrewdalpino commented 3 years ago

Hey thanks alot @remicollet you are awesome!

All of this is new to me, I just assumed Zephir would set us up for success out of the box. Can you help me understand the issue we're facing here?

Also, do you think it would a good idea to team up with the Zephir people to fix it from a higher level?

remicollet commented 3 years ago

Can you help me understand the issue we're facing here?

On RPM distro, cblas.h and lapack.h headers are in /usr/include/openblas so are not found during the build

andrewdalpino commented 3 years ago

Thanks for breaking that down for me @remicollet

It's looking like the config.w4 file will get overwritten by the Zephir compilation to C process. See https://github.com/zephir-lang/zephir/issues/2150

Can you think of another way in which we can approach this issue? Would remembering to manually add this code before every release work as a temporary fix for now?

andrewdalpino commented 3 years ago

Since this file is overwritten by Zephir during compilation, I went ahead and created a patch file that adds these checks after compilation. However, compilation fails on Ubuntu now as the OpenBLAS headers are located in /usr/include/x86_64-linux-gnu.