search

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

SuiteSparseBLAS.cmake: Unset cache variable BLA_VENDOR #802

Closed mmuetzel closed 1 month ago

mmuetzel commented 1 month ago

BLA_VENDOR is a cache variable in SuiteSparseBLAS.cmake (not a "normal" variable). Use the correct syntax to unset it when necessary.

Fixes #800.

Also remove a configure variable that is now redundant from the CI.

mmuetzel commented 1 month ago

Next attempt.

@traversaro: I agree that having cache variables and normal variables of the same name is pretty confusing. Does removing the cache variable resolve the issue for you?

IIUC, BLA_VENDOR="ANY" didn't work in a while (potentially never?). I'm hoping that removing ANY allows to somewhat simplify the logic in SuiteSparseBLAS.cmake. Reading the documentation for FindBLAS.cmake, the only documented way to select any BLAS implementation is to not set BLA_VENDOR to a specific identifier. Maybe, SuiteSparse should just use the same definition. The current revision is an attempt at doing that.

traversaro commented 1 month ago

Thanks for the fast iteration. The new version is indeed fixing https://github.com/DrTimothyAldenDavis/SuiteSparse/issues/800 .

DrTimothyAldenDavis commented 1 month ago

Thanks for reporting the bug, @traversaro, and thanks for the fast fix, @mmuetzel !

traversaro commented 1 month ago

Thanks a lot for the fast feedback! Just FYI it seems some docs are now outdated:

mmuetzel commented 1 month ago

Good point. I opened #803 with the documentation change.

DrTimothyAldenDavis commented 1 month ago

This change breaks the macos CI. See https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/9201576210/job/25310008402

That CI is only run manually because it can hang intermittently (for reasons outside of my control, if I recall). I ran it manually and the changes to SuiteSparse_config don't work on the Mac. The BLAS is not found.

DrTimothyAldenDavis commented 1 month ago

This change seems to fix it though: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/45a009924d7cc19b01856ad4729a9927f8678177

The CI on the mac is still in progress.

mmuetzel commented 1 month ago

I've opened #807 to address the build issues on the macOS runners.

I don't recall why we weren't using Apple BLAS before. But it would probably be possible to revert 45a009924d7cc19b01856ad4729a9927f8678177 after that PR is applied.