Repository navigation
Use proper include path for cholmod.h. - #364
Conversation
|
Hi @stellaraccident, thanks! I'll have a look at your PR tomorrow, in the meantime, please target the fix at the |
* Adds host-math group and includes OpenBLAS and SuiteSparse in it to provide libcholmod (so that we have the option to avoid system deps for these). * Includes upstream patch ROCm/hipSOLVER#364 * Includes upstream patch ROCm/rocminfo#97 Includes workarounds for #82, #83 --------- Co-authored-by: Stella Laurenzo <stellaraccident@gmail.com>
cb286bb to
3466f65
Compare
jmachado-amd
left a comment
jmachado-amd
left a comment
There was a problem hiding this comment.
Hi @stellaraccident, it looks like my week was far busier than anticipated -- apologies for the delay. I see two other changes that are required in order to allow hipSOLVER to find its Suitesparse dependencies in a general setting:
should be updated into:
find_path(CHOLMOD_INCLUDE_DIR
NAMES cholmod.h
PATH_SUFFIXES suitesparse
)should be updated into:
find_path(SUITESPARSE_CONFIG_INCLUDE_DIR
NAMES SuiteSparse_config.h
PATH_SUFFIXES suitesparse
)Thanks again for taking the time to make the PR, and please let me know if this works for you.
cgmb
left a comment
cgmb
left a comment
There was a problem hiding this comment.
If this is wrong, then mea culpa. I'd hoped that Spack would catch this sort of error, but I guess they had a patch for an earlier problem which ended up sidestepping this issue. :(
The bit that confuses me is that the SuiteSparse::CHOLMOD target is actually quite a new thing in the upstream SuiteSparse project. It didn't exist in older versions, so it's weird that there's an expectation that the user will add an include path to their build flags just to use the OS-provided SuiteSparse headers.
|
Yeah, it's kind of a head scratcher if trying to unwind how this got this way and ever worked right. The details on this are context switched out of my head, but I have an easy repro to try changes with a local built SuiteSparse. |
|
@stellaraccident, I've opened a pr with the changes I suggested last week. They work in my own tests, but please let me know if you can try them in your reproducer. |
|
Hello @renjithravindrankannath, we would be very grateful if you could test this PR in spack. Many thanks in advance! |
renjithravindrankannath
left a comment
renjithravindrankannath
left a comment
There was a problem hiding this comment.
Verified the change in Spack mainline. Looks good to me.
jmachado-amd
left a comment
jmachado-amd
left a comment
There was a problem hiding this comment.
This is all that I need approve the PR, at this point I would just move to rebase and merge and start a cherry-pick into 6.4.
@cgmb, do you have anything to add?
cgmb
left a comment
cgmb
left a comment
There was a problem hiding this comment.
I still think the suitesparse behaviour is a bit weird, but I see that with the find module update provided by Julio, this change builds properly with libsuitesparse-dev 1:5.10.1+dfsg-4build1 from Ubuntu 22.04 (which did not include CHOLMODConfig.cmake). Thanks @stellaraccident and @jmachado-amd.
The User Guide for CHOLMOD also includes a sample program that shows the direct inclusion of cholmod.h.
It appears that the `SuiteSparse::CHOLMOD` CMake library advertises its include directory as `$PREFIX/include/suitesparse`, making the proper way to include it `#include <cholmod.h>`. When installed in the system `/usr` prefix, the `suitesparse/cholmod.h` path works by accident. When using a custom built suitesparse at a custom location, this is not so. I believe that this change is the correct way to do it for all configurations, based on how the SuiteSparse library is set up.
hipSOLVER's find module scripts (for suitesparse) were using `find_path` on name `suitesparse/cholmod.h` instead of `cholmod.h`, which allowed hipSOLVER to `#include <suitesparse/cholmod.h>`. This commit updates the aforementioned cmake scripts, to honour the way `Suitesparse::CHOLMOD` advertises its include directories.
c27e02c to
ffb8ccd
Compare
* Use proper include path for cholmod.h. It appears that the `SuiteSparse::CHOLMOD` CMake library advertises its include directory as `$PREFIX/include/suitesparse`, making the proper way to include it `#include <cholmod.h>`. When installed in the system `/usr` prefix, the `suitesparse/cholmod.h` path works by accident. When using a custom built suitesparse at a custom location, this is not so. I believe that this change is the correct way to do it for all configurations, based on how the SuiteSparse library is set up. * Update suitesparse's find module scripts hipSOLVER's find module scripts (for suitesparse) were using `find_path` on name `suitesparse/cholmod.h` instead of `cholmod.h`, which allowed hipSOLVER to `#include <suitesparse/cholmod.h>`. This commit updates the aforementioned cmake scripts, to honour the way `Suitesparse::CHOLMOD` advertises its include directories. --------- Co-authored-by: Julio Machado Silva <161654951+jmachado-amd@users.noreply.github.com> [ROCm/hipSOLVER commit: c35541f]
It appears that the
SuiteSparse::CHOLMODCMake library advertises its include directory as$PREFIX/include/suitesparse, making the proper way to include it#include <cholmod.h>. When installed in the system/usrprefix, thesuitesparse/cholmod.hpath works by accident. When using a custom built suitesparse at a custom location, this is not so.I believe that this change is the correct way to do it for all configurations, based on how the SuiteSparse library is set up.