Optimize CMake install targets and add top-level CMakeLists.txt

[chengts95]

Is your feature request related to a problem? Please describe.
Currently, the project consists of many small dependent CMake projects. But there is no top-level CMakeLists.txt which is very inconvenient and inconsistent especially on Windows.
Moreover, "Find*.cmake" was designed to find third-party libs built without CMake, these files are cumbersome and not stable. The current user experience is like the project is not much easier to use than the pure makefile age.
Describe the solution you'd like

I propose using ExternalProject command from CMake to build all projects and maybe create a universal CMake install target.

cmake_minimum_required(VERSION 3.19)

if(WIN32)
#Set vcpkg to install BLAS on Windows
  set(VCPKG_TARGET_TRIPLET "x64-windows-static" CACHE STRING "")
  set(CMAKE_TOOLCHAIN_FILE "path to vcpkg cmake module"
    CACHE STRING "Vcpkg toolchain file")
  set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded)
endif()

project(SuiteSparse)
set(CMAKE_INSTALL_PREFIX ${CMAKE_CURRENT_SOURCE_DIR}/install)

# Targets we have
set(TARGETS_LIST SuiteSparse_config Mongoose AMD BTF CAMD CCOLAMD COLAMD CHOLMOD CXSparse LDL KLU UMFPACK RBio SuiteSparse_GPURuntime GPUQREngine SPQR GraphBLAS SPEX)


#  ExternalProject
include(ExternalProject)

foreach(TARGET ${TARGETS_LIST})
  # sub projects are built in their own folders while codes stay in-place
  ExternalProject_Add(${TARGET}
    PREFIX ${TARGET}
    SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/${TARGET}
    CMAKE_ARGS
    -DCMAKE_TOOLCHAIN_FILE:STRING=${CMAKE_TOOLCHAIN_FILE}
    -DCMAKE_INSTALL_PREFIX:PATH=${CMAKE_INSTALL_PREFIX}

    BUILD_COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR}/${TARGET}/src/${TARGET}-build --config Release
    INSTALL_COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR}/${TARGET}/src/${TARGET}-build --config Release --target install
    CONFIGURE_COMMAND
    ${CMAKE_COMMAND} -DCMAKE_BUILD_TYPE=$<CONFIG> -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}
    -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded
    -DCMAKE_INSTALL_PREFIX:PATH=${CMAKE_INSTALL_PREFIX}
    -DBLA_STATIC=ON
    -DBLA_VENDOR=OpenBLAS ${CMAKE_CURRENT_SOURCE_DIR}/${TARGET}
  )
  add_library(SuiteSparse::${TARGET} INTERFACE IMPORTED)

  set_target_properties(SuiteSparse::${TARGET} PROPERTIES
    INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_INSTALL_PREFIX}/${TARGET}/include
  )
endforeach()

Moreover, exposing the installed CMake targets via "*Config.cmake" files can significantly simplify the building process and increase the usability of the library.

Describe alternatives you've considered
I believe the above method is relatively easy and useful. Maybe there is a better way to create a simpler process.

[chengts95]

After fixing some dependency problems manually, I got these binaries with the CMake command i provided:

and I have no idea why I can build *_cuda.dll even though I didn't install any cuda. And UMFPack is missing for unknown reasons with OpenBlas is presented.

[chengts95]

The current SuiteSparse cmake projects are referring to others by Find*.cmake modules and relative paths instead of a proper CMake install target. This results in much confusion and redundant header including sets.

I created a branch to demonstrate how we use can CMake properly. Generally, I dropped all Find*.cmake and create each target via CMake APIs. The Find*.cmake modules are used for finding non-cmake-built libraries.
Here is my experimental branch. Branch Diff. In the past, the Find*.cmake construct {libraries},{headers} as variables which is not very popular in modern CMake. Now with my modifications, we can import a module easily:

set(CMAKE_PREFIX_PATH "${CMAKE_PREFIX_PATH};${SUITESPARSE_DIR}/lib/cmake/")
find_package(suitesparseconfig REQUIRED)
find_package(colamd REQUIRED)
find_package(amd REQUIRED)
find_package(btf REQUIRED)
find_package(klu REQUIRED) #these find packages can be fused if we define a top-level package

