Re: [Buildroot] [PATCH v4 3/3] package/armadillo: allows to select between clapack, lapack or openblas

[Arnout Vandecappelle <arnout@mind.be>]

On 25/07/2021 09:57, Yann E. MORIN wrote:
> Arnout, All, 
> 
> On 2021-07-24 23:45 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
>> From: Gwenhael Goavec-Merou <gwenhael.goavec-merou@trabucayre.com>

[snip]
>> +config BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS
> 
> Note the option name here ^^^ [...]
> 
>> +	bool "use openblas"
>> +	depends on BR2_PACKAGE_OPENBLAS_ARCH_SUPPORTS
>> +	select BR2_PACKAGE_OPENBLAS
>> +	help
>> +	  Use OpenBLAS as BLAS library. Without this option, clapack or lapack
>> +	  will be used.
>> +
>> +endchoice
>> +
>> +endif
>> diff --git a/package/armadillo/armadillo.mk b/package/armadillo/armadillo.mk
>> index 624b842ef6..82df7602be 100644
>> --- a/package/armadillo/armadillo.mk
>> +++ b/package/armadillo/armadillo.mk
>> @@ -7,11 +7,37 @@
>>  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, clapack or openblas
>> +# blas library from (c)lapack is libblas.a, libopenblas.a otherwise
>> +ARMADILLO_CONF_OPTS += -DBLAS_FOUND=ON
>> +ifeq ($(BR2_PACKAGE_ARMADILLO_OPENBLAS),y)
> 
> [...] so I guess you meant BR2_PACKAGE_ARMADILLO_BLAS_OPENBLAS here, no?

 See, that's why I resent this patch rather than just applying it :-)

 TBH I meant BR2_PACKAGE_ARMADILLO_OPENBLAS in Config.in, but it doesn't matter
much.

> 
>> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lopenblas
>> +ARMADILLO_DEPENDENCIES = openblas
>> +else
>> +ARMADILLO_CONF_OPTS += -DBLAS_LIBRARIES=-lblas
>> +ifeq ($(BR2_PACKAGE_CLAPACK), y)
> 
> We don't usually add a space after the comma in an ifeq, especially in
> cases like this simple test.
> 
> Why duplicate the dependency on lapack/clapack in the !openblas case,
> when the same dependencies already exist, below?
> 
> I mean: in the !openblas case, we know that either lapack or clapack are
> enabled, so we know will hit either case in the block [...]
> 
>> +ARMADILLO_DEPENDENCIES = clapack
>> +else
>> +ARMADILLO_DEPENDENCIES = lapack
>> +endif
>> +endif
> 
> [...] here:
> 
>> +# lapack support may be provided by lapack or clapack
>> +# but not by openblas
>> +ifeq ($(BR2_PACKAGE_CLAPACK),y)
>> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON
>> +ARMADILLO_DEPENDENCIES += clapack
>> +else ifeq ($(BR2_PACKAGE_LAPACK),y)
>> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=ON
>> +ARMADILLO_DEPENDENCIES += lapack
>> +else
>> +ARMADILLO_CONF_OPTS += -DLAPACK_FOUND=OFF
>> +endif
> 
> Additionally, this block will be hit even in the openblas case. Is this
> expected?

 Historical accident. This is reworked from v3 which had an explicit Config.in
option for the lapack choice

 I'll send a v5 which fixes it. I still want Gwenhael to confirm that it's good
in this shape though.

 Regards,
 Arnout

_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot