Suggestion for root CMakeLists.txt

[Fabian188]

I propose a simple CMakeLists.txt

It makes use of the SuiteSparse CMake files via cmake external projects and just adds the root CMakeLists.txt.

This is just a template with a small selection of options to demonstrate how it could be re realized.

I'm far from a CMake guru but it appears as an appropriate way for a simple wrapper for the original SuiteSparse CMake structure.

Please feel free to use and modify the file as you like. I wrote it myself and as it is so small and easy I put it into public domain without any restrictions - no need to mention me.

Example usage for an empty SuiteSparse folder: mkdir mybuild; cd mybuild; ccmake ..

When changing the CMakeLists.txt for some major changes it is necessary to clean the build directory (here mybuild) and call make clean of SuiteSparse. I testet it with dev2 from Nov 10

CMakeLists.txt

[DrTimothyAldenDavis]

Thanks -- I'll take a look.

[fabiencastan]

@Fabian188 Very useful! It works fine for me.
I just needed to remove all the lines with:
INSTALL_COMMAND "" # if removed, tried to call make install in build
If we set these lines, it disables the installation of the libraries, which is quite problematic!
If we remove these lines and run cmake with cmake -DCMAKE_INSTALL_PREFIX=/custom/path, it installs everything correctly in the custom path.

[fabiencastan]

Making a confusion in my various tests, I also needed to propagate the CMAKE_INSTALL_PREFIX:

set(CMAKE_ARGS
    -DNSTATIC:BOOL=(NOT BUILD_STATIC)
    -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}  # <<< added this line
    )

[Fabian188]

@fabiencastan thanks for the input. I just created a fork SuiteSparse-root-cmake and included your suggestion.

Feel free to suggest more :)

[Fabian188]

What I do not like with this approach is, that a "make" implies a "make install". This is probably due to the external projects.

[fabiencastan]

@Fabian188 Do you have a link to your proposal? Or could you create a draft PR?

[fabiencastan]

The link: https://github.com/Fabian188/SuiteSparse-root-cmake

[Fabian188]

@DrTimothyAldenDavis I'm currently trying to fix this automatic make install issue.
I want to add some more stuff, I need (only static libs, optionally use lib64 instead of lib) and plan to continue on the fork the next days.

[Fabian188]

Honestly, I beliefe a real central cmake configuration is a much better solution.

• central configuration once and the cmake variables can be read everywhere directly
• e.g. finding and configuring BLAS and openMP only once
• integrate current projects via add_subdirectory() such that all shares a common cmake variable scope.
• my current proposal with ExternalProject_Add() does not really integrate

One could e.g. configure once and then select modules via ccmake or by make AMD, make CHOLMOD, ...

This would be much more work than my approach but in the end the solution is IMHO the better one with much less total cmake code. Central cmake configuration have been realized by https://github.com/jlblancoc/suitesparse-metis-for-windows and https://github.com/sergiud/SuiteSparse. We actually have a own very old solution of my former colleague Simon Triebenbacher which consists of 20 CMakeLists.txt with a total of 700 lines, where the current SuiteSparse 6.0.0 has 35 CMakeLists.txt with a total of 20K lines. I'm happy to share Simon's outdated solution (he'll be happy, too).

[Fabian188]

I solved the instant make install. Now the procedure is as common:

• mkdir build; cd build
• ccmake ..
• make
• make install

[waebbl]

Not sure, if the use of ExternalProject is a good way to do this. From my understanding ExternalProject is meant to download, patch and build projects from another repository. The structure of the SuiteSparse repository rather guides me to think of the root directory as some kine of super project, which can either be build as a whole (with certain options for its sub projects). Alternatively the sub projects could be build independantly as far as the build- and run-time dependencies are met.
Wouldn't it be easier to just use add_subdirectory calls, eventually guarding the cmake code for the sub projects with a variable like SUITESPARSE_IS_SUPER to allow for code which might be different if built in the context of the whole project or as an independant sub project?

[Fabian188]

I also would prefer a central root CMakeLists.txt with add_subdirectory(). However the subprojects are currently independent projects with a total of 20K cmake lines and would require a lot cmake changes.

