armadillo/10.7.0

[samuel-emrys]

Specify library name and version: armadillo/10.6.1

Armadillo is a math library that I'd like to use. From a quick survey it's a dependency of the following packages:

• gdal - [request] gdal/3.0.4 #1023
• mlpack: MLPack - popular machine learning library wishlist#106
• ensmallen: [request] ensmallen/2.17.0 #6797

It's also on the list of most relevant c++ libraries not already packaged:

• [request] some of most relevant c++ libraries in github (by number of stars) #621

This recipe is designed to be as true to the original packaging as it feasibly could. Of note, this means:

• Option names match the configuration settings exposed by armadillo
• Retains the ability to search for system libraries if specified - specifically, it can be configured to use libraries that aren't currently packaged by conan as system dependencies (these are optional, though, and not used by default):
  • ATLAS - [request] ATLAS/3.10.3 #6757
  • SuperLU - [request] SuperLU/5.2.2 #6756
  • ARPACK - [request] ARPACK/1.0 #6755
  • MKL - [request] MKL/2021.3 #6801
  • FlexiBLAS - [request] FlexiBLAS/3.0.4 #6827
  • LAPACK - [request] lapack/3.9.0 #4509

By default, this package utilises OpenBLAS to fulfill its BLAS and LAPACK requirements. It uses OpenBLAS by default on Linux and Windows operating systems, and configures it to build with LAPACK support. On Macos, it's configured to use the Accelerate framework by default. This usage of OpenBLAS introduces an upstream system dependency on a fortran compiler in order to provide LAPACK support.

Armadillo is capable of running without LAPACK support, however it only runs with a subset of features and the provided example code will fail with an error indicating LAPACK functionality needs to be provided when attempting to execute a command that requires it.

Of note, this recipe will introduce opaque errors if the usage of LAPACK is not disabled and a fortran compiler is not present on the system. This is because OpenBLAS will continue to build when configured to build with LAPACK, even when it can't find a fortran compiler. In this case, it will build with BLAS support only, and defer this problem downstream to the consumer. Armadillo will also build successfully, however it will have linked against the LAPACK symbols, and so any consumer usage of the resulting library will result in unresolved symbol errors. I've raised #7294 to address this issue and trigger a failure if a fortran compiler is not found.

I'm looking for some feedback on this recipe, specifically:

• Are we happy to accept the ability to toggle on usage of system libraries?
  • My thoughts are that this will be good in the mid-term until the above libraries can be packaged in conan. The only system requirement imposed currently is fortran, which is really a requirement of the upstream OpenBLAS recipe so it should be possible for users to use this recipe in its default configuration if they have a fortran compiler. If they don't, BLAS or LAPACK functionality can be turned off to provide whatever functionality is required, or augmented with system libraries as required.
• I've got a number of replace_in_file calls in the recipe - are these better placed as patches?
• Is there a better way of managing co-dependent/mutually exclusive options than what I've done here?
• Are there any other improvements that can be made?

I'm not the author of this library.

Closes #6754

• I've read the guidelines for contributing.
• I've followed the PEP8 style guides for Python code in the recipes.
• I've used the latest Conan client version.
• I've tried at least one configuration locally with the
  conan-center hook activated.

[samuel-emrys]

Does the CI not attempt to build upstream dependencies? Also, as per the description, the success of these CI runs will depend on fortran compiler availability in the CI. This statement leads me to believe that this should be available?

[jgsogo]

I'm a bit concerned about the options and their logic. There is no need to map all the options in CMakeLists.txt to Conan options, sometimes a different approach is more convenient.

Some comments about them:

• we try to use lowercase names for options

• sometimes mutually exclusive options can be turned into choice ones:

  "use_blas": [False, "openblas", "system_openblas", "mkl", "flexiblas", "system_blas", "framework_accelerate"]
  
  [...]
  
  self._cmake.definitions["USE_OPENBLAS"] = self.options.blas
  self._cmake.definitions["USE_SYSTEM_BLAS"] = (self.options.blas == "system_blas")
  self._cmake.definitions["USE_OPENBLAS"] = (self.options.blas == "openblas")
  ...

[samuel-emrys]

I'm a bit concerned about the options and their logic. There is no need to map all the options in CMakeLists.txt to Conan options, sometimes a different approach is more convenient.

Some comments about them:

* we try to use lowercase names for options

