[CMake] Correctly handle tmva-cpu=ON use_gsl_cblas=OFF case#22044
[CMake] Correctly handle tmva-cpu=ON use_gsl_cblas=OFF case#22044guitargeek wants to merge 1 commit intoroot-project:masterfrom
tmva-cpu=ON use_gsl_cblas=OFF case#22044Conversation
| message(SEND_ERROR "Option tmva-cpu requires a BLAS library, but none could be found on the system. Either install a BLAS library like OpenBLAS (preferred), or set use_gsl_cblas=ON (possibly also builtin_gsl=ON if GSL not installed on the system).") | ||
| else() | ||
| set(use_gsl_cblas ON CACHE BOOL "Using GSL CBLAS for TMVA as find_package(BLAS) didn't find anything" FORCE) | ||
| endif() |
There was a problem hiding this comment.
| endif() | |
| if(NOT builtin_gsl) | |
| find_package(GSL) | |
| if (NOT GSL_FOUND) | |
| set(builtin_gsl ON CACHE BOOL "Using builtin GSL since CBLAS for TMVA required us to use GSL-CBLAS and no system GSL was found" FORCE) | |
| message(STATUS "Enabling builtin_GSL for TMVA-cpu gsl-CBLAS [GPL]") | |
| endif() | |
| endif() |
shouldn't this be also auto-enabled in cascade? Otherwise we might fail later on
There was a problem hiding this comment.
True, that's right. I need to go back to the drawing board and see how to handle builtin_gsl.
There was a problem hiding this comment.
My personal opinion:
I'd wish fail-on-missing wouldn't exist and would be always mandatorily ON. I'd would save a ton of headaches and issues xD
it's faster to type -DbuiltinXX=ON by the user than to handle all these casuistic by the devs.
and avoid weird things such as https://its.cern.ch/jira/browse/ROOT-10743
Auto-enabling is dangerous and developer-time-consuming. Most users download binaries, people who build can afford typing a bit what they want ;) So my vote for removing fail-on-missing and simplifying this overly complex builtin mechanism nightmare.
There was a problem hiding this comment.
Yes I vote for this for years 😆 But I forgot who was actually against it at this point. I'll bring it up again in the meetings (tagging also @hageboeck, the ROOT 7 planner).
If the BLAS library is not found at configuration time, we should try to fallback to the GSL CBLAS library, and if it still can't be found, error out in the configuration. The branch that handles the error was already there, but the GSL fallback was not implemented correctly. The GSL fallback also needs to consider `builtin_gsl` correctly, and to achieve this, finding GSL is not done after checking the required BLAS library for TMVA CPU. Closes root-project#22020.
ferdymercury
left a comment
ferdymercury
left a comment
There was a problem hiding this comment.
thanks! just some message fixes
| message(STATUS " For the time being switching OFF 'mathmore' option") | ||
| if (mathmore) |
There was a problem hiding this comment.
| message(STATUS " For the time being switching OFF 'mathmore' option") | |
| if (mathmore) | |
| if (mathmore) | |
| message(STATUS " For the time being switching OFF 'mathmore' option") |
| set(mathmore OFF CACHE BOOL "Disable because builtin_gsl disabled and external GSL not found (${mathmore_description})" FORCE) | ||
| endif() | ||
| if (tmva-cpu AND use_gsl_cblas) | ||
| set(tmva-cpu OFF CACHE BOOL "Disable because use_gsl_cblas enabled, builtin_gsl disabled and external GSL not found (${tmva-cpu_description})" FORCE) |
There was a problem hiding this comment.
| set(tmva-cpu OFF CACHE BOOL "Disable because use_gsl_cblas enabled, builtin_gsl disabled and external GSL not found (${tmva-cpu_description})" FORCE) | |
| message(STATUS " For the time being switching OFF 'tmva-cpu' option") | |
| set(tmva-cpu OFF CACHE BOOL "Disable because use_gsl_cblas enabled, builtin_gsl disabled and external GSL not found (${tmva-cpu_description})" FORCE) |
Test Results 22 files 22 suites 3d 10h 11m 13s ⏱️ Results for commit ee12911. |
2 participants
If the BLAS library is not found at configuration time, we should try to fallback to the GSL CBLAS library, and if it still can't be found, error out in the configuration. The branch that handles the error was already there, but the GSL fallback was not implemented correctly.
Closes #22020.