Improved CMake variables / behavior

[haampie]

Hi, I'm trying out the new CMake build system after struggling to keep up with all breaking changes in the Makefile-based build system.

Few comments.

1. Could you please drop double negatives, it's only confusing. Do not not use openmp?

   option ( NOPENMP "ON: do not use OpenMP.  OFF (default): use OpenMP" OFF )

2. Please prefix user facing variables: for example SUITESPARSE_WITH_OPENMP, SUITESPARSE_WITH_CUDA, etc.

3. I don't understand why there's so much special handling of rpaths in suite sparse? What problem is being solved that is not already solved by CMake? (e.g. CMAKE_INSTALL_RPATH_USE_LINK_PATH=ON) Is there anything unique about suitesparse that requires this? If not, could you please remove special handling and leave it to packagers?

4. Please use find_package(... REQUIRED) instead of silently turning a feature off.

   if ( NOPENMP )
       # OpenMP has been disabled
       message ( STATUS "OpenMP disabled" )
       set ( OpenMP_C_FOUND OFF )
   else ( )
       find_package ( OpenMP )  # <--- find_package(OpenMP REQUIRED) would be better
   endif ( )

   Same for CUDA: if enabled by the user, require it, instead of

   SuiteSparse/LAGraph/cmake_modules/SuiteSparsePolicy.cmake

   Lines 301 to 304 in a02c53b

   	if ( CUDAToolkit_VERSION VERSION_LESS "11.2" )
   	# CUDA is present but too old
   	message ( STATUS "CUDA: not enabled (CUDA 11.2 or later required)" )
   	set ( SUITESPARSE_CUDA OFF )

5. Please be clear about the minimum CMake version. The top-level CMakeLists.txt requires 3.20 but says higher may be required. What's the minimum then? I don't want to go over each and every subproject. Just set the top-level min to max(min(subproject) for subproject in subprojects)

   Further, there are a lot of outdated and redundant cmake policy things, can they be removed? They're all NEW by default.

   SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparsePolicy.cmake

   Lines 98 to 103 in a02c53b

   	cmake_policy ( SET CMP0042 NEW ) # enable MACOSX_RPATH by default
   	cmake_policy ( SET CMP0048 NEW ) # VERSION variable policy
   	cmake_policy ( SET CMP0054 NEW ) # if ( expression ) handling policy
   	if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.18.0" )
   	cmake_policy ( SET CMP0104 NEW ) # initialize CUDA architectures
   	endif ( )

Would be great if this is addressed before the official release, so that there won't be a series of breaking changes to the CMake build system too. I'm packaging suitesparse for spack where we wanna be able to install older versions as well as newer versions, and we're probably skipping the 5 < version < 7.4 versions because of breaking changes. But see also here, people are hesitant to package things if the build system changes every release: JuliaPackaging/Yggdrasil#7803 (comment).

[mmuetzel]

Thank you for the feedback.

Re 1: Good point. IIUC, those variable names are remnants from the transition from the Makefile based build system to the CMake build system.

Re 2: Good point. (See Re 1.)

Re 3: PETSc packagers said SuiteSparse behaved different to their other dependencies before this change. I'm open to reverting that change. But maybe that should be coordinated. See: https://gitlab.com/petsc/petsc/-/merge_requests/7104#note_1693474274

Re 4: That could probably be done after the negative flag NOPENMP is replaced by a "positive" flag. At the moment, there is no flag that "forces" OpenMP to be used. So, failing if it isn't used, would be strange imho.
Edit: That change should be done carefully. IIUC, it's generally preferred to use OpenMP if available. But some subprojects require a higher minimum version than others. If such a flag would be enforced, that would mean that the highest required OpenMP version would be required for all subprojects even if a lower version would suffice for some.
Same for CUDA.

Re 5: The minimum CMake version depends on the set of subprojects that you'd like to build. It could probably be raised in the root CMake file. But iiuc, that would mean that some (older) versions of CMake could no longer be used to build subprojects for which they would be "new enough".

[mmuetzel]

I opened #600 that could be used to transition away from NOPENMP. (There are a couple more N* flags. But I guess NOPENMP is the most prominent one.)

