Use common/simpler FindArmadilo.cmake (and BLAS/LAPACK)

[birm]

This is related to #2113, at least in the sense that it would help simplify CMakeLists.txt

Armadillo, BLAS, and LAPACK have nice and simple preexisting cross-platform FindX.cmake files in the cmake repo (and thus shipped with most newer versions of Cmake) which would help remove some special configuration done.

Note that this would also involve slight modifications to the test pipeline related changes made in mlpack/models#52 so that it would no longer have to patch CMakeLists.txt

[rcurtin]

I'm not sure if FindArmadillo.cmake is shipped with all versions of CMake we support, but in any case, we have a version in CMake/.

The specific lines of code in CMakeLists.txt that we'd like to remove are lines 300 to 338. We'll need to ensure that the refactored/cleaned CMakeLists.txt works correctly on Windows, and possibly also update tutorials and other documentation.

[codeboy5]

Hi, can I work on this issue?? I don't have a lot of experience with cmake, so if you can guide me with how to get started it would be great.

[PranavReddyP16]

Some guidance would be really helpful as even I don't have any experience with cmake and would like to learn

[birm]

To add some context, cmake >=3.6.0, find_package finds lapack, blas, and armadillo without requiring any special setup (in most circumstances at least).
So, there are different ways to handle this depending on how much you want to do now.

First, you could copy the findLapack and findBlas cmake files distributed with newer versions of cmake here, and apply something resembling this patch to cmake lists. I'd recommend testing different ways of acquiring blas and lapack on windows while testing this, since the patch was only concerned with vcpkg.

If you want to look deeper, mlpack's FindArmadillo.cmake has a lot of custom pseudo modules for finding blas/lapack/mkl etc. If we used cmake's own findArmadillo along with the findLapck and findBlas as above, it would greatly simplify the mlpack cmake configuration.

In short, either case would need to be accompanied by a fair amount of testing. I'm not even sure if all of the above linked findX.cmake files are compatible with cmake 3.3.2 as mlpack currently uses.

[rcurtin]

I'd strongly suggest that anyone wanting to work on this issue be familiar with CMake: CMake is our build system and so changes to it affect every single person who builds the library. So I am pretty paranoid about the CMake setup and I think we all should be. :)

@birm I think that we can use only mlpack's CMake/FindArmadillo.cmake for this purpose, to remove the code from line 300 to 338. The FindArmadillo.cmake distributed with CMake unfortunately doesn't find any deeper dependencies of Armadillo that would be needed if ARMA_USE_WRAPPER is not set in the Armadillo configuration. I suspect that linking on Windows can just use the ARMADILLO_LIBRARIES set by CMake/FindArmadillo.cmake, instead of the specific code on lines 300 to 338 that try to manually find BLAS and LAPACK, but it would need some testing.

Let me know if you think otherwise; I'm open to other ideas for sure, I just wrote down what I was thinking. :)

[birm]

This, is based mostly on my thoughts rather than any testing at this point. With that disclaimer out of the way...

If I'm reading mlpack:FindArmadillo right, it tries to find a blas compatible and a lapack compatible library on the system, and it puts them all in a variable ARMADILLO_LIBRARIES. However, since we're not building armadillo, it's not armadillo that needs to link to lapack and blas, but mlpack. Thus, I think in the case where ARMA_USE_WRAPPER is defined just using find_package on blas lapack and then armadillo.

In the case where we aren't wrapping armadillo, then I think the only difference would be that we don't have a library to link to. We probably will want to incorporate mlpack:FindArmadillo.cmake's system for determining how to set ARMA_USE_WRAPPER, since it looks like CMake:FindArmadillo.cmake doesn't account for wrapping at all. Or we could ask the user to just tell us.

I'll test this theory next week (or sooner if time somehow allows) if someone doesn't beat me to it.

[rcurtin]

Sorry for the slow response. What you wrote is pretty close to the intention of the CMake scripts but let me add a few extra details---

You're exactly right that FindArmadillo.cmake tries to find LAPACK and BLAS libraries and adds them to ARMADILLO_LIBRARIES... but it should only be doing this when ARMA_USE_WRAPPER is set. Basically, if ARMA_USE_WRAPPER is set, then there will be a file libarmadillo.so (or libarmadillo.dylib on OS X or armadillo.dll on Windows) that is literally just a wrapper for LAPACK and BLAS.

In essence, this wrapper means you can compile a program that uses Armadillo by linking with only -larmadillo and not -llapack -lblas. This is because Armadillo is set up in such a way that if ARMA_USE_WRAPPER is set, the Armadillo code will only call symbols present in libarmadillo.so, meaning that's the only dependency that needs to be linked. If ARMA_USE_WRAPPER is not set, then the Armadillo code will call LAPACK and BLAS symbols directly, meaning that the code must link against those two libraries instead.

On Windows, it seems that linking only to Armadillo does not work; I think this is because the nature of linking is different on Windows. Therefore, our existing CMake scripts require the locations of BLAS_LIBRARY and LAPACK_LIBRARY in order to link directly against them.

So where I am going with all of this is that I think we can achieve this same behavior through FindArmadillo.cmake. Perhaps it might need just a little modification to specify that ARMADILLO_LIBRARIES should be filled with all the dependencies even if ARMA_USE_WRAPPER is set.

I typed all that on a phone... my thumbs hurt now 😵

[knakul853]

Any PR relate to this. After mlpack/models#49
If not would love to work on.: )

[PranavReddyP16]

I don't think there's anything as of yet and I'm a bit busy so go ahead with it I guess. Just confirm with @codeboy5 to make sure he isn't working/worked on it already

[codeboy5]

I don't think there's anything as of yet and I'm a bit busy so go ahead with it I guess. Just confirm with @codeboy5 to make sure he isn't working/worked on it already

No I am not working on this issue, feel free to go ahead !!!

[knakul853]

@codeboy5 @PranavReddyP16 thanks for confirmation