* sometimes mutually exclusive options can be turned into _choice_ ones:
  ```python
  "use_blas": [False, "openblas", "system_openblas", "mkl", "flexiblas", "system_blas", "framework_accelerate"]
  
  [...]
  
  self._cmake.definitions["USE_OPENBLAS"] = self.options.blas
  self._cmake.definitions["USE_SYSTEM_BLAS"] = (self.options.blas == "system_blas")
  self._cmake.definitions["USE_OPENBLAS"] = (self.options.blas == "openblas")
  ...
  ```

I tried to mimic the configuration options that match the documentation as closely as possible to make it easy for users to rely on the documentation as much as possible and less inference about what the recipe requires.
Having said that, I can see the utility in presenting mutually exclusive options this way - much simpler and cleaner. I didn't realise the options could be specified this way, though it makes sense looking back at it now. I'll try to refactor using this principle where i can

[samuel-emrys]

The CI tests here are being affected by the fact that the documentation for apple-clang indicates the default C++ language standard is gnu++14, however the language standard is actually gnu++98 by default. I'm not sure which is what it should be, but Armadillo requires C++11 or higher to compile, so on Macos the tests fail (pass?) with invalid configuration errors.

[samuel-emrys]

@samuel-emrys , please, have a look at this PR #7477. We think it can be a good way to handle these close source packages in the future. Of course, Conan will fail if the user tries to activate those options, but there should be a comment clarifying that they should provide the reference themselves. wdty?

Thanks!

@jgsogo Is this something that I should incorporate in to this package now?

[conan-center-bot]

All green in build 19 (76c01b99d8187b102a745b019740d5925bb50424):

• armadillo/10.7.0@:
  All packages built successfully! (All logs)

[samuel-emrys]

Is there anything I can do to move this forward?

[uilianries]

LGTM

Thank you for hard working on this.

[jgsogo]

Agree, we can try to merge and iterate later.

[darcamo]

I have created my own armadillo recipe, along with others recipes that I need that are/were not in conan center. I decided to finally investigate how I could include it in conan center when I found this recipe already in a very good shape. This recipe seems well written and I can imagine how much work was put into this. I have just a few pointers that you might consider looking at before merging it.

• Enabling the option use_wrapper results in link errors when consuming the library. I also have this problem in my own recipe and I didn't find a solution for this. It is worth testing if you have the same problem, in which case it might be better to suppress this option until/if if can be made to work correctly (IMHO).

• With use_wrapper option set to False, I don't think the use_extern_rng option is having any effect. The reason is that when use_extern_rng is set to True it will set a define in CMake. However, that is only used when you compile the armadillo wrapper library. When the wrapper library is not created, armadillo is used as a header only library and you need to define ARMA_USE_EXTERN_RNG to use the external random generator from C++11. The way I implemented this in my own recipe was adding the code below in the package_info method

  if self.options.use_extern_rng:
      self.cpp_info.defines.append("ARMA_USE_EXTERN_RNG")

  To test that the use_extern_rng currently has no effect (with use_wrapper set to False) you can run the program below with use_extern_rng set to False and later with it set to True. You will get exactly the same values in both cases indicating that the option is not really used.

  #include <armadillo>
  #include <iostream>
  
  int main(int argc, char *argv[])
  {
      arma::vec v{1,2,3,4};
      v.print("v");
  
      arma::arma_rng::set_seed(1237);
      v.randn();
      v.print("v");
  
      return 0;
  }

  Now try adding #define ARMA_USE_EXTERN_RNG before including armadillo and you will get different numbers in the vector.

• Armadillo's current stable version is 10.7.1

[uilianries]

@darcamo Thank you for your review! Indeed Armadillo requires a lot of effort to be package. Any PR will be very welcome, you can open a PR directly to Samuel's repository, or wait for this PR be merged (which is very close) and open a PR to Conan Center Index instead.

[prince-chrismc]

Let's improve it in the next PR

[samuel-emrys]

* Enabling the option `use_wrapper` results in link errors when consuming the library. I also have this problem in my own recipe and I didn't find a solution for this. It is worth testing if you have the same problem, in which case it might be better to suppress this option until/if if can be made to work correctly (IMHO).

I've just done some testing and have confirmed that it does fail in some cases, but not all. Specifically, it fails when built with lapack. I remember finding a resolution to this in the past - but the solution is escaping me right now. From memory part of the issue was building armadillo as a static library with dependencies on shared libraries upstream, especially in the context of using the system lapack.

Regardless, I've raised #7840 to discuss and track this issue further.

[darcamo]

I have created a pull-request (#7919) to add the missing define.