I'm a bit hesitant to go with your suggestions 4 or 5 for the reasons described above.

[mmuetzel]

Re your suggestion 3, the PETSc packagers will set that CMake flag in their project. I opened #601 that would revert the change that sets the RPATH for installed libraries. (The INSIDE_SUITESPARSE code path should not be triggered when packaging SuiteSparse.)

[haampie]

Thanks @mmuetzel, this all looks great :)

Regarding 4) I would say it's good practice. For example if someone upgrades or changes their compiler toolchain but forgets to install the relevant openmp package, it's more convenient to get a configure error than working binaries for something they didn't ask for. Similarly when you have some automated way of building the package in a fixed configuration (say SUITESPARSE_WITH_CUDA=ON) on different machines, it's more convenient to get a configure error when e.g. cuda is missing than working binaries without cuda support.

[mmuetzel]

Regarding 4) I would say it's good practice. For example if someone upgrades or changes their compiler toolchain but forgets to install the relevant openmp package, it's more convenient to get a configure error than working binaries for something they didn't ask for. Similarly when you have some automated way of building the package in a fixed configuration (say SUITESPARSE_WITH_CUDA=ON) on different machines, it's more convenient to get a configure error when e.g. cuda is missing than working binaries without cuda support.

Which idiom would you suggest to check whether a variable was set by a user or whether its default value is used?
How should differing minimum version requirements of different subprojects be handled?

[haampie]

Which idiom would you suggest to check whether a variable was set by a user or whether its default value is used?

Hm, you can't really with ON/OFF. LLVM uses a tri-state ON / OFF / FORCE_ON, but dunno if that's very common:

if(LLVM_ENABLE_TERMINFO STREQUAL FORCE_ON)
  set(MAYBE_REQUIRED REQUIRED)
else()
  set(MAYBE_REQUIRED)
endif()
if(LLVM_ENABLE_TERMINFO)
  find_library(COMPILER_RT_TERMINFO_LIB NAMES terminfo tinfo curses ncurses ncursesw ${MAYBE_REQUIRED})
endif()

How should differing minimum version requirements of different subprojects be handled?

Don't know what if that can be made dynamic, you really have to specify cmake_minimum_required(...) before doing anything that it may have an affect on. I don't think it's too bad to require the latest CMake, since it's so easy to download / build a newer CMake.

[mmuetzel]

Don't know what if that can be made dynamic, you really have to specify cmake_minimum_required(...) before doing anything that it may have an affect on. I don't think it's too bad to require the latest CMake, since it's so easy to download / build a newer CMake.

I meant different minimum version requirements for CUDA or OpenMP.

[DrTimothyAldenDavis]

The double negative derives from things like the NDEBUG in the C/C++ preprocessor, where if not defined, NDEBUG is false so the default is no debugging.

For CUDA, I really need to have the options of (a) do not use CUDA even if the GPU is available, and (b) use CUDA if available but build the packages without it if not. I'm OK with adding a third option: (c) use CUDA and fail if not available.

Option (b) needs to be the default for CHOLMOD and SPQR, and eventually GraphBLAS when the CUDA kernels are ready for production use (they are not yet ready and cannot be used at all in production).

The same is true for OpenMP: the packages should use OpenMP by default, if available, but they can still be built without it.

Other user-facing variables have been in place a long time, like NCAMD, NPARTITION, and so on, well before any cmake bulid system. They act like the C NDEBUG and date back many years. I'd like to keep those the same for continuity.

Regarding things like cmake_policy ( SET CMP0104 NEW ): yes, as long as the cmake version is new enough, we can remove those.

For the cmake_minimum_required ( VERSION 3.22 ), etc: it's complicated because SuiteSparse is a meta-pacakge.
Some of its packages like GraphBLAS, LAGraph, and CSparse can be built outsideof SuiteSparse, without SuiteSparse_config, but GraphBLAS and LAGraph have copies of the SuiteSparsePolicy.cmake. The SuiteSparse_config package needs 3.22 so it can find the BLAS/LAPACK libraries, for UMFPACK, CHOLMOD, and SPQR. Most packages are fine with 3.20 otherwise. There are some old cmake scripts inside packages like METIS, cpu_features, etc, but I don't use those at all. I've had some people report that they have trouble building GraphBLAS, or all of SuiteSparse, because the cmake minimum is so high.