target_link_libraries(circuit  SuiteSparse::klu)  # SuiteSparse::klu_static if you want static lib
# that is all, no need to add individual modules as dependency is recorded in SuiteSparse::klu

Finally, I can import KLU with less trouble.

[K20shores]

This seems like it may be related to #267, possibly?

[chengts95]

Yes. The differences are:

1. I do not want to add a custom cmake template because the auto-generated target is good enough;
2. I put every package in the SuiteSparse namespace so they are imported as SuiteSparse::klu, etc.
3. I provide a top-level CMakeLists.txt to build everything and produce a top-level SuiteSparseConfig.cmake to auto-load every library in one find_package. In this case, all references to the relative CMake build folder are purged. Sub-packages are referring to previously built packages from the installed paths.
4. I also disabled the custom FindBlas in my branch because it has some problems finding BLAS.

[mmuetzel]

Regarding your point 1: IIUC, @DrTimothyAldenDavis needs the variables that are currently set in the Find*.cmake modules in some places where cmake targets won't work. So, a replacement needs to still define those.

Points 2 and 3 sound interesting. Especially, point 2. Do you mind if I'd include that at some point in #267. (See #267 (comment).)
For the moment, I thought it would be more important to avoid overlinking when changing to the approach with cmake targets. (See #306.)

I can't comment on point 4.

[chengts95]

I didn't delete find*.cmake, but when building with cmake they are useless. find*.cmake is used to find libraries built without cmake. And we don't need to install these files because we can simply copy all these files to a folder without cmake's help. They are not needed for CMake targets because CMake produces everything needed to use its own targets. You can try my branch, there is only an included file needed for some flags and variables, not find*.cmake.

Find*.cmake files are for non-CMake projects, logically they shouldn't be used with packages managed by CMake.

Free feel to use my changes because I wrote these with the intention to help others. My principle is, when i build these projects, I build with order and install them one by one. For example, the suitesparseconfig is installed to install_prefix first, then other projects can use find_package to link suitesparseconfig from the installed location with its properly generated cmake targets just like how users use the libraries.

There is nothing overlinked in my suitesparse top package because it is just a alias to a set of find_package. As I know, I cannot use KLU without btf.dll and other DLLs so they are not overlinked (and I am not sure whether you still need to provide individual dependent static libraries if use the private flag to link static libraries). If these dlls are in the dependencies, they should appear in the final cmake targets. For example, when you apt install a package in Debian Ubuntu, wouldn't it become natural to download all dependencies automatically? If they are necessary when I link to KLU, the btf, and other dlls are not overlinked so they should be linked with public flags.

I thought if you link to dlls with private flags, they may hide the dependencies required to link DLLs. I would suggest you check the INTERFACE_LINK_LIBRARIES of klu target to see if the private link flag still preserves the dependencies. In my branch, I managed to install Klu's dependencies with this command in an external cmake project :

get_target_property(klu_location SuiteSparse::klu INTERFACE_LINK_LIBRARIES)

install(IMPORTED_RUNTIME_ARTIFACTS SuiteSparse::klu
DESTINATION bin)
foreach(dll  ${klu_location} )
install(IMPORTED_RUNTIME_ARTIFACTS ${dll}
DESTINATION bin)
endforeach()

I believe it only works when you link DLLs with public flags, but i don't have time to test private flags at the moment.

[mmuetzel]

They are not needed for CMake targets because CMake produces everything needed to use its own targets.

While that is kind of true, there can still be cases where CMake can't generate build rules automatically from targets. ISTR that the use-case here was to gather the compiler arguments to build .mex files with Matlab/Octave. It is possible to gather those arguments from the CMake targets. But it requires a bit more than a couple of lines to get that information for all targets and with all generators. I agree with @DrTimothyAldenDavis that it is easier to have that logic in one place and keep it available for all downstream users instead of sprinkling it across different projects.

As I know, I cannot use KLU without btf.dll and other DLLs so they are not overlinked

I'm not sure whether we are talking about the same thing here. There are probably better sources to explain the issue with overlinking shared libraries. But let me try to give a short example trying to illustrate it:
Let's say there is a shared library B that depends on a shared library A. I.e., library B might call some functions that are implemented in library A. Let's say further that there is an application C that depends on library B. I.e., it might call some functions that are implemented in library B, but it does not call any functions that are implemented in library A directly.
You are right that it is necessary to install both libraries A and B to be able to execute application C correctly. But it is not necessary to link application C against library A.
In fact, let's imagine what would happen on an update of the shared library A that breaks its API. It might be easier to imagine the effects on a platform that bumps the SONAME on API breakage (i.e., the name of the shared library A is changing on that platform):
If application C is not linked against library A, a user (or distributor) would only need to rebuild library B, and the application C would just continue working as before.
On the other hand, if application C is directly linked against library A (i.e., "overlinking" it), a user (or distributor) would need to rebuild library B and application C. That's the main reason why overlinking should be avoided afaict.

[chengts95]

As long as you want to link to klu, link to btf and other stuff cannot be omitted. Your example is strange to me, if A.dll changes that means the function name has been changed, and B depends on A, that is to say, B must be rebuilt after A changes the APIs because it refers to A's APIs at runtime. You simply cannot use old B with new A.dll. If you think you can change A's API without rebuilding B, it creates a timed bomb for B to throw an "unknown reference to function xxx".

If I link Klu to my program statically, all other modules are assumed to be called because Klu will call them and that is the case for the static library. You cannot even build a klu program without the link to static libraries such as btf even if your program use nothing from btf. Remember, link public or private to static libraries are always public:

If I link KLU to my program dynamically, klu must be assumed to call other dlls, so that I have to make sure all dlls are there and APIs are consistent. If the depended btf dll is changed, and it changed its API function name that is used, then the outdated KLU will fail when my program calls KLU routines. Therefore, If program C links to KLU, it should be linked to BTF too because it uses BTF indirectly.

In our case, cmake doesn't care about the SONAME because the cmake targets are installed by cmake, the update-to-date targets are always found when i use find_package.

CMake targets are used for cmake C/C++ projects. If you are talking about mex, i don't find anything special in the current Find*.cmake for mex files. The project-oriented definitions are all in this file.
The Find*.cmake doesn't provide anything special except the {_libraries}, {_includes} variables since special things cannot be recovered when you installed all targets in to install prefix.

I didn't touch anything in Find*.cmake, and the CMakeLists.txt of original projects doesn't contain the rules to build mex so there is no need to consider mex in auto-generated targets. Therefore, these Matlab special cases should be considered in additional config files instead of merging all these together. I don't believe that Matlab mex libraries cannot be built with knowledge about headers, dlls or static libraries.

[mmuetzel]

You simply cannot use old B with new A.dll.

Please read carefully. I never wrote that. My point was whether application C needs to be recompiled.

If I link Klu to my program statically

Again please read carefully: I explicitly wrote about shared libraries in the example - not static libraries. And if you'll have a look at #306, you'll find that the dependencies for static libraries are kept PUBLIC.

Therefore, If program C links to KLU, it should be linked to BTF too because it uses BTF indirectly.

That is incorrect when using shared libraries afaict. There is no such requirement. And best-practices recommend against it.

the CMakeLists.txt of original projects doesn't contain the rules to build mex so there is no need to consider mex in auto-generated targets.

Even if the CMakeLists.txt files within SuiteSparse don't contain rules for building .mex files, that doesn't mean that there are no downstream projects that do...

[chengts95]

You simply cannot use old B with new A.dll.

Please read carefully. I never wrote that. My point was whether application C needs to be recompiled.

If I link Klu to my program statically

Again please read carefully: I explicitly wrote about shared libraries in the example - not static libraries. And if you'll have a look at #306, you'll find that the dependencies for static libraries are kept PUBLIC.

Therefore, If program C links to KLU, it should be linked to BTF too because it uses BTF indirectly.

That is incorrect when using shared libraries afaict. There is no such requirement. And best-practices recommend against it.

the CMakeLists.txt of original projects doesn't contain the rules to build mex so there is no need to consider mex in auto-generated targets.

Even if the CMakeLists.txt files within SuiteSparse don't contain rules for building .mex files, that doesn't mean that there are no downstream projects that do...
I am very sorry I made huge mistakes in understanding what you said. I read that comment at 2 a.m..

I think you are right. Dynamic libraries are not using the same rules as static libraries. I can see the real problem is not just recompiling but duplicating symbols if C depends on D with the same symbols as A. I can accept the duplicated symbol problems because they are reported at the compiled time to remind users to check their link flags.
Recompiling is just a tiny problem because dynamic linking is very fast.

Linking dlls privately brings a question of whether the klu's dependent dlls should be hidden from users.
Actually, it is better to link static libraries if I want to ensure all libraries are included in one piece, but there is a GPL problem with static linking for some libraries.

For mex, my comments were they should be added with additional configurations. Building a mex library should be no different from building a regular c/c++ program. What I find is we can use matlab_add_mex in CMake to build mex libraries, which is likely not used by current SuiteSparse.

If i maintain downstream projects, I dislike special manual configurations in the building process and I prefer a cmake target with default behaviors.

I feel building a mex doesn't have differences from linking to a normal C library so no special configurations are needed.
If we really want to produce the old ${_LIBRARIES} and ${_INCLUDES} variables, we can produce these from the target's properties. There is no reason to override CMake's default configs and manually create the config.cmake just to produce some causal variables. If we really want them we can produce a virtual target based on auto-generated install targets.

[mmuetzel]

If i maintain downstream projects, I dislike special manual configurations in the building process and I prefer a cmake target with default behaviors.

I agree. But I also agree with @DrTimothyAldenDavis that there should be at least a transition period where downstream can continue using the current cmake variable. Maybe that "transition period" should even be indefinite.

If we really want to produce the old ${_LIBRARIES} and ${_INCLUDES} variables, we can produce these from the target's properties. There is no reason to override CMake's default configs and manually create the config.cmake just to produce some causal variables. If we really want them we can produce a virtual target based on auto-generated install targets.

Maybe I'm misunderstanding what you mean. But isn't that very similar to what is proposed in #267? (E.g., https://github.com/DrTimothyAldenDavis/SuiteSparse/pull/267/files#diff-6c61cdc8330af33a389d42be594819a8f12070dd984139abe8ded32fc740ad24R38 include ( ${CMAKE_CURRENT_LIST_DIR}/AMDTargets.cmake ))

[chengts95]

I think the transition period can be keeping the old Find*.cmake and the new install target together.

What I mean is we should do it in the opposite way: keep the auto-generated target as default Config.cmake and put backward compatibility somewhere else. In my original plan, the backward compatibility is provided by the old Find*.cmake while the new config only provides new features.

However, I realized the old Find*.cmake are not convenient to use so I understand why you just removed them. What i suggest now is the templates can be more automated, like this:

@PACKAGE_INIT@
set ( @PACKAGE_NAME@_DATE "@DATE@" )
set ( @PACKAGE_NAME@_VERSION_MAJOR @VERSION_MAJOR@ )
set ( @PACKAGE_NAME@_VERSION_MINOR @VERSION_MINOR@ )
set ( @PACKAGE_NAME@_VERSION_PATCH @VERSION_SUB@ )
set ( @PACKAGE_NAME@_VERSION "@VERSION_MAJOR@.@VERSION_MINOR@.@VERSION_SUB@" )

include ( ${CMAKE_CURRENT_LIST_DIR}/@[email protected] )

# The following is only for backward compatibility with FindKLU.

set ( _target_shared @DYNAMIC_LIBRARY_NAME@ )
set ( _target_static  @STATIC_LIBRARY_NAME@ )
set ( _var_prefix "@PACKAGE_NAME@" )

to have a more universal template for all targets. Do you think this is better? Also, we can modify project names to capitalized project names and make a consistent PACKAGE_NAME = PROJECT_NAME.
And maybe we should consider a top-level cmake module to avoid :

set ( CMAKE_PREFIX_PATH ${CMAKE_PREFIX_PATH}
    ${CMAKE_SOURCE_DIR}/../AMD/build
    ${CMAKE_SOURCE_DIR}/../BTF/build
    ${CMAKE_SOURCE_DIR}/../CAMD/build
    ${CMAKE_SOURCE_DIR}/../CCOLAMD/build
    ${CMAKE_SOURCE_DIR}/../CHOLMOD/build
    ${CMAKE_SOURCE_DIR}/../COLAMD/build
    ${CMAKE_SOURCE_DIR}/../SuiteSparse_config/build )

Because we should not decide on building directories when the building directories can be changed by users. Meanwhile, there is a chance to collide with system packages. If we want to specify the directories for find_package we can use {NAME}_DIR instead. They should not be specified in subprojects if we want to make subprojects really independent. In my opinion, it is better to use installed packages from the install prefix or assign the paths from top-level cmake lists. I believe we should add the top-level CMakeLists.txt to ultimately solve the cmake building issue.

[mmuetzel]

to have a more universal template for all targets. Do you think this is better?

I was considering that. However, these variable names aren't consistent across the different libraries. So instead of coding a bunch of exceptions, I decided it would be cleaner to just handle that on a per-library level.

Because we should not decide on building directories when the building directories can be changed by users.

Afaict, those are only there to simplify building in a "common" source tree. If users prefer to build in separate trees, they'd need to set CMAKE_PREFIX_PATH accordingly themselves.
Similarly for the CMAKE_MODULE_PATH for find_package.

I believe we should add the top-level CMakeLists.txt to ultimately solve the cmake building issue.

I agree. But I think that can be handled independently (i.e., in a separate PR).

[chengts95]

That is what we want to improve, make them more consistent.

But for example, if the system has an AMD target installed, it will collide with the local path added to CMAKE_PREFIX_PATH and create confusion. if the user wants to specify a dir for AMD, they can set AMD_DIR. You can set CMAKE_PREFIX_PATH but it is not easy to remove a path from it. Besides, from my experience the find_package uses CMAKE_PREFIX_PATH instead of CMAKE_MODULE_PATH

[mmuetzel]

That is what we want to improve, make them more consistent.

I guess you are referring to the *_VERSION* variables. Is that correct?
In case you do: One thing is what we might want to do. Another thing is what we might have to do for backward compatibility.

You can set CMAKE_PREFIX_PATH but it is not easy to remove a path from it. Besides, from my experience the find_package uses CMAKE_PREFIX_PATH instead of CMAKE_MODULE_PATH

Thanks for pointing that out. Re-reading the documentation, I think we should be setting <PackageName>_ROOT instead.
There is also CMAKE_FIND_PACKAGE_REDIRECTS_DIR which sounds like it could be more convenient once we switch to building with a top-level CMakeLists.txt. But that was added for 3.24. And afaict, SuiteSparse currently requires only version 3.22. It might also be more difficult to use if building a single library should still be supported (i.e., without using the top-level cmake file).

[chengts95]

Obviously these

set ( KLU_DATE "Jan 17, 2023" )
set ( KLU_VERSION_MAJOR 2 )
set ( KLU_VERSION_MINOR 0 )
set ( KLU_VERSION_SUB   3 ) 

should be

set ( DATE "Jan 17, 2023" )
set ( VERSION_MAJOR 2 )
set ( VERSION_MINOR 0 )
set ( VERSION_SUB   3 )

And why we are setting versions here and there? why not use the existing CMake project to obtain these versions? Besides, I think we should add VERSION ${PROJECT_VERSION} to write_basic_package_version_file

.
I believe my proposal of the config template should work without too much trouble. because we pass KLU or AMD only once ( if they are just project names, it is even better because why the project name is lower cases while the package name is capitalized, and one project for one installed package is awesome). In this way we won't be afraid of misspelling somewhere in nearly 60 text files. You can pass specific version variables to the template. Because the find_package command must match the XXX_LIBRAIRES's name, I believe there is no problem to automate this backward compatibility template. I don't think manually maintaining these names everywhere is a good solution, but maybe we won't need to touch these manually created templates anymore.

I think a top-level cmake file is quite useful if we want to improve the overall user experience. I don't know what is the difference between <PackageName>_ROOT and <PackageName>_DIR. I used to use <PackageName>_DIR a lot and it can override global prefix effectively. and It is very safe to use these variables because: "If _DIR has been set to a directory not containing a configuration file CMake will ignore it and search from scratch."

If we really want to assign fixed paths for internal find_packages, we may directly provide paths like this:

find_package (<PackageName> PATHS paths... NO_DEFAULT_PATH)

[mmuetzel]

I'm not going to argue with you whether the current VERSION variable names should be kept or not. Imho that is completely independent of whether Find*.cmake modules or *Config.cmake files are used. That can be changed at an earlier or later time in an independent PR. Feel free to propose that.

IIUC, the main difference between <PackageName>_ROOT and <PackageName>_DIR is the search priority. <PackageName>_ROOT is searched before CMAKE_PREFIX_PATH - <PackageName>_DIR after.
Like you correctly pointed out, it could lead to unexpected results if a user already installed an older version at CMAKE_PREFIX_PATH. Setting <PackageName>_ROOT would avoid that potential issue.

If we really want to assign fixed paths for internal find_packages, we may directly provide paths like this:

find_package (<PackageName> PATHS paths... NO_DEFAULT_PATH)

Didn't you write earlier that you think building the libraries independently in separate source trees or build trees should be possible?

[chengts95]

Yes, it is not only possible but also verified.

I reconsider this _ROOT or Prefix problem. I found the real conflict is when I have a system suitesparse package installed and build the packages out of the build folder you assigned (because normally I prefer putting all build files in one build folder). It will silently use system packages instead of the latest build because CMAKE_PREFIX_PATH is just a hint. If we change it to _ROOT, it seems that we still have fixed the build trees which is not a good idea. The real solution is to remove the path to these temporal build trees.

My idea is to provide a temporal prefix by commands such as ExternalProject_Add to specify the installed package location instead of their build trees and another command can create a dependency between projects so everything is built in order. According to the doc, CMAKE_SYSTEM_PREFIX_PATH contains CMAKE_INSTALL_PREFIX, so if we install all libraries to CMAKE_INSTALL_PREFIX in order, they should be found automatically. At least in this way, we don't need to worry about the paths of source trees.

But local libraries may still co-exist with system-installed libraries. In this case, we set the CMAKE_PREFIX_PATH, which has higher priority than CMAKE_SYSTEM_PREFIX_PATH, to the local installation folder. Because the local installation folder is defined by CMAKE_INSTALL_PREFIX and all libraries are installed together, we ensure the build system can find the latest binaries.

I think these can be achieved by adding a config to SuiteSparsePolicy.cmake. But it is better to set it from top-level cmakelists so users can choose the predefined rules as a whole, or custom building process of individual projects.

_DIR has higher priority than CMAKE_PREFIX_PATH because "If _DIR has been set to a directory not containing a configuration file CMake will ignore it and search from scratch."

Currently, I have no time to create a PR but at least I recorded some todos here. I think the current multiple PRs should be merged first to allow further PR changes.

[mmuetzel]

My idea is to provide a temporal prefix by commands such as ExternalProject_Add to specify the installed package location instead of their build trees and another command can create a dependency between projects so everything is built in order. According to the doc, CMAKE_SYSTEM_PREFIX_PATH contains CMAKE_INSTALL_PREFIX, so if we install all libraries to CMAKE_INSTALL_PREFIX in order, they should be found automatically. At least in this way, we don't need to worry about the paths of source trees.

The build and install steps need to be kept separate. Otherwise, distributors will run into trouble. You can't rely on a mechanism that requires install steps interleaved in the build process or you will cause a lot of headaches.

_DIR has higher priority than CMAKE_PREFIX_PATH because "If _DIR has been set to a directory not containing a configuration file CMake will ignore it and search from scratch."

Please have a look at the documentation for the search order of find_package in config mode:
https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure
Scroll down a bit and check the numbered list (from 1. to 9.).

[chengts95]

You are just using the same building setup as end users if use the installed prefix to find_package.
CMAKE_PREFIX_PATH has 2 versions one is the cached variable other is the environment.
If you think we should not install libraries one by one, then we stick to the old ways. And we have to write another function to install individual cmake projects.

[PMeira]

@DrTimothyAldenDavis, hope you don't mind a quick comment on the CMake support here (didn't seem worth of a new issue). I noticed various files were added as wrappers with #defines that were previously in the Makefile:

