| Message ID | 20210725111231.359830-3-arnout@mind.be |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v5,1/3] package/python-numpy: use lapack instead of clapack | expand |
Arnout, all, On Sun, 25 Jul 2021 13:12:31 +0200 "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> wrote: > From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > > armadillo can use lapack or openblas as BLAS provider. LAPACK support is > optional. > > This patch > - adds an _ARCH_SUPPORTS variable to check if one is available > - adds an option to choose lapack or openblas as BLAS provider > > The choice is required since applications may potentially need lapack. > > Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > Changes v4 -> v5: [Done by Arnout] > - Consistent naming of BR2_PACKAGE_ARMADILLO_OPENBLAS (Yann E. Morin) > - Remove double dependency on lapack/clapack (Yann E. Morin) > - Remove stray 'endchoice' > - "Solve" circular dependency by removing clapack > Changes v3 -> v4: [Done by Arnout] > - Split off lapack/clapack arch depends in separate patches > - Simplify the comments (no need for the powerpc complexity) > - Remove the choices because clapack/lapack choice doesn't work. Keep a > single config for openblas. > Changes v2 -> v3: > - drop all default statements for choice (Thomas) > - add explicit -l since blas libary is called liblas for (c)lapack and > libopenblas for openblas (Thomas) > - add a choice for lapack selection between lapack, clapack or none > Changes v1 -> v2: > - add openblas as blas provider > --- > package/armadillo/Config.in | 34 ++++++++++++++++++++++++++-------- > package/armadillo/armadillo.mk | 18 +++++++++++++++++- > 2 files changed, 43 insertions(+), 9 deletions(-) > > diff --git a/package/armadillo/Config.in b/package/armadillo/Config.in > index b2b61a3233..7aed4fd02f 100644 > --- a/package/armadillo/Config.in > +++ b/package/armadillo/Config.in > @@ -1,20 +1,38 @@ > comment "armadillo needs a toolchain w/ C++" > + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS blas may be provided by lapack too: maybe it's required to add the same condition as armadillo itself ? > depends on !BR2_INSTALL_LIBSTDCPP > - depends on !BR2_powerpc > - depends on !BR2_m68k_cf > > -comment "armadillo needs a glibc toolchain w/ C++" > - depends on BR2_powerpc > - depends on !BR2_INSTALL_LIBSTDCPP || BR2_TOOLCHAIN_USES_UCLIBC > +comment "armadillo needs a toolchain w/ fortran, C++" fortran is only required if lapack has to be used: if lapack is said as optional this is maybe not required ? > + depends on !BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS # otherwise, see comment above > + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS > + depends on !BR2_TOOLCHAIN_HAS_FORTRAN || !BR2_INSTALL_LIBSTDCPP > > config BR2_PACKAGE_ARMADILLO > bool "armadillo" > + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS || \ > + (BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN) > depends on BR2_INSTALL_LIBSTDCPP > - depends on !BR2_powerpc || BR2_TOOLCHAIN_USES_GLIBC # clapack > - depends on !BR2_m68k_cf # clapack > - select BR2_PACKAGE_CLAPACK > help > Armadillo: An Open Source C++ Linear Algebra Library for > Fast Prototyping and Computationally Intensive Experiments. > > http://arma.sourceforge.net/ > + > +if BR2_PACKAGE_ARMADILLO > + > +choice > + prompt "BLAS implementation" > + > +config BR2_PACKAGE_ARMADILLO_OPENBLAS > + bool "openblas" > + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > + select BR2_PACKAGE_OPENBLAS > + > +config BR2_PACKAGE_ARMADILLO_LAPACK > + bool "lapack" > + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN > + select BR2_PACKAGE_LAPACK > + > +endchoice > + > +endif > diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk > index 624b842ef6..dcac2bf8cd 100644 > --- a/package/armadillo/armadillo.mk > +++ b/package/armadillo/armadillo.mk > @@ -7,11 +7,27 @@ > ARMADILLO_VERSION = 9.900.2 > ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz > ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma > -ARMADILLO_DEPENDENCIES = clapack > ARMADILLO_INSTALL_STAGING = YES > ARMADILLO_LICENSE = Apache-2.0 > ARMADILLO_LICENSE_FILES = LICENSE.txt > > ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false > > +# blas support may be provided by lapack (libblas.a) or openblas (libopenblas.a) > +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON > +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) > +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas > +ARMADILLO_DEPENDENCIES = openblas > +else > +# Since BR2_PACKAGE_LAPACK is selected in this case, the dependency on it is > +# added below. > +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas > +endif > + > +# lapack support is optional and can only be provided by lapack, not openblas > +ifeq ($(BR2_PACKAGE_LAPACK),y) > +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON > +ARMADILLO_DEPENDENCIES += lapack > +endif > + > $(eval $(cmake-package)) > -- > 2.31.1 > It's an open question: until now armadillo was always built with blas and lapack support provided by clapack. To avoid breaking current behaviour (lapack support) why not considering selecting lapack package unconditionaly and let user choise for blas support? I know the main issue is the dependency to fortran... Thanks Gwen
On 25/07/2021 17:57, Gwenhael Goavec-Merou wrote: > Arnout, all, > > On Sun, 25 Jul 2021 13:12:31 +0200 > "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> wrote: > >> From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> >> >> armadillo can use lapack or openblas as BLAS provider. LAPACK support is >> optional. >> >> This patch >> - adds an _ARCH_SUPPORTS variable to check if one is available >> - adds an option to choose lapack or openblas as BLAS provider >> >> The choice is required since applications may potentially need lapack. >> >> Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> >> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> >> --- >> Changes v4 -> v5: [Done by Arnout] >> - Consistent naming of BR2_PACKAGE_ARMADILLO_OPENBLAS (Yann E. Morin) >> - Remove double dependency on lapack/clapack (Yann E. Morin) >> - Remove stray 'endchoice' >> - "Solve" circular dependency by removing clapack >> Changes v3 -> v4: [Done by Arnout] >> - Split off lapack/clapack arch depends in separate patches >> - Simplify the comments (no need for the powerpc complexity) >> - Remove the choices because clapack/lapack choice doesn't work. Keep a >> single config for openblas. >> Changes v2 -> v3: >> - drop all default statements for choice (Thomas) >> - add explicit -l since blas libary is called liblas for (c)lapack and >> libopenblas for openblas (Thomas) >> - add a choice for lapack selection between lapack, clapack or none >> Changes v1 -> v2: >> - add openblas as blas provider >> --- >> package/armadillo/Config.in | 34 ++++++++++++++++++++++++++-------- >> package/armadillo/armadillo.mk | 18 +++++++++++++++++- >> 2 files changed, 43 insertions(+), 9 deletions(-) >> >> diff --git a/package/armadillo/Config.in b/package/armadillo/Config.in >> index b2b61a3233..7aed4fd02f 100644 >> --- a/package/armadillo/Config.in >> +++ b/package/armadillo/Config.in >> @@ -1,20 +1,38 @@ >> comment "armadillo needs a toolchain w/ C++" >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > blas may be provided by lapack too: maybe it's required to add the same condition > as armadillo itself ? >> depends on !BR2_INSTALL_LIBSTDCPP >> - depends on !BR2_powerpc >> - depends on !BR2_m68k_cf >> >> -comment "armadillo needs a glibc toolchain w/ C++" >> - depends on BR2_powerpc >> - depends on !BR2_INSTALL_LIBSTDCPP || BR2_TOOLCHAIN_USES_UCLIBC >> +comment "armadillo needs a toolchain w/ fortran, C++" > fortran is only required if lapack has to be used: if lapack is said as optional this is > maybe not required ? Oh, I intended to insert a comment there but apparently forgot: We need either openblas (which doesn't require fortran) or lapack (which does require fortran). Therefore, on architectures which do support openblas, we should have a comment that just says "C++", while on architectures that don't support openblas (but do support lapack) we should have a comment that says "C++ and Fortran". >> + depends on !BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS # otherwise, see comment above >> + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS >> + depends on !BR2_TOOLCHAIN_HAS_FORTRAN || !BR2_INSTALL_LIBSTDCPP >> >> config BR2_PACKAGE_ARMADILLO >> bool "armadillo" >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS || \ >> + (BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN) >> depends on BR2_INSTALL_LIBSTDCPP >> - depends on !BR2_powerpc || BR2_TOOLCHAIN_USES_GLIBC # clapack >> - depends on !BR2_m68k_cf # clapack >> - select BR2_PACKAGE_CLAPACK >> help >> Armadillo: An Open Source C++ Linear Algebra Library for >> Fast Prototyping and Computationally Intensive Experiments. >> >> http://arma.sourceforge.net/ >> + >> +if BR2_PACKAGE_ARMADILLO >> + >> +choice >> + prompt "BLAS implementation" >> + >> +config BR2_PACKAGE_ARMADILLO_OPENBLAS >> + bool "openblas" >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS >> + select BR2_PACKAGE_OPENBLAS >> + >> +config BR2_PACKAGE_ARMADILLO_LAPACK >> + bool "lapack" >> + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN >> + select BR2_PACKAGE_LAPACK >> + >> +endchoice >> + >> +endif >> diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk >> index 624b842ef6..dcac2bf8cd 100644 >> --- a/package/armadillo/armadillo.mk >> +++ b/package/armadillo/armadillo.mk >> @@ -7,11 +7,27 @@ >> ARMADILLO_VERSION = 9.900.2 >> ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz >> ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma >> -ARMADILLO_DEPENDENCIES = clapack >> ARMADILLO_INSTALL_STAGING = YES >> ARMADILLO_LICENSE = Apache-2.0 >> ARMADILLO_LICENSE_FILES = LICENSE.txt >> >> ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false >> >> +# blas support may be provided by lapack (libblas.a) or openblas (libopenblas.a) >> +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON >> +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) >> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas >> +ARMADILLO_DEPENDENCIES = openblas >> +else >> +# Since BR2_PACKAGE_LAPACK is selected in this case, the dependency on it is >> +# added below. >> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas >> +endif >> + >> +# lapack support is optional and can only be provided by lapack, not openblas >> +ifeq ($(BR2_PACKAGE_LAPACK),y) >> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON >> +ARMADILLO_DEPENDENCIES += lapack >> +endif >> + >> $(eval $(cmake-package)) >> -- >> 2.31.1 >> > It's an open question: > until now armadillo was always built with blas and lapack support provided by > clapack. To avoid breaking current behaviour (lapack support) why not > considering selecting lapack package unconditionaly and let user choise for > blas support? I know the main issue is the dependency to fortran... What you're basically saying is that in the choice between lapack and openblas, lapack should be the default, right? Can you maybe explain a bit what your original reason for this patch was? Armadillo basically hides the BLAS implementation, so unless it's buggy, you shouldn't even notice which implementation is behind. Also, the lapack package itself says that you should not use its BLAS implementation but you should use a better one instead. Regards, Arnout
On Mon, 26 Jul 2021 08:24:37 +0200 Arnout Vandecappelle <arnout@mind.be> wrote: > On 25/07/2021 17:57, Gwenhael Goavec-Merou wrote: > > Arnout, all, > > > > On Sun, 25 Jul 2021 13:12:31 +0200 > > "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> wrote: > > > >> From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > >> > >> armadillo can use lapack or openblas as BLAS provider. LAPACK support is > >> optional. > >> > >> This patch > >> - adds an _ARCH_SUPPORTS variable to check if one is available > >> - adds an option to choose lapack or openblas as BLAS provider > >> > >> The choice is required since applications may potentially need lapack. > >> > >> Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > >> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > >> --- > >> Changes v4 -> v5: [Done by Arnout] > >> - Consistent naming of BR2_PACKAGE_ARMADILLO_OPENBLAS (Yann E. Morin) > >> - Remove double dependency on lapack/clapack (Yann E. Morin) > >> - Remove stray 'endchoice' > >> - "Solve" circular dependency by removing clapack > >> Changes v3 -> v4: [Done by Arnout] > >> - Split off lapack/clapack arch depends in separate patches > >> - Simplify the comments (no need for the powerpc complexity) > >> - Remove the choices because clapack/lapack choice doesn't work. Keep a > >> single config for openblas. > >> Changes v2 -> v3: > >> - drop all default statements for choice (Thomas) > >> - add explicit -l since blas libary is called liblas for (c)lapack and > >> libopenblas for openblas (Thomas) > >> - add a choice for lapack selection between lapack, clapack or none > >> Changes v1 -> v2: > >> - add openblas as blas provider > >> --- > >> package/armadillo/Config.in | 34 ++++++++++++++++++++++++++-------- > >> package/armadillo/armadillo.mk | 18 +++++++++++++++++- > >> 2 files changed, 43 insertions(+), 9 deletions(-) > >> > >> diff --git a/package/armadillo/Config.in b/package/armadillo/Config.in > >> index b2b61a3233..7aed4fd02f 100644 > >> --- a/package/armadillo/Config.in > >> +++ b/package/armadillo/Config.in > >> @@ -1,20 +1,38 @@ > >> comment "armadillo needs a toolchain w/ C++" > >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > > blas may be provided by lapack too: maybe it's required to add the same > > condition as armadillo itself ? > >> depends on !BR2_INSTALL_LIBSTDCPP > >> - depends on !BR2_powerpc > >> - depends on !BR2_m68k_cf > >> > >> -comment "armadillo needs a glibc toolchain w/ C++" > >> - depends on BR2_powerpc > >> - depends on !BR2_INSTALL_LIBSTDCPP || BR2_TOOLCHAIN_USES_UCLIBC > >> +comment "armadillo needs a toolchain w/ fortran, C++" > > fortran is only required if lapack has to be used: if lapack is said as > > optional this is maybe not required ? > > Oh, I intended to insert a comment there but apparently forgot: We need > either openblas (which doesn't require fortran) or lapack (which does require > fortran). Therefore, on architectures which do support openblas, we should > have a comment that just says "C++", while on architectures that don't > support openblas (but do support lapack) we should have a comment that says > "C++ and Fortran". > Ok. > > >> + depends on !BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS # otherwise, see > >> comment above > >> + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS > >> + depends on !BR2_TOOLCHAIN_HAS_FORTRAN || !BR2_INSTALL_LIBSTDCPP > >> > >> config BR2_PACKAGE_ARMADILLO > >> bool "armadillo" > >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS || \ > >> + (BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && > >> BR2_TOOLCHAIN_HAS_FORTRAN) depends on BR2_INSTALL_LIBSTDCPP > >> - depends on !BR2_powerpc || BR2_TOOLCHAIN_USES_GLIBC # clapack > >> - depends on !BR2_m68k_cf # clapack > >> - select BR2_PACKAGE_CLAPACK > >> help > >> Armadillo: An Open Source C++ Linear Algebra Library for > >> Fast Prototyping and Computationally Intensive Experiments. > >> > >> http://arma.sourceforge.net/ > >> + > >> +if BR2_PACKAGE_ARMADILLO > >> + > >> +choice > >> + prompt "BLAS implementation" > >> + > >> +config BR2_PACKAGE_ARMADILLO_OPENBLAS > >> + bool "openblas" > >> + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > >> + select BR2_PACKAGE_OPENBLAS > >> + > >> +config BR2_PACKAGE_ARMADILLO_LAPACK > >> + bool "lapack" > >> + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && > >> BR2_TOOLCHAIN_HAS_FORTRAN > >> + select BR2_PACKAGE_LAPACK > >> + > >> +endchoice > >> + > >> +endif > >> diff --git a/package/armadillo/armadillo.mk > >> b/package/armadillo/armadillo.mk index 624b842ef6..dcac2bf8cd 100644 > >> --- a/package/armadillo/armadillo.mk > >> +++ b/package/armadillo/armadillo.mk > >> @@ -7,11 +7,27 @@ > >> ARMADILLO_VERSION = 9.900.2 > >> ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz > >> ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma > >> -ARMADILLO_DEPENDENCIES = clapack > >> ARMADILLO_INSTALL_STAGING = YES > >> ARMADILLO_LICENSE = Apache-2.0 > >> ARMADILLO_LICENSE_FILES = LICENSE.txt > >> > >> ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false > >> > >> +# blas support may be provided by lapack (libblas.a) or openblas > >> (libopenblas.a) +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON > >> +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) > >> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas > >> +ARMADILLO_DEPENDENCIES = openblas > >> +else > >> +# Since BR2_PACKAGE_LAPACK is selected in this case, the dependency on it > >> is +# added below. > >> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas > >> +endif > >> + > >> +# lapack support is optional and can only be provided by lapack, not > >> openblas +ifeq ($(BR2_PACKAGE_LAPACK),y) > >> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON > >> +ARMADILLO_DEPENDENCIES += lapack > >> +endif > >> + > >> $(eval $(cmake-package)) > >> -- > >> 2.31.1 > >> > > It's an open question: > > until now armadillo was always built with blas and lapack support provided > > by clapack. To avoid breaking current behaviour (lapack support) why not > > considering selecting lapack package unconditionaly and let user choise for > > blas support? I know the main issue is the dependency to fortran... > > What you're basically saying is that in the choice between lapack and > openblas, lapack should be the default, right? > > Can you maybe explain a bit what your original reason for this patch was? > Armadillo basically hides the BLAS implementation, so unless it's buggy, you > shouldn't even notice which implementation is behind. Also, the lapack package > itself says that you should not use its BLAS implementation but you should > use a better one instead. > At the beginning the first version was to allows users to select between clapack and lapack (This is because one application I use need armadillo but crash if this library is build using clapack) for armadillo's lapack functions. After that, Romain has asked to extends this patch to have choice, for blas functions between all providers. For armadillo's blas part if lapack said to prefer openblas it's seems logical to add the priority to this library. But for armadillo's lapack part, when lapack is not available, everything about lapack in armadillo will be disabled. Current behaviour is to select CLAPACK, and with this patch default behaviour is to enable lapack functions only when lapack is selected. > Regards, > Arnout Regards, Gwenhael
On Sun, 25 Jul 2021 13:12:31 +0200 "Arnout Vandecappelle (Essensium/Mind)" <arnout@mind.be> wrote: > From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > > armadillo can use lapack or openblas as BLAS provider. LAPACK support is > optional. > > This patch > - adds an _ARCH_SUPPORTS variable to check if one is available > - adds an option to choose lapack or openblas as BLAS provider > > The choice is required since applications may potentially need lapack. > > Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com> > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > Changes v4 -> v5: [Done by Arnout] > - Consistent naming of BR2_PACKAGE_ARMADILLO_OPENBLAS (Yann E. Morin) > - Remove double dependency on lapack/clapack (Yann E. Morin) > - Remove stray 'endchoice' > - "Solve" circular dependency by removing clapack > Changes v3 -> v4: [Done by Arnout] > - Split off lapack/clapack arch depends in separate patches > - Simplify the comments (no need for the powerpc complexity) > - Remove the choices because clapack/lapack choice doesn't work. Keep a > single config for openblas. > Changes v2 -> v3: > - drop all default statements for choice (Thomas) > - add explicit -l since blas libary is called liblas for (c)lapack and > libopenblas for openblas (Thomas) > - add a choice for lapack selection between lapack, clapack or none > Changes v1 -> v2: > - add openblas as blas provider > --- > package/armadillo/Config.in | 34 ++++++++++++++++++++++++++-------- > package/armadillo/armadillo.mk | 18 +++++++++++++++++- > 2 files changed, 43 insertions(+), 9 deletions(-) Applied to master, thanks. Thomas
Hello, On Mon, 26 Jul 2021 08:24:37 +0200 Arnout Vandecappelle <arnout@mind.be> wrote: > > fortran is only required if lapack has to be used: if lapack is said as optional this is > > maybe not required ? > > Oh, I intended to insert a comment there but apparently forgot: We need either > openblas (which doesn't require fortran) or lapack (which does require fortran). > Therefore, on architectures which do support openblas, we should have a comment > that just says "C++", while on architectures that don't support openblas (but do > support lapack) we should have a comment that says "C++ and Fortran". ... but this is exactly what your patch does: comment "armadillo needs a toolchain w/ C++" depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS depends on !BR2_INSTALL_LIBSTDCPP comment "armadillo needs a toolchain w/ fortran, C++" depends on !BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS # otherwise, see comment above depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS depends on !BR2_TOOLCHAIN_HAS_FORTRAN || !BR2_INSTALL_LIBSTDCPP If we're on an arch supported by openblas, "needs a toolchain w/ C++" is displayed. If we're on an arch not supported by openblas, but supported by lapack, "needs a toolchain w/ fortran, C++" is displayed. So it all looks good to me. > > It's an open question: > > until now armadillo was always built with blas and lapack support provided by > > clapack. To avoid breaking current behaviour (lapack support) why not > > considering selecting lapack package unconditionaly and let user choise for > > blas support? I know the main issue is the dependency to fortran... > > What you're basically saying is that in the choice between lapack and openblas, > lapack should be the default, right? > > Can you maybe explain a bit what your original reason for this patch was? > Armadillo basically hides the BLAS implementation, so unless it's buggy, you > shouldn't even notice which implementation is behind. Also, the lapack package > itself says that you should not use its BLAS implementation but you should use a > better one instead. I was also confused by Gwenhael's comment, including with his reply to your question. So at this point, I've applied your patch as-is, we can always improve/fix later on if needed. Best regards, Thomas
On 03/08/2021 22:57, Thomas Petazzoni wrote: > Hello, > > On Mon, 26 Jul 2021 08:24:37 +0200 > Arnout Vandecappelle <arnout@mind.be> wrote: > >>> fortran is only required if lapack has to be used: if lapack is said as optional this is >>> maybe not required ? >> >> Oh, I intended to insert a comment there but apparently forgot: We need either >> openblas (which doesn't require fortran) or lapack (which does require fortran). >> Therefore, on architectures which do support openblas, we should have a comment >> that just says "C++", while on architectures that don't support openblas (but do >> support lapack) we should have a comment that says "C++ and Fortran". > > ... but this is exactly what your patch does: I meant a "#" comment, not a "comment" comment :-) Regards, Arnout > > comment "armadillo needs a toolchain w/ C++" > depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS > depends on !BR2_INSTALL_LIBSTDCPP > > comment "armadillo needs a toolchain w/ fortran, C++" > depends on !BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS # otherwise, see comment above > depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS > depends on !BR2_TOOLCHAIN_HAS_FORTRAN || !BR2_INSTALL_LIBSTDCPP > > If we're on an arch supported by openblas, "needs a toolchain w/ C++" > is displayed. > > If we're on an arch not supported by openblas, but supported by lapack, > "needs a toolchain w/ fortran, C++" is displayed. > > So it all looks good to me. > > >>> It's an open question: >>> until now armadillo was always built with blas and lapack support provided by >>> clapack. To avoid breaking current behaviour (lapack support) why not >>> considering selecting lapack package unconditionaly and let user choise for >>> blas support? I know the main issue is the dependency to fortran... >> >> What you're basically saying is that in the choice between lapack and openblas, >> lapack should be the default, right? >> >> Can you maybe explain a bit what your original reason for this patch was? >> Armadillo basically hides the BLAS implementation, so unless it's buggy, you >> shouldn't even notice which implementation is behind. Also, the lapack package >> itself says that you should not use its BLAS implementation but you should use a >> better one instead. > > I was also confused by Gwenhael's comment, including with his reply to > your question. So at this point, I've applied your patch as-is, we can > always improve/fix later on if needed. > > Best regards, > > Thomas >
diff --git a/package/armadillo/Config.in b/package/armadillo/Config.in index b2b61a3233..7aed4fd02f 100644 --- a/package/armadillo/Config.in +++ b/package/armadillo/Config.in @@ -1,20 +1,38 @@ comment "armadillo needs a toolchain w/ C++" + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS depends on !BR2_INSTALL_LIBSTDCPP - depends on !BR2_powerpc - depends on !BR2_m68k_cf -comment "armadillo needs a glibc toolchain w/ C++" - depends on BR2_powerpc - depends on !BR2_INSTALL_LIBSTDCPP || BR2_TOOLCHAIN_USES_UCLIBC +comment "armadillo needs a toolchain w/ fortran, C++" + depends on !BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS # otherwise, see comment above + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS + depends on !BR2_TOOLCHAIN_HAS_FORTRAN || !BR2_INSTALL_LIBSTDCPP config BR2_PACKAGE_ARMADILLO bool "armadillo" + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS || \ + (BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN) depends on BR2_INSTALL_LIBSTDCPP - depends on !BR2_powerpc || BR2_TOOLCHAIN_USES_GLIBC # clapack - depends on !BR2_m68k_cf # clapack - select BR2_PACKAGE_CLAPACK help Armadillo: An Open Source C++ Linear Algebra Library for Fast Prototyping and Computationally Intensive Experiments. http://arma.sourceforge.net/ + +if BR2_PACKAGE_ARMADILLO + +choice + prompt "BLAS implementation" + +config BR2_PACKAGE_ARMADILLO_OPENBLAS + bool "openblas" + depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS + select BR2_PACKAGE_OPENBLAS + +config BR2_PACKAGE_ARMADILLO_LAPACK + bool "lapack" + depends on BR2_PACKAGE_LAPACK_ARCH_SUPPORTS && BR2_TOOLCHAIN_HAS_FORTRAN + select BR2_PACKAGE_LAPACK + +endchoice + +endif diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk index 624b842ef6..dcac2bf8cd 100644 --- a/package/armadillo/armadillo.mk +++ b/package/armadillo/armadillo.mk @@ -7,11 +7,27 @@ ARMADILLO_VERSION = 9.900.2 ARMADILLO_SOURCE = armadillo-$(ARMADILLO_VERSION).tar.xz ARMADILLO_SITE = https://downloads.sourceforge.net/project/arma -ARMADILLO_DEPENDENCIES = clapack ARMADILLO_INSTALL_STAGING = YES ARMADILLO_LICENSE = Apache-2.0 ARMADILLO_LICENSE_FILES = LICENSE.txt ARMADILLO_CONF_OPTS = -DDETECT_HDF5=false +# blas support may be provided by lapack (libblas.a) or openblas (libopenblas.a) +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y) +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas +ARMADILLO_DEPENDENCIES = openblas +else +# Since BR2_PACKAGE_LAPACK is selected in this case, the dependency on it is +# added below. +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas +endif + +# lapack support is optional and can only be provided by lapack, not openblas +ifeq ($(BR2_PACKAGE_LAPACK),y) +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON +ARMADILLO_DEPENDENCIES += lapack +endif + $(eval $(cmake-package))