I require cmake 3.16 for the GraphBLAS JIT, which is need by the end user of GraphBLAS to compile their JIT kernels, even if they get a GraphBLAS.h and libgraphblas.so already built and installed in a linux distro / spack / brew / Windows / etc. That should stay the same.

So this is why things like this are needed:

 if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.18.0" ) 
     cmake_policy ( SET CMP0104 NEW )    # initialize CUDA architectures 
 endif ( ) 

I'd like to keep that one, but I can remove the older ones since I see those have been in CMake a long time now.

[DrTimothyAldenDavis]

See 3ed3bd7

for the removal of CMP00* policies.

[haampie]

where if not defined, NDEBUG is false so the default is no debugging.

Are you sure? This drives the point home ;)

(a) do not use CUDA even if the GPU is available, and (b) use CUDA if available but build the packages without it if not. I'm OK with adding a third option: (c) use CUDA and fail if not available.

How about a tri-state AUTO (default) / ON / OFF then. Explicit -DSUITESPARSE_USE_CUDA=ON is strict and users don't have to deal with custom values like LLVM does in the example above. If you tell suite sparse explicitly to enable a certain feature it errors if it can't. This is also in line with typical autotools --with-x / --without-x (if not specified it sometimes autodetects).

Regarding

 if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.18.0" ) 
     cmake_policy ( SET CMP0104 NEW )    # initialize CUDA architectures 
 endif ( ) 

you could use cmake_minimum_required(VERSION <min>...<policy_max>) which uses new policies while allowing old cmake too.

[DrTimothyAldenDavis]

Oh ... right. Ack. I'm a bit dyslexic.

I will simplify the cmake_minimum_required statements. Most can change to 3.22 since they depend on SuiteSparse_config, which requires 3.22.

GraphBLAS, LAGraph, and CSparse can be built as stand-alone packages, so I will leave their cmake_minimum_required the same.

See:
6efda2c

[DrTimothyAldenDavis]

OK ... I've decided to ditch NCAMD, etc, and go with CHOLMOD_CAMD, etc, instead.

This will also prefix these user-facing variables to "(package)_WHATEVER" instead of just "WHATEVER".

[haampie]

Thanks, that should also make name clashes less likely (although already unlikely given those names...) and CMakeCache.txt files orderly when suitesparse itself is used as a sub-project elsewhere.

[mmuetzel]

BUILD_SHARED_LIBS and BUILD_STATIC_LIBS are default CMake flags that should not be prefixed.

[mmuetzel]

How about a tri-state AUTO (default) / ON / OFF then.

CMake options are Boolean by default. They can't really be tri-state unless we'd use cache variables. But that could cause issues with packagers (e.g., setting a flag to 1 might lead to a different behavior than setting it to ON, or on, or true, ...):
https://cmake.org/cmake/help/latest/command/option.html

[haampie]

You'll be happy to learn that strings like "OFF" and "0" etc are falsy:

cmake_minimum_required(VERSION 3.20)
project(example)
set(EXAMPLE "AUTO" CACHE STRING "EXAMPLE: can be AUTO, ON, OFF")

if (EXAMPLE STREQUAL "AUTO")
    message("Autodetection")
    set(EXAMPLE ON)
endif ()

if (EXAMPLE)
    message("Enabled")
else ()
    message("Disabled")
endif ()

build $ cmake --fresh .. | grep 'ENABLED|DISABLED|AUTO'
Autodetection
Enabled
build $ cmake --fresh .. -DEXAMPLE=ON | grep 'ENABLED|DISABLED|AUTO'
Enabled
build $ cmake --fresh .. -DEXAMPLE=OFF | grep 'ENABLED|DISABLED|AUTO'
Disabled
build $ cmake --fresh .. -DEXAMPLE=0 | grep 'ENABLED|DISABLED|AUTO'
Disabled
build $ cmake --fresh .. -DEXAMPLE=false | grep 'ENABLED|DISABLED|AUTO'
Disabled

[mmuetzel]

