Move imported CMake targets to SuiteSparse:: namespace
#352
Conversation
mmuetzel
force-pushed
the
cmake
branch
2 times, most recently
from
018def1
to
21cd2b4
Compare
|
Since there was no reaction in more than a week, I'm assuming that renaming those targets is OK. I'm planning to rename the imported targets as follows: I also plan to use the suffix Please, let me know if you prefer to use different target names. |
|
I've been on vacation the past 2 weeks so I'm a bit behind, but the name space changes look good at first glance. |
|
No worries. |
|
Rebased to a current head to resolve conflict. |
DrTimothyAldenDavis
merged commit c3cb273
into
DrTimothyAldenDavis:dev2
|
@mmuetzel : I'm getting an error when trying to build SPQR with CUDA enabled, with this update to dev2 for the SuiteSparse:: namespace. The SPQR build says the following: Is this because the libcholmod_cuda.so file is in CHOLMOD/build/GPU perhaps? This report from the SPQR build also looks strange: The include report is broken. I think this might be a flaw in the use of "TARGET_PROPERTY". I see in most places it is used, the whole thing is encapulated in a double-quoted string. But in GPUQREngine, SPQR/SPQRGPU, and SuiteSparse_GPURuntime, the double qoutes are missing. I fixed the double quotes but I still get errors when (for example) building UMFPACK, which looks for CHOLMOD as an optional dependency. It still reports the error that The library is present: The CHOLMOD build (after building it already) says that the library libcholmod_cuda.a and *so exists: Any ideas? |
|
See the build output (after building once, first), attached. |
I don't know how to use CUDA. But I'll try to look into it.
That's a CMake generator expression. I guess the generator didn't expand that expression yet. Do you see actual errors because of this? If not, this is mostly cosmetic imho. |
|
Could you please upload your generated |
|
Maybe, #357 resolves the build issue with CUDA. |
|
Thanks for taking a look. Here's a folder with all of CHOLMOD/build/*cmake, and also the CHOLMOD/build/GPU/*cmake. I'll try out #357 now. |
This is a first proposal for how to move the imported CMake targets to a
SuiteSparse::namespace.For the time being it only touches the
suitesparseconfigtarget. I'd like to get feedback on this first to avoid more changes later on.Target names in CMake are case sensitive. There are no central guidelines for the letter case of namespaces and target names. But most projects seem to use CamelCase for the namespace and target and underscore
_to separate "variants". E.g., OpenMP uses imported targets with names likeOpenMP::OpenMP_CorOpenMP::OpenMP_Fortran.The target names have been entirely internal until recently (#267). Currently, they are all lower-case. If we'd like to change that, now is the time for that. A version of SuiteSparse with #267 hasn't been released yet and we don't need to worry about backward compatibility yet.
In the current version of this PR, I used CamelCase for the name of the namespace. But I didn't touch the letter-case of the target itself.
Should the targets themselves be renamed to be CamelCase, too? I'd be slightly in favor of that, i.e., changing it to
SuiteSparseConfigandSuiteSparseConfig_static. Even if that means a little more effort now, it might be the "nicer" solution in the long run. But I'd be happy with whatever you prefer.@DrTimothyAldenDavis: What do you think?