https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/a977c4a8575a5f168cf91d1ea8a78f440afb910c/COLAMD/Source/colamd_l.c#LL11-L12

Since I couldn't find anyone commenting about it, it's worth mentioning CMake allows reusing the same file with different constants defined. For example, we use object libraries for our custom build in our "KLUSolve" wrapper.

Maybe there are other reasons for the file approach. If so, sorry for the noise.

(By the way, this wrapper I mentioned will be retired soon and the CMake support in SuiteSparse will help simplify things.)

[DrTimothyAldenDavis]

Thanks for the feedback. Cmake is a complex language and I've tried to keep my usage of it as basic as I can. I wasn't able to figure out how to use cmake to compile a file multiple times. I used to do that in my old Makefile setup, before using cmake. From all the cmake documentation and examples I saw, it seemed to work best with each file being compiled just once. Thus the introduction of files like this one: https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/dev/COLAMD/Source/colamd_l.c

I've gotten this method to work, and it seems robust, so I'll stick with it for now at least.

[PMeira]

@DrTimothyAldenDavis Yeah, it's not really an issue. Indeed CMake is complex and some features are not widely used, hence few examples.
I wouldn't suggest changing that now anyway since it doesn't add much, it's just an incremental clean-up.
Since there is no rush, if you're open to consider it down the line, I can happily at least give the other approach a try someday, and contribute a PR if I'm able to confirm it works in various environments without issues.