What does cmake --fresh .. -DEXAMPLE="This is a test" do? Should that be an error?
What about cmake --fresh .. -DEXAMPLE=auto or cmake --fresh .. -DEXAMPLE=Auto?

[haampie]

I don't think it's something to care too much about, given that people will use ON/OFF 99.9% of the time. If the value is not a special string it's simply truthy.

[mmuetzel]

So AUTO would be different to auto? That would confuse the heck out of me (and probably others).

[haampie]

You can use string(TOLOWER ...), but the auto value is really just a sentinel value that allows you to differentiate between "the user requires it" and "the user didn't specify anything"

Also don't think too much of option(...), cause you can pass arbitrary strings to it and it won't error. It's cmake after all

[haampie]

Actually you can even set(EXAMPLE "AUTO" CACHE BOOL "description"), so a "boolean" with default value AUTO. As long as you set it to ON or OFF later it also behaves nicely in interactive mode (ccmake)

[mmuetzel]

Good to know. But maybe we could postpone that to until after SuiteSparse 7.4.0 is released.
That change doesn't affect the names of the configuration flags. So, we don't need to hurry to avoid changing the configuration interface too often.
On the other hand, if not done carefully, that change might bare the risk of introducing regressions. (E.g., scoping rules are a bit different for cache variables and options. I might be wrong with that though.)

(I hope we are approaching a point where everything is "stable enough" for a release soon.)

[DrTimothyAldenDavis]

I'd like to postpone this too. I'd like to release a stable SuiteSparse 7.4.0 soon, and consider adding AUTO later.

[DrTimothyAldenDavis]

I think these issues (except AUTO) are now resolved in the latest dev2 branch, soon to be released as SuiteSparse 7.4.0.beta6 (hopefully a stable 7.4.0).

I'll leave this issue open for the AUTO consideration, however.

[imciner2]

From the packaging point of view, ensuring there is an error when a flag can't work would be greatly appreciated. There is a good case that highlights this problem in the Julia binary packaging:

We wanted to build just the GPU targets into a separate library, but it turns out CUDA wasn't being configured by our environment correctly, and so CMake couldn't find nvcc to build any CUDA code. CMake's CUDA detection reported the error, but the builder instead continued and just gave us the CPU library again instead of the GPU library like we really wanted. This was kind of just caught by accident when reviewing the logs due to an unrelated packaging failure. So including the option to error if a requested option is unavailable would have caught that before we even merged the new version because it would have failed the build process with an obvious error.

[haampie]

Yeah, that's the kind of thing I had in mind when opening this issue.

Even if you don't wanna make this auto / on / off tri-state, you could still make ON strict regardless. I think more people build in an automated environment than by hand, and if you build by hand, is it that much to ask users to disable e.g. OpenMP if it can't be found?

[mmuetzel]

and if you build by hand, is it that much to ask users to disable e.g. OpenMP if it can't be found?

It's not that trivial. I tried to point out earlier that different sub-projects of SuiteSparse have different version requirements for OpenMP. E.g., ParU requires OpenMP 4.5 or newer. CHOLMOD is fine with OpenMP 2.0.
So, if a toolchain provides OpenMP 2.0, and a user configures to build with OpenMP, should building fail because the available OpenMP is not new enough for ParU? Or should it build OpenMP for the subprojects for which it is new enough and just don't use it for ParU?

The "AUTO" approach would probably work for that. But implementing it will probably take a bit (and might cause regressions in the process).

[DrTimothyAldenDavis]

This is rather complicated.

To simplify the logic in the cmake scripts, and as part of the renaming of variables to prefix them with SUITESPARSE_ or a package name, like UMFPACK_, I now have variables that hold the requested options:

SUITESPARSE_USE_CUDA
SUITESPARSE_USE_OPENMP
GRAPHBLAS_USE_OPENMP
etc.

I never change those values, regardless of whether or not CUDA and/or OpenMP are available. I have another set of values that I set internally that hold the result of whether or not OpenMP and CUDA are used in a particular package, such as:

SPQR_HAS_CUDA
SPQR_HAS_OPENMP

