Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify CMake for Armadillo and its Dependencies #2247

Merged
merged 16 commits into from Mar 11, 2020
6 changes: 3 additions & 3 deletions .ci/windows-steps.yaml
Expand Up @@ -28,7 +28,7 @@ steps:
# Configure armadillo
- bash: |
git clone --depth 1 https://github.com/mlpack/jenkins-conf.git conf

curl -O http://masterblaster.mlpack.org:5005/armadillo-8.400.0.tar.gz -o armadillo-8.400.0.tar.gz
tar -xzvf armadillo-8.400.0.tar.gz

Expand Down Expand Up @@ -60,8 +60,8 @@ steps:

cmake $(CMakeGenerator) `
$(CMakeArgs) `
-DBLAS_LIBRARY:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `
-DLAPACK_LIBRARY:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `
-DBLAS_LIBRARIES:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably avoid having to change this flag by adding a case to map X_LIBRARY to X_LIBRARIES for the impacted libraries. Does this seem like a good idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason for using such an ancient version (0.2.14) of OpenBLAS ?
OpenBLAS has had releases with many bug fixes since then:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but the pipeline is currently using nuget, and the latest version there seems to be 0.2.14.1
Though there seem to be other openblas nuget projects, so we could try switching to those to get all the way to 0.2.19.3.
I think it makes more sense to consider another windows package manager such as vcpkg, as that openblas seems to be up to date. Though that's probably a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can certainly trust the Windows "world" to be a royal mess in many aspects. Instead of jumping through these silly hoops, why not just simply ship a pre-compiled copy of OpenBLAS along with mlpack?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think actually that's exactly what we're doing---we're just grabbing the version from nuget. I have no issue switching versions or anything like that... if you want to try that feel free, either in this PR or another or wherever. But yeah... the Windows distribution world is a total nightmare...

-DLAPACK_LIBRARIES:FILEPATH=$(Agent.ToolsDirectory)\OpenBLAS.0.2.14.1\lib\native\lib\x64\libopenblas.dll.a `
-DARMADILLO_INCLUDE_DIR="..\armadillo-8.400.0\include" `
-DARMADILLO_LIBRARY="..\armadillo-8.400.0\Release\armadillo.lib" `
-DBOOST_INCLUDEDIR=$(Agent.ToolsDirectory)\boost.1.60.0.0\lib\native\include `
Expand Down
37 changes: 0 additions & 37 deletions CMake/ARMA_FindACML.cmake

This file was deleted.

37 changes: 0 additions & 37 deletions CMake/ARMA_FindACMLMP.cmake

This file was deleted.

39 changes: 0 additions & 39 deletions CMake/ARMA_FindARPACK.cmake

This file was deleted.

44 changes: 0 additions & 44 deletions CMake/ARMA_FindBLAS.cmake

This file was deleted.

47 changes: 0 additions & 47 deletions CMake/ARMA_FindCBLAS.cmake

This file was deleted.

48 changes: 0 additions & 48 deletions CMake/ARMA_FindCLAPACK.cmake

This file was deleted.

44 changes: 0 additions & 44 deletions CMake/ARMA_FindLAPACK.cmake

This file was deleted.

49 changes: 0 additions & 49 deletions CMake/ARMA_FindMKL.cmake

This file was deleted.