Updating SuiteSparse to 5.8.1 #7125

AnHeuermann · 2021-02-02T14:03:44Z

I'm trying to update SuiteSparse to version 5.8.1.

But as far as I can tell we don't have any changes in SuiteSparse (?), so do we need to have the sources in 3rdParty. On Linux we can probably just link to umfpack and klu (and with amd, colamd, btf).
But probably I'll need to change stuff for Windows MINGW and MSVC, as always.

Changes:

The UMFPACK_TARGET is now the SUITESPARSE_TARGET
- Changed configure to use SUITESPARSE instead of UMFPACK
Adding -lsuitesparseconfig to lpsolve linker flags during configure call
Build libsuitesparseconfig and adding linker flag for it to all libs using SuiteSparse in some form.
Installing all header files of SuiteSparse to /build/include/omc/c/suitesparse (there was an additional /build/include/omc/c/suitesparse/Include I removed).

TODO

Related ticket

trac ticket 6362

sjoelund · 2021-02-02T14:54:59Z

But as far as I can tell we don't have any changes in SuiteSparse (?), so do we need to have the sources in 3rdParty. On Linux we can probably just link to umfpack and klu (and with amd, colamd, btf).

Yes, because only SuitesParse 4 is available in el7 and it would get different results making debugging different platforms really annoying...

Also, if we ever include these solvers in source-code FMUs, we need the source code available

- Updating 3rdParty. - The `UMFPACK_TARGET` is now the `SUITESPARSE_TARGET`. - Changed configure to use `SUITESPARSE` instead of `UMFPACK`. - Adding `-lsuitesparseconfig` to lpsolve linker flags during configure call. - Build `libsuitesparseconfig` and adding linker flag for it to all libs using SuiteSparse in some form. - Installing all header files of SuiteSparse to `/build/include/omc/c/suitesparse`. - There was an additional `/build/include/omc/c/suitesparse/Include` which is now removed.

mahge · 2021-02-09T11:09:15Z

OMCompiler/Compiler/runtime/CMakeLists.txt

@@ -245,7 +245,7 @@ else()
  set(CONFIG_WITH_OPENMP 0)
 endif()

-set(WITH_UMFPACK "#define WITH_UMFPACK")
+set(WITH_SUITESPARSE "#define WITH_SUITESPARSE")


Just FYI, these config related defines are not complete for the new CMake config. I added them to get a quick working compilation. We need to update the autoconf defines with proper names and consistent #define style.
set(...) to a whole "#define ..." is probably not the way to go. Both CMake and autoconf have builtin support for handling these "availability" defines.
I will try to finish it up but maybe you can take over as well. Someone needs to be responsible for it :)

mahge · 2021-02-09T11:12:51Z

OMCompiler/Makefile.omdev.mingw

@@ -501,7 +499,7 @@ runtimeCPPmsvcinstall: getMSVCversion
 	# Force make to do suitesparse and sundials in the correct order and wait for umfpack to be finished before starting sundials
 	# This is a terrible idea
 	rm -rf $(OMBUILDDIR)/lib/omc/msvc/umfpack.lib
-	$(MAKE) -f $(defaultMakefileTarget) OMBUILDDIR=$(OMBUILDDIR) Umfpack_msvc
+	$(MAKE) -f $(defaultMakefileTarget) OMBUILDDIR=$(OMBUILDDIR) suitesparse_msvc


I forgot the discussion last time but was anyone still using the msvc targets? If not they should be removed. Otherwise it is just extra work every time.

I think the Cpp guys are using it and they say it is significantly faster on Windows. But I'm not so sure now that I remember that correctly.

Same. I think at some point in the meetings no one replied to the question who is using these exact targets.

mahge

LGTM. Just minor things that can go in another PR. See comments.

mahge · 2021-02-09T11:16:35Z

OMCompiler/SimulationRuntime/OMSICpp/CMake/FindSuiteSparse.cmake

@@ -13,10 +13,10 @@ SET(SUITESPARSE_UMFPACK_FOUND false)
 SET(SUITESPARSE_UMFPACK_USE_OMC false)

 SET(SUITESPARSE_UMFPACK_INCLUDE_DIR_OMC "")
-MESSAGE(STATUS "Searching for umfpack.h of OpenModelica in ${CMAKE_INSTALL_PREFIX}/include/omc/c/suitesparse/Include")
-FIND_FILE(UMFPACK_H_OMC umfpack.h HINTS "${CMAKE_INSTALL_PREFIX}/include/omc/c/suitesparse/Include" NO_DEFAULT_PATH)
+MESSAGE(STATUS "Searching for umfpack.h of OpenModelica in ${CMAKE_INSTALL_PREFIX}/include/omc/c/suitesparse")