[DrTimothyAldenDavis]

That would not work for several reasons: (1) it changes the library structure (-lcolamd includes both the 32-bit integer API and the 64-bit API, but this approach creates multiple libraries), and (2) it would fail for more complex packages such as UMFPACK, which uses a mix of files (some compiled once, some twice, some 4 times).

[DrTimothyAldenDavis]

Most of these related issues are now resolved in the dev2 branch, to appear soon as SuiteSparse v7.2.0.beta, except for the top-level CMakeLists.txt file. That's a useful extension that I'll consider for the next version (say v7.3). So I'll leave this issue open until then.

[mmuetzel]

There is a proposal for a root CMakeLists.txt in #415.
That is still a work in progress.
Feedback is welcome.

[DrTimothyAldenDavis]

I think this is now fixed in SuiteSparse 7.4.0, in the default dev branch of the SuiteSparse repo. I'll release a 7.4.0.beta1 soon.

Can you give it a try?

[DrTimothyAldenDavis]

Thanks for all the feedback. SuiteSparse 7.4.0.beta1 has now been released (as a pre-release), with a top-level CMakeLists.txt file. I think this issue is now resolved, but can you give it a try, @chengts95 ?

[DrTimothyAldenDavis]

Fixed in SuiteSparse 7.4.0.