There are forks doing exactly this, is e.g. Sergui's at https://github.com/sergiud/SuiteSparse
But I'm not sure if these will be upgraded to SuiteSparse 6.0

[waebbl]

Thanks for the link, I wasn't aware of that fork. Should have read all comments before posting.

[DrTimothyAldenDavis]

I'm very reluctant to add a top-level CMakeLists.txt file. It would essentially merge all of the suitesparse packages into a single package with a single build folder. That would require lots of changes to each package cmake build system, and it would make suitesparse monolithic.

Currently, the suitesparse packages are a la carte. End users and pick and choose which they want, and build each independently. This is essential for licensing issues. Each package has a different license -- some GPL, some LGPL, some Apache, some BSD, and one mixed (CHOLMOD: GPL and LGPL). End users who can only use LGPL, Apache, and BSD, for example, can simply remove the GPL packages.

Right now, the top-level Makefile is optional, and each package can just be built as its own package.

[waebbl]

I'm very reluctant to add a top-level CMakeLists.txt file. It would essentially merge all of the suitesparse packages into a single package with a single build folder. That would require lots of changes to each package cmake build system, and it would make suitesparse monolithic.

Currently, the suitesparse packages are a la carte. End users and pick and choose which they want, and build each independently. This is essential for licensing issues. Each package has a different license -- some GPL, some LGPL, some Apache, some BSD, and one mixed (CHOLMOD: GPL and LGPL). End users who can only use LGPL, Apache, and BSD, for example, can simply remove the GPL packages.

Right now, the top-level Makefile is optional, and each package can just be built as its own package.

I don't think it would make suitesparse monolithic. You could add options to select each package individually and use the cmake_dependent_option to drive inter-package dependencies, so the top-level CMakeLists.txt just acts as a driver to select what is going to get build and also use this to control the licensing issues.

My view is from the perspective of a package maintainer for the source-based Gentoo Linux distribution. While the package is being released as a single tarball, we have to maintain several packages with different versioning, instead of a single package with options to build and install each of the suitesparse packages. It could make the life of maintainers easier if we would have the possibility of a top-level CMakeLists.txt.

[DrTimothyAldenDavis]

Thanks for the feedback. I'll consider it in a future release (after I post v6.0.2, which should appear soon).

[DrTimothyAldenDavis]

v7.0.0.beta1 is now posted. I'll be considering this for v7.1.0 in the near future. I do agree it would make life easier for Windows users, as something to use in place of my simple (but linux/mac only) top-level Makefile.

[chengts95]

I have created a CMake file to build all modules independently. I don't think using ExternalProject violated any license since the codes are not even copied during the building stages.

[DrTimothyAldenDavis]

We have something like this in progress. There are lots of pending changes to the SuiteSparse build system, in the various development branches.

[chengts95]

Thank you for the reply. What about the suggestions i made here #309 ? As I know, these Find*.cmake modules are supposed to be used for finding libraries not built by CMake. For CMake install targets we should use something like this:

    install ( TARGETS amd_static
        EXPORT  ${PROJECT_NAME} #Export a target that can be install as SuiteSparse::amd_static
        ARCHIVE DESTINATION ${SUITESPARSE_LIBDIR} ) 
    target_include_directories(amd_static INTERFACE
        $<INSTALL_INTERFACE:${SUITESPARSE_INCLUDEDIR}>) #set the interface header dir
install (EXPORT  ${PROJECT_NAME}
        FILE  ${PROJECT_NAME}Config.cmake
        NAMESPACE SuiteSparse::
        DESTINATION ${CMAKE_INSTALL_PREFIX}/cmake) #install the cmake config modules 

[mmuetzel]

The CMake build rules are modified to export targets instead of modules now. (There can still be improvements to them. But the basic transition should be complete.)

If we'd like to replace the root Makefile by a root CMakeLists.txt, we'd probably need to add support for all existing Makefile targets.
IIUC, the demos target in the current root Makefile doesn't yet corresponds to CMake rules for all subprojects.
It would probably be good to convert those demos to ctests. (That would probably also make it easier to spot failing tests in the CI.)
I can try to start with that. But I'd probably need some help. For some libraries, I don't know how to correctly interpret the output of that target to the console.
E.g., does the following output in the CI mean that the test succeeded?
https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/6191999485/job/16811275748#step:9:1360
What would be the criteria to decide that the tests failed?

