[cmake] Disable SOFIE if Blas is not found #16722
Conversation
| if(GSL_FOUND) | ||
| message(STATUS "Using GSL CBLAS for TMVA SOFIE") | ||
| else() | ||
| set(tmva-sofie OFF CACHE BOOL "Disabled because BLAS was not found (${tmva-cpu_description})" FORCE) |
There was a problem hiding this comment.
This breaks the fail-on-missing=ON contract. You would need a separate code path for that like in the other cases where we automatically switch off ROOT features.
There was a problem hiding this comment.
Well, it was already a problem is the code for the tmva-cpu option: https://github.com/root-project/root/blob/master/cmake/modules/SearchInstalledSoftware.cmake#L1665
Either this is fixed in both cases, or you keep it as it is now for consistency. But I really like the fail-on-missing flag so I argue for the former :)
There was a problem hiding this comment.
I think we have alread the tmva-cpu flag checking for BLAS. We need to be sure tmva-sofie is dependent on that flag, I think there is no need to another BLAS check
There was a problem hiding this comment.
Actually we can still build SOFIE without BLAS, so no need to switch OFF tmva-sofie. We need only to protect tutorials with tmva-cpu. I can make a PR for this
There was a problem hiding this comment.
Are we really okay with users that write code that is something 'like' the tutorials to "only" see an error/failure that is 'hard' to understand? I.e. the current symptom is "missing symbol _sgemm" rather than "Please rebuild ROOT once you configured it to find a BLAS library". Can we upgrade the code to have a better error?
There was a problem hiding this comment.
"Missing symbol _sgemm" is not so hard. It is a well known issue, and asking chatGPT or Google you get 100% the right answer. And you don't need to rebuild ROOT, but just install BLAS to have the tutorial working.
Test Results 18 files 18 suites 3d 20h 37m 27s ⏱️ Results for commit 38b0d88. |
|
This improves the situations. The first 3 tests are now properly disabled. This leaves this test: as an issue (and the related but maybe or maybe not different #16719) |
fixes #16720