[chengts95]

I got ninja: error: build.ninja:215: multiple rules generate SuiteSparse_config/suitesparseconfig.lib with clang+ninja on windows. May be caused by shared and static library options.

Also, SuiteSparse_tic and SUITESPARSE_TIME are omp_get_wtime but suitesparse_config link to openmp with private flag, and it caused problems for Mongoose_Test_Reference because it was not linked to openmp libs for static libraries.

[mmuetzel]

Thanks for reporting these issues.
To make it easier to cross-reference issues from commits and PRs, could you please open two separate issues for the problems you found?

[chengts95]

Thanks for reporting these issues. To make it easier to cross-reference issues from commits and PRs, could you please open two separate issues for the problems you found?
I am not very interested in solving building problems now.

I have figured out the causes for multiple rules, this is because on Windows, clang can be a MSVC front-end so it uses windows SDK and standard libraries. It also generates the same MSVC libraries. Basically, on Windows static libraries are always seperated from dynamic libraries names because you need xxx.lib for dlls and xxx_static.lib for static libraries. Also, because of using MSVC libraries, the clang on windows will use complex.h for C language.

[mmuetzel]

That is what I was thinking, too.
Could you please open a new issue? You don't have to find a solution yourself if you aren't interested. But running some tests would probably help...

[chengts95]

I can do it when I have time, and also I may check how to compile it on Mac OS with an M2 arm CPU. It seems that clang for windows has a different link flag for OpenMP, and the link to OpenMP::OpenMP_C didn't add -fopenmp to the linker flag.

[mmuetzel]

@chengts95: I opened #749 and #750 to keep track of the issues you reported. (Took about 2 minutes. Not sure why you couldn't do that yourself...)