If OpenMP is requested (SUITESPARSE_USE_OPENMP is ON), then I look for OpenMP. Say I'm building GraphBLAS. If I find it, then GRAPHBLAS_HAS_OPENMP is set to ON. If I don't find it, or if it's too old, then GRAPHBLAS_HAS_OPENMP will be OFF.

So I treat the _USE_ variables as read-only (with one exception: GRAPHBLAS_USE_CUDA exists but is forced OFF because it's not ready for production). The _HAS_ variables cannot directly be set by whoever is building SuiteSparse.

This simplifies the logic, and it might also be simpler to revise the logical for the tri-state case. The _USE_ variables could become AUTO / ON / OFF and the _HAS_ would be just ON or OFF.

[DrTimothyAldenDavis]

Actually, in replying in the above just now, I went looking and found that I wasn't as consistent as I meant to be. So I fixed it; see:

#618

For example, SUITESPARSE_USE_FORTRAN tells me that I can go looking for a Fortran compiler (if set to ON). If I find one, then SUITESPARSE_HAS_FORTRAN is set ON. Otherwise, if SUITESPARSE_USE_FORTRAN is OFF, or if I do not find a Fortran compiler, then SUITESPARSE_HAS_FORTRAN is set OFF.

None of the *_HAS_* variables are user-visible (in the ccmake gui for example).

[DrTimothyAldenDavis]

The CUDA setting is also complicated when considering a possible tri-state behavior. CHOLMOD has a setting called CHOLMOD_GPL, which defaults to ON. If set to OFF, then none of the GPL-licensed Modules are used. That includes the GPU Module for CHOLMOD. So if CHOLMOD_GPL is set by the user to OFF, then the CHOLMOD_USE_CUDA is ignored and CHOLMOD does not use CUDA (CHOLMOD_HAS_CUDA is set OFF).

So if CHOLMOD_USE_CUDA becomes tristate: AUTO / ON / OFF, with ON meaning "it must be ON or an error is flagged and CHOLMOD is not built", what happens if CHOLMOD_GPL is simultaneously OFF?

I'm really unsure about how to handle these variations.

[DrTimothyAldenDavis]

Oh ... here's a simple solution. It keeps all my logic the same, and does not introduce any tristate AUTO/ON/OFF settings, but it enables the behavior you want for spack, Julia, linux distros, etc.

I just add a single new ON/OFF setting:

SUITESPARSE_USE_STRICT

By default, this is OFF.

If SUITESPARSE_USE_STRICT is ON, then all _USE_ settings are treated strictly. That is, if package_USE_something is ON but package_HAS_something is OFF because the something is not found, then a fatal error occurs.

I'm adding this now and will include it in SuiteSparse 7.4.0.beta8.

For example:

SuiteSparse/GraphBLAS/CMakeLists.txt

Lines 98 to 115 in 9439bda

	option ( GRAPHBLAS_USE_OPENMP "ON: Use OpenMP in GraphBLAS if available. OFF: Do not use OpenMP. (Default: SUITESPARSE_USE_OPENMP)" ${SUITESPARSE_USE_OPENMP} )
	if ( GRAPHBLAS_USE_OPENMP )
	find_package ( OpenMP GLOBAL )
	else ( )
	# OpenMP has been disabled.
	set ( OpenMP_C_FOUND OFF )
	endif ( )
	if ( OpenMP_C_FOUND )
	set ( GRAPHBLAS_HAS_OPENMP ON )
	else ( )
	set ( GRAPHBLAS_HAS_OPENMP OFF )
	endif ( )
	# check for strict usage
	if ( SUITESPARSE_USE_STRICT AND GRAPHBLAS_USE_OPENMP AND NOT GRAPHBLAS_HAS_OPENMP )
	message ( FATAL_ERROR "OpenMP required for GraphBLAS but not found" )
	endif ( )

[DrTimothyAldenDavis]

See #621

[DrTimothyAldenDavis]

I think all these issues are now fixed in my latest 7.4.0.beta8 version: https://github.com/DrTimothyAldenDavis/SuiteSparse/releases/tag/v7.4.0.beta8 .

Can you let me know if this is resolved?

[DrTimothyAldenDavis]

Fixed in SuiteSparse 7.4.0.