Why do the include files for suitesparse go in omc/c btw? They belong where the other 3rdParty includes are. This together with the now-fixed Include (capital I) directory was confusing.

It is cleaner and expected to have the install folder structure to match the source code folder structure if possible.

This is also (maybe) why we have problems for dependents of suitesparse. Include files are included from the install/build folder instead of the 3rdParty source folder during build. These means suitesparse needs to be built and installed before dependencies (which only need the headers) can be built.

Why do the include files for suitesparse go in omc/c btw?

This is indeed a good question. I left it as it was but I think all 3rdParty includes should be independent of the C or Cpp runtimes. I can change that in a new PR or we do it when we do some more CMake magic for 3rdParty.

Yes. Please change it if you can. I was going to change it but had to deal with the other CMake stuff first.

Includes should be fixed everywhere to be the form #include suitesparse/... and during build of omc we should specify 3rdParty/ folder as include directory -I .../3rdParty. And for simulation we will use build/install directory's include/ folder as an include directory -I .../include/

Some 3rdParty things are part of the source code FMUs. Not all, but some. I think suitesparse isn't needed though.

At the moment not, but that might change soon. We already have CVODE in FMUs. Using klu in there wouldn't be much of a change.
I'll open a new PR to change linking with BLAS/LAPACK on Windows and an additional one to change the include directories.

Should this PR be merged as it is now?

From my side you can merge it. This was just a suggestion to improve later. You can leave it for now since things work. We can plan on it later together with the other CMake changes.

casella · 2021-02-09T21:29:16Z

@AnHeuermann, unfortunately it seems that your PR broke all the RPM-based builds in linux, see report. @sjoelund, do you have any suggestion how to fix this?

sjoelund · 2021-02-10T05:56:51Z

I think simply adding an ignore for this so since we provide it. It's a new library, right? The others should probably already be listed in there

casella · 2021-02-10T08:47:14Z

Just started a new build, let's see if this works out.

But we should try to get it going, the RPM releases are strategic for us, particularly in the power sector where companies are often bound to RedHat installations.

AnHeuermann added CI/Build MINGW Build pull request with Windows MSYS2 MINGW64 CI/Build CentOS labels Feb 2, 2021

AnHeuermann self-assigned this Feb 2, 2021

AnHeuermann marked this pull request as draft February 2, 2021 14:05

AnHeuermann removed CI/Build CentOS CI/Build MINGW Build pull request with Windows MSYS2 MINGW64 labels Feb 2, 2021

AnHeuermann added CI/Build CentOS CI/Build MINGW Build pull request with Windows MSYS2 MINGW64 labels Feb 5, 2021

AnHeuermann force-pushed the updateSuiteSparse branch from da43379 to 7677bdd Compare February 5, 2021 15:22

AnHeuermann marked this pull request as ready for review February 5, 2021 15:23

AnHeuermann force-pushed the updateSuiteSparse branch from 7677bdd to fe308a7 Compare February 5, 2021 15:25

AnHeuermann force-pushed the updateSuiteSparse branch from fe308a7 to bc86faa Compare February 8, 2021 12:09

AnHeuermann requested a review from mahge February 8, 2021 12:19

mahge reviewed Feb 9, 2021

View reviewed changes

mahge approved these changes Feb 9, 2021

View reviewed changes

AnHeuermann merged commit efa7994 into OpenModelica:master Feb 9, 2021

AnHeuermann deleted the updateSuiteSparse branch February 9, 2021 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating SuiteSparse to 5.8.1 #7125

Updating SuiteSparse to 5.8.1 #7125

AnHeuermann commented Feb 2, 2021 •

edited

sjoelund commented Feb 2, 2021 •

edited

mahge Feb 9, 2021

mahge Feb 9, 2021

AnHeuermann Feb 9, 2021

mahge Feb 9, 2021

mahge left a comment

mahge Feb 9, 2021

mahge Feb 9, 2021 •

edited

AnHeuermann Feb 9, 2021

mahge Feb 9, 2021 •

edited

sjoelund Feb 9, 2021

AnHeuermann Feb 9, 2021

mahge Feb 9, 2021

casella commented Feb 9, 2021

sjoelund commented Feb 10, 2021

casella commented Feb 10, 2021

Updating SuiteSparse to 5.8.1 #7125

Updating SuiteSparse to 5.8.1 #7125

Conversation

AnHeuermann commented Feb 2, 2021 • edited

Changes:

TODO

Related ticket

sjoelund commented Feb 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahge Feb 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahge Feb 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casella commented Feb 9, 2021

sjoelund commented Feb 10, 2021

casella commented Feb 10, 2021

AnHeuermann commented Feb 2, 2021 •

edited

sjoelund commented Feb 2, 2021 •

edited

mahge Feb 9, 2021 •

edited

mahge Feb 9, 2021 •

edited