-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CHOLMOD: Add dependency to system libraries after SuiteSparse libraries #555
CHOLMOD: Add dependency to system libraries after SuiteSparse libraries #555
Conversation
This might help with the search order of include and library directories. Use OpenMP CMake target instead of library and include folder names. Maybe helps with the issue described in DrTimothyAldenDavis#175.
6840af6
into
DrTimothyAldenDavis:dev2
I've also added #error messages if a stale header is found, so that the compile-time errors (if any) will be more obvious. See my next push to dev2, coming shortly. |
If this change fixes CHOLMOD, it would need to be done in all packages that use OpenMP, right? That would be GraphBLAS, LAGraph, ParU, SPQR, and UMFPACK. SuiteSparse_config also brings in OpenMP. |
@mmuetzel Did I understand you right, that I shall pull dev2 and do cmake --fresh? I did like this
I get the new error message but the problem persists.
|
Honestly I wonder why brew brings in the headers. Does brew build locally? I thought binaries are pulled and AAIK I don't have any dev variants tools to compile against. |
Ok. That's progress. At least the new #error check is catching the issue. Here's the problem that remains: ... Note that the homebrew include comes before the AMD include. So it gets the stale amd.h in /opt/himebrew/include and not the latest amd.h in Users/fwein/code/SuiteSparse-org/AMD/Include My guess is we need the same fix to umfpack as @mmuetzel made to cholmod, to bring in OpenMP after AMD. Same for SPQR, ParU, etc. |
This is the list of what had be built:
|
Correct. Theoretically, it's not only OpenMP that can be problematic. The same could happen for BLAS, LAPACK, MPFR, or any other dependency. |
CHOLMOD uses the BLAS, LAPACK, OpenMP, and CUDA, and your change to CHOLMOD worked for @Fabian188 because the most recent failure was in UMFPACK not getting the proper amd.h. So CHOLMOD must have worked (@Fabian188 : can you confirm)? If it worked, it found the correct amd.h, and not the stale one in /opt/homebrew/include. I tried homebrew on my Mac (intel-based) and this issue didn't occur. I might be using a different OpenMP library that isn't in homebrew, perhaps. I installed suite-sparse from homebrew, too, just to see what would happen. It has an older version (SuiteSparse 7.2) but it didn't put the include files in /opt/homebrew/include. I'll try it on my MacBook Air; maybe that will trigger this issue. |
Got it! I can replicate the issue. I installed with "brew install suite-sparse" on my MacBook Air, and it placed a slightly stale copy of all of SuiteSparse include files in /opt/homebrew/include. Then I did "make" at the top-level of SuiteSparse (not using the root CMakeLists.txt). CHOLMOD builds fine, so it must be finding the correct amd.h (version 3.3.0). The /opt/homebrew/include/amd.h is 3.0.4. But then KLU failed. It reports the following:
In the script above, /usr/local/include comes before /Users/davis/dev2/SuiteSparse/BTF/Include.
Either btf.h file above needs to be ignored by cmake, of course. I deleted the /usr/local/include/btf.h file and KLU built just fine. Interestingly, it was KLU_CHOLMOD_static that failed. KLU itself built just fine. If I touch Source/klu.c and rebuild with verbose, klu.c builds fine and doesn't use -I/usr/local/include:
If I put a garbage /usr/local/include/btf.h with a syntax error in it, and do a clean build, klu.o builds just fine. klu_cholmod.c chokes on the garbage /usr/local/include/btf.h. Building the klu_cholmod.c for the shared library works just fine. It's only the STATIC klu_cholmod build that fails by tripping over the garbage btf.h. |
So it's not homebrew that is the problem, I think, because /usr/local/include/btf.h wasn't coming from brew. |
Continuing further, it's the both the dynamic and static build of UMFPACK that fails, because it finds the wrong SuiteSparse_config and the wrong amd.h:
Note the order of include directories in the command above that fails:
I'm using the built-in Apple Clang 15. I'm using SuiteSparse/UMFPACK/CMakeLists.txt directly, not the root CMakeLists.txt. Removing /usr/local/include/amd.h and /usr/local/include/SuiteSparse_config.h allows UMFPACK to build successfully. |
Then the ParU build fails because gets the stale /usr/local/include/umfpack.h. It would also fail because it would link in the stale amd.h, colamd.h, etc.
|
Correction, ParU doesn't #include amd.h, colamd.h, etc directly. Once the stale umfpack.h is removed, ParU builds fine. Then SPQR fails because it gets the stale CHOLMOD, etc. So I can replicate the problem on my side, and I can test against the #include'ing of stale files in my /usr/local/include folder. It's not quite the same error that @Fabian188 is seeing, where /opt/homebrew/include causes the problem. But the theme is the same. I can force all /usr/local/include/*.h files from a stale SuiteSparse to be present, and then watch which new build of SuiteSparse fails. |
I assume you have an older Intel MacBook air? If you switch, you'll compile significantly faster (4 times for my software) and your errors will be from /opt/homebrew, too :) |
It's a M1 Air. The brew I used to install suite-sparse was the arm-based brew, and I'm building a native arm. The gcc I'm using is not really gcc but:
I'm not sure why I'm seeing the errors from the /usr/local/include folder and not /opt/homebrew/include. But the principle is the same, at least. |
Oh, here's a twist. SPEX uses gmp and mpfr from homebrew, and I get conflicts with /opt/homebrew/include, not from /usr/local/include:
Note the bad order in the command above:
Sorry for all the github spam :-). |
Documentation is a bit sparse on this topic (or I didn't find the correct places). But what I gather from CMake's sources is that the library dependencies (i.e., the I'll try to prepare a changeset for that some time later today or tomorrow. But I can't reproduce the issue here. It would be nice if you could check if that actually helps. |
Changes to the CMake build rules for KLU, ParU, SPQR, and UMFPACK that are similar to DrTimothyAldenDavis#555.
The problem comes from both dynamic and static cases. I'm seeing the same kind of errors with stale headers when building the Example library on my MacBook Air, after I did
The Example library prints out the version numbers it gets from the headers, and (in most cases) also calls the library itself to get the version number via a function call. It prints out both ... and they don't match. The headers are stale but the libraries are the most recent ones. For example, for CHOLMOD, both my_demo and my_demo_static report:
This was after a LOCAL_INSTALL option, so all the include files and libraries are found in SuiteSparse/include and SuiteSparse/lib. I will try a "make ; sudo make install" of suiteSparse and then try the Example to see if that makes a difference. |
Changes to the CMake build rules for KLU, ParU, SPEX, SPQR, and UMFPACK that are similar to DrTimothyAldenDavis#555.
Changes to the CMake build rules for KLU, ParU, SPEX, SPQR, and UMFPACK that are similar to DrTimothyAldenDavis#555.
Changes to the CMake build rules for KLU, ParU, SPEX, SPQR, and UMFPACK that are similar to DrTimothyAldenDavis#555.
The headers of OpenMP might be installed at a location where headers of older versions of SuiteSparse libraries are installed.
Try two approaches to avoid that these older headers are picked up when building CHOLMOD:
Maybe that helps with the issue described in #175.
@Fabian188: Could you please test this if you come around to it?