[Fabian188]

From my experience with ctest, there needs to be a tool doing the decision and returning either 0 (ok) or another value (at least return values >= 1 indicate errors).

I quickly checked cs_demo1/2/3.c and it seems that these always return 0 within main.
So the demos shall probably be adapted for ctest if I understand them right.

If one does still want to run all tests and not exit on the first failing function, the tests could return an error value, main could sum the return values and return it for the demo. So only if all functions return 0, the result will be 0.

[DrTimothyAldenDavis]

E.g., does the following output in the CI mean that the test succeeded?
https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/6191999485/job/16811275748#step:9:1360
What would be the criteria to decide that the tests failed?

Yes, those are all passing tests. I would need to work through all my packages to revise the demos to return 0 from the main program, to make them suitable for ctest, however. I am not sure if I do that now.

I've used ctest a bit, but not in my own package. LAGraph is a package I contribute to quite a bit as a co-author, and it has a ctest (I didn't write the ctest framework, but I do write tests in LAGraph for ctest). See https://github.com/GraphBLAS/LAGraph . At some point (soon) when LAGraph becomes reasonably stable, I will mirror it inside SuiteSparse. It's a package that relies on GraphBLAS to solve graph problems (or any library that implements the GraphBLAS C API, in particular, not just SuiteSparse/GraphBLAS).

A ctest framework would be a very good idea to add in the future. It will take me a while to get to this though.

It wouldn't be necessary for a top-level cmake build system to enable all existing Makefile targets, at least at first. ctest would be nice. I also have a top-level "make cov" which does test coverage in all packages. Those tests are substantial but only work on Linux. Most work on the Mac I think. I doubt any would work (as is) on Windows. The GraphBLAS test coverage is a very strange one: it has to be run inside MATLAB, and it requires my own test coverage mechanism because you can't do statement coverage of a C code called by MATLAB using existing tools. It takes about 2 hours to run. I've run it on Mac and Linux; I don't think it works on Windows but it might.

[mmuetzel]

In the light of #383 and #408: Are SuiteSparse libraries meant to be compilable as stand-alone projects?
If they aren't and it would be ok to build all libraries via a root CMakeLists.txt (maybe with the option to select which libraries to build), that would probably make the design of the build system a bit easier.

[DrTimothyAldenDavis]

I didn't write the original LAGraph CMakeLists.txt. I can't recall why it got added. I think it's an attempt to pick up a non-SuiteSparse GraphBLAS, but that should be handled by FindGraphBLAS.cmake anyway (if there is another version).

So let's delete that line.

[DrTimothyAldenDavis]

The original reason for that line is here, in the comments:

https://github.com/GraphBLAS/LAGraph/blob/09c7d372374178c1370b2e8c3ae8bb2cfbb2c939/CMakeLists.txt#L170

[mmuetzel]

I opened #552 that deletes that line.

[DrTimothyAldenDavis]

I'll have a few more changes as I sync the SuiteSparse/LAGraph v1.1 with the original LAGraph repo. So far it's looking good; just one tiny change to the LAGraph/CMakelists.txt. I'll push that to SuiteSparse shortly.

[DrTimothyAldenDavis]

These changess allow LAGraph to use GraphBLAS 7.1.0 (using its old build system, and older style FindGraphBLAS.cmake), and it allows LAGraph to be built outside of SuiteSparse:

5e4e5df

The LAGraph/CMakeLists.txt file doesn't change much. Just the place where it doesn't find itself inside SuiteSparse. I had to copy the SuiteSparsePolicy.cmake into the LAGraph/cmake_modules, which is a bit unfortunate, but it works.

[Fabian188]

I freshly cloned

git clone https://github.com/DrTimothyAldenDavis/SuiteSparse.git SuiteSparse-org
cd SuiteSparse-org
git checkout dev2
git log | head -> allow LAGraph to be used stand-alone, outside of SuiteSparse
mkdir mybuild
cd mybuild
cmake ..
make

And compilation fails

