DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.11k stars 256 forks source link

Conflict between SuiteSparse and CMake's FindBLAS use of BLA_VENDOR CMAke variable #800

Open traversaro opened 1 month ago

traversaro commented 1 month ago

First of all, thanks a lot for all the hard work on suitesparse!

Describe the bug

Both CMake's FindBLAS and SuiteSparse's CMake use the BLA_VENDOR variable. However, they use it with slightly different semantics, that can lead to unexpected behaviour.

CMake's FindBLAS describe BLA_VENDOR (https://cmake.org/cmake/help/latest/module/FindBLAS.html) as:

Set to one of the BLAS/LAPACK Vendors to search for BLAS only from the specified vendor. If not set , all vendors are considered.

So, for CMake the value of BLA_VENDOR that means "find any BLAS available" is if BLA_VENDOR is unset (or is an empty string or a bunch of other conditions, see https://cmake.org/cmake/help/latest/command/if.html#constant) or if set to All, see https://github.com/Kitware/CMake/blob/2e5b40f581c9c58a92d6f166e6ca28e8a3cc3563/Modules/FindBLAS.cmake#L398, but this is just an undocumented implementation detail.

Instead, for SuiteSparse BLA_VENDOR is (https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/31bdd56256b3d2a7dffaade1fbd074d48489865c/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake#L24):

if ANY (default): searches for any BLAS. Otherwise: search for a specific BLAS" )

And the ANY default value is enforced by default by SuiteSparse.

This means that if one tries to build by default SuiteSparse, the BLA_VENDOR default value of ANY can be interpreted by CMake as a specific BLAS implementation, and find_package(BLAS) may fail if it is not found.

To Reproduce

If you are familiar with conda, on Windows this fails

conda create -n suitesparsedev -c conda-forge cmake pkg-config compilers make ninja blas-devel libblas=*=*netlib* libcblas liblapack metis tbb-devel
git clone https://github.com/DrTimothyAldenDavis/SuiteSparse
mkdir build
cd build
cmake .. -GNinja   -DSUITESPARSE_ENABLE_PROJECTS="suitesparse_config;amd;btf;camd;ccolamd;colamd;cholmod;cxsparse;ldl;klu;umfpack;paru;rbio;spqr" -DBUILD_SHARED_LIBS=ON -DBUILD_STATIC_LIBS=OFF -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=%CONDA_PREFIX%\Library -DCMAKE_PREFIX_PATH=%CONDA_PREFIX%\Library

Expected behavior

Without specifing any BLA_VENDOR value, the default value of BLA_VENDOR should be the one that means "find any blas" also for CMake, i.e. empty string.

Screenshots

N.a.

Desktop (please complete the following information):

Additional context

Problem emerged in https://github.com/conda-forge/suitesparse-feedstock/pull/92 .

traversaro commented 1 month ago

Note that a workaround of setting BLA_VENDOR to All on Windows is used even in SuiteSparse's CI, see https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/31bdd56256b3d2a7dffaade1fbd074d48489865c/.github/workflows/root-cmakelists.yaml#L529 .

mmuetzel commented 1 month ago

Thank you for your report.

I opened #802. Does that change fix the issue for you?

traversaro commented 1 month ago

I opened #802. Does that change fix the issue for you?

Thanks! I just checked, and it seems not to fix the issue. I added a print after the unset:

diff --git a/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake b/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake
index 69ab1e941..13ae7d09d 100644
--- a/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake
+++ b/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake
@@ -154,6 +154,7 @@ if ( SUITESPARSE_USE_64BIT_BLAS )

     # Look for any 64-bit BLAS
     unset ( BLA_VENDOR CACHE )
+    message(STATUS "========> BLA_VENDOR: ${BLA_VENDOR}")
     message ( STATUS "Looking for any 64-bit BLAS" )
     set ( BLA_SIZEOF_INTEGER 8 )
     find_package ( BLAS )
@@ -239,6 +240,7 @@ endif ( )

 # Look for any 32-bit BLAS (this is required)
 unset ( BLA_VENDOR CACHE )
+message(STATUS "========> BLA_VENDOR: ${BLA_VENDOR}")
 message ( STATUS "Looking for any 32-bit BLAS" )
 set ( BLA_SIZEOF_INTEGER 4 )
 find_package ( BLAS REQUIRED )

and the output is:

-- Could NOT find BLAS (missing: BLAS_LIBRARIES)
-- ========> BLA_VENDOR: FLAME
-- Looking for any 32-bit BLAS
CMake Error at D:/miniforge3/envs/suitesparsedev/Library/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find BLAS (missing: BLAS_LIBRARIES)
Call Stack (most recent call first):
  D:/miniforge3/envs/suitesparsedev/Library/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  D:/miniforge3/envs/suitesparsedev/Library/share/cmake-3.29/Modules/FindBLAS.cmake:1387 (find_package_handle_standard_args)
  SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake:246 (find_package)
  SuiteSparse_config/CMakeLists.txt:123 (include)

BLA_VENDOR seems to have the value set by the last normal (non-cache) set. Not sure how this can be solved, in my experience it is quite error-prone to deal with with CACHE and normal variables with the same name. Perhaps we need to unset both the normal and the cache variable? Note that however this means that a user needs to pass -DBLA_VENDOR to each invocation of CMake, that may be unexpected as CMake options are tipically cached.