[  7%] Building C object CHOLMOD/CMakeFiles/CHOLMOD_static.dir/Supernodal/cholmod_l_super_numeric.c.o
In file included from /Users/fwein/code/SuiteSparse-org/CHOLMOD/Supernodal/cholmod_l_super_numeric.c:13:
In file included from /Users/fwein/code/SuiteSparse-org/CHOLMOD/Supernodal/cholmod_super_numeric.c:88:
/Users/fwein/code/SuiteSparse-org/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c:777:17: error: call to undeclared function 'SUITESPARSE_BLAS_ssyrk'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                SUITESPARSE_BLAS_ssyrk ("L", "N",

Interesting that it does fail for Apple clang version 15.0.0 but not vanialla clang 17, often I have similar issues on both - for IntelLLVM (also clang based) the issues are more often different.

I suggest to handle this for "AppleClang".

[mmuetzel]

Does the following change fix the error for you?

diff --git a/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c b/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c
index 8cec363cb..fdfee8f29 100644
--- a/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c
+++ b/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c
@@ -9,6 +9,7 @@
 //------------------------------------------------------------------------------
 
 #include "cholmod_template.h"
+#include "SuiteSparse_config.h"
 
 // Template routine for cholmod_super_numeric.  All xtypes supported, except
 // that a zomplex A and F result in a complex L (there is no supernodal zomplex

[Fabian188]

Does the following change fix the error for you?

No, same error.

[DrTimothyAldenDavis]

This is the same problem: you are getting a stale SuiteSparse_config.h.

The old SuiteSparse_config.h does not have macros that wrap the single precision BLAS. THe new one does. So you're getting an old SuiteSparse_config.h from somewhere.

[mmuetzel]

Could you please run cmake --fresh .. | tee cmake_config.log and upload that file here?

[DrTimothyAldenDavis]

I should revise all of my public headers to make them check version numbers of my other packages they rely on. They would #error in the C/C++ preprocessor if the version is outside a valid range. The error message would be more clear than something like "Suitesparse BLAS ssyrk not defined" from the compiler.

All my packages have #defined package_VERSION_* macros just for this purpose.

For example, cholmod 5.1.0 requires suitesparse_config 7.4.0 and will die badly (what you see here) if it finds an older Suitesparse_config.h.

[Fabian188]

Could you please run cmake --fresh .. | tee cmake_config.log and upload that file here?

Here you are - I'm quite busy today and almost not at my computer

cmake_config.log

[Fabian188]

This is the same problem: you are getting a stale SuiteSparse_config.h.

The old SuiteSparse_config.h does not have macros that wrap the single precision BLAS. THe new one does. So you're getting an old SuiteSparse_config.h from somewhere.

You are right, there is a SuiteSparse_config.h in my /usr/local/include from nov 2022.
What issue do you claim to come from that? The -Wimplicit-function-declaration thing?

/Users/fwein/code/SuiteSparse-org/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c:777:17: error: call to undeclared function 'SUITESPARSE_BLAS_ssyrk'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                SUITESPARSE_BLAS_ssyrk ("L", "N",

I still have it after removing the file in /usr/local/include and cmake --fresh. It compiles with the suppression of the warning.

[mmuetzel]

It looks like OpenMP is adding that path to the include directories:

-- OpenMP C libraries:      /opt/homebrew/lib/libomp.dylib 
-- OpenMP C include:        /opt/homebrew/include 
-- OpenMP C flags:          -Xclang -fopenmp 

I'll try and propose some changes.
@Fabian188: If it's ok with you, I'll ping you if I find something. Could you test changes when you come around to it?

Fwiw, it's the following for me (using Clang/MinGW):

-- OpenMP C libraries:      C:/msys64/clang64/lib/libomp.dll.a
-- OpenMP C include:
-- OpenMP C flags:          -fopenmp=libomp

[Fabian188]

It looks like OpenMP is adding that to the include directories:

-- OpenMP C libraries:      /opt/homebrew/lib/libomp.dylib 
-- OpenMP C include:        /opt/homebrew/include 
-- OpenMP C flags:          -Xclang -fopenmp 

AAIK installing an external OpenMP is still the necessary variant for macOS. So all developers shall have this configuration.

But note, that the issues were on /usr/local/include.

brew is a common packager for macOS. With x86_64 it used /usr/local to install code. With arm64 the arm64 compiled tools are by default installed to /opt/homebrew. However one can run the x86_64 binaries from /usr/local transparently via emulation.

I'll try and propose some changes. @Fabian188: If it's ok with you, I'll ping you if I find something. Could you test changes when you come around to it?

Sure, I'll respond whenever it is possible. The question is no how to compile SuiteSparse out of the box on macOS, right?

[Fabian188]

in /opt/homebrew/include I again have the headers, now more recent

ll GraphBLAS.h SuiteSparse_config.h
lrwxr-xr-x  1 fwein  admin  48 28 Apr  2023 GraphBLAS.h -> ../Cellar/suite-sparse/7.0.1/include/GraphBLAS.h
lrwxr-xr-x  1 fwein  admin  57 28 Apr  2023 SuiteSparse_config.h -> ../Cellar/suite-sparse/7.0.1/include/SuiteSparse_config.h
include git:(master) pwd
/opt/homebrew/include

[mmuetzel]

Sure, I'll respond whenever it is possible. The question is no how to compile SuiteSparse out of the box on macOS, right?

It compiles with Homebrew on macOS in the CI on GitHub. However, the CI doesn't install an older version of the SuiteSparse libraries before building. So, issues like the one you are seeing can go unnoticed...

[mmuetzel]

in /opt/homebrew/include I again have the headers, now more recent

IIUC, you'd need the headers from version 7.4.0 (or newer). That (together with the OpenMP include directory) could explain the C99 error...
Maybe #555 could help with that.

[mmuetzel]

@DrTimothyAldenDavis: We can't do anything about older versions. But could the headers of the SuiteSparse libraries be installed in a sub-directory (e.g., /usr/local/include/suitesparse)?
Currently, there is the risk of headers from older libraries being picked up when the include path (e.g., /usr/local/include) gets added for other dependencies installed on the build system (e.g., OpenMP). That risk would be lower if the headers were installed in their own directory. CMake targets and pkg-config files would help dependent projects to find the correct folder with the headers.

Probably not something that should be changed for the current release (or ever). But maybe for a later release?

Edit: Looking at the commit log, this is probably not a new issue. But it might have gone unnoticed so far because there weren't any breaking changes in SuiteSparse_config.h in a long time...

[DrTimothyAldenDavis]

with this change to the dev2 branch:
bc21716
all headers are checked for the right versions, and an #error will occur in the compiler, if a stale header is found.

[DrTimothyAldenDavis]

This is the same problem: you are getting a stale SuiteSparse_config.h.
The old SuiteSparse_config.h does not have macros that wrap the single precision BLAS. THe new one does. So you're getting an old SuiteSparse_config.h from somewhere.

You are right, there is a SuiteSparse_config.h in my /usr/local/include from nov 2022. What issue do you claim to come from that? The -Wimplicit-function-declaration thing?

/Users/fwein/code/SuiteSparse-org/CHOLMOD/Supernodal/t_cholmod_super_numeric_worker.c:777:17: error: call to undeclared function 'SUITESPARSE_BLAS_ssyrk'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                SUITESPARSE_BLAS_ssyrk ("L", "N",

I still have it after removing the file in /usr/local/include and cmake --fresh. It compiles with the suppression of the warning.

Yes, if the SUITESPARSE_BLAS_ssyrk is being seen as an undeclared function, then you're getting a stale SuiteSparse_config.h. Their would be many other errors as well.

I have a stale SuiteSparse_config.h in /usr/local/include on my Mac, because that's where "make install" in SuiteSparse installs itself, and I had my own (old) copy there. But doing a fresh build doesn't bring that in. The old one is version 7.2.0, and the new one is 7.4.0, and none of my #error's trigger, at least with this update on the dev2 branch.

[DrTimothyAldenDavis]

The root CMakeLists.txt file is working well in the prerelease (SuiteSparse 7.4.0.beta1). Shall I close this issue?

[Fabian188]

Thanks both of you very much for your effort! I close the issue. Possibly I suggest in the future variants of handling the root CMakeLists.txt.

[DrTimothyAldenDavis]

Thanks again for the suggestion and for the help in testing it.