Skip to content
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

These are some improvements to SuperLU to try to make it more cross p… #25

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JPeterMugaas
Copy link
Contributor

…latform and facilitate packaging.

These improvements are:

  1. Force the built in blas library to be compiled as static instead of shared. This is so it can linked into the shared library in Windows and probably other Operating Systems. The internal cblas library can NOT be built as a a shared library by itself.
  2. Remove the assumption that the SuperLU library shared run-time library will be prefixed with lib and have a “.so” or “.a” file extension. In Windows, the shared run-time library has a .dll extension. In Windows Visual C, a prefix such as “lib” is not used and the extension is “.lib”, not “.a” In Cygwin and MSYS2, the run-time .dll prefix is not “lib” at all. It is “cyg” and “msys-“ respectively. Making this more complicated is that in Windows, you do not link directly with the shared-library run-time file but rather, an import library. In MINGW-W64, CygWin, and MSYS, that has the extension, “.dll.a”.
  3. Another issue is that in Windows 64, unlike many Unix-like Operating Systems, the int, uint, and long do not expand to 8 bits in the 64-bit version of Windows, Those types are still 4 bytes wide which is not wide enough for an 8-byte sized pointer. To fix this, I had to change int to ptrdiff_t in ExpHeader and LU_stack_t in slu_util.h. For sizes, the size_t really should be used and for pointers, a pointer should be used. GCC will rightfully warn you about this issue.
  4. I implemented building doxygen documentation into the cmake build process and Cmake can also install the documentation. You now have the option of building the SuperLU documentation as .HTML or the traditional man pages as well as both depending upon packaging needs.
  5. I tried fixing some doxygen warnings. I use Doxygen 1.8.14 along with Grapviz to generate diagrams. I made a Doxyfile.in that generates a Doxyfile in the build directory if enable_docs is on. I also use Doxygen to create to .HTML and Unix man documentation.
  6. The make.inc file generated by cmake is copied to the build directories. The thing is that sometimes, you might be building several different sets of binaries for i686 (32-bit) and (x86_64) 64-bit as well as a static and shared version for inclusion in packages. IOW, I am generating 4 sets of SuperLU binary files. Actually, I think the Makefile build system is not adequate for building stuff on Windows using GCC because you use a command such as “g++ -shared -o example_dll.dll example_dll.o -Wl,--out-implib,libexample_dll.a”
  7. In Windows, it’s usually a good idea to embed version information right into the .dll run-time so that if you right click it, you can get information such as the copyright and .DLL version in Windows explorer.
  8. I had to fix the library install so that the .dll file is always installed in the bin directory instead of the lib directory. Windows will search first for .dll’s in the same directory as the executable. MINGW, Cygwin, and MSYS packages also place the .DLL’s in the appropriate directory to take advantage of this.
  9. When building the interal CBLAS library, Cmake will check for the f2c package and fail if that is not located.

…latform and facilitate packaging.

These improvements are:
1) Force the built in blas library to be compiled as static instead of shared.  This is so it can linked into the shared library in Windows and probably other Operating Systems.  The internal cblas library can NOT be built as a a shared library by itself.
2) Remove the assumption that the SuperLU library shared run-time library will be prefixed with lib and have a “.so” or “.a” file extension.  In Windows, the shared run-time library has a .dll extension.  In Windows Visual C, a prefix such as “lib” is not used and the extension is “.lib”, not “.a”  In Cygwin and MSYS2, the run-time .dll prefix is not “lib” at all.  It is “cyg” and “msys-“  respectively.  Making this more complicated is that in Windows, you do not link directly with the shared-library run-time file but rather, an import library.  In MINGW-W64, CygWin, and MSYS, that has the extension, “.dll.a”.
3) Another issue is that in Windows 64, unlike many Unix-like Operating Systems, the int, uint, and long do not expand to 8 bits in the 64-bit version of Windows,  Those  types are still 4 bytes wide which is not wide enough for an 8-byte sized pointer.  To fix this, I had to change int to ptrdiff_t in ExpHeader and LU_stack_t in slu_util.h.  For sizes, the size_t really should be used and for pointers, a pointer should be used.   GCC will rightfully warn you about this issue.
4) I implemented building doxygen documentation into the cmake build process and Cmake can also install the documentation.  You now have the option of building the SuperLU documentation as  .HTML or the traditional man pages as well as both depending upon packaging needs.
5) I tried fixing some doxygen warnings.  I use Doxygen 1.8.14 along with Grapviz to generate diagrams.  I made a Doxyfile.in that generates a Doxyfile in the build directory if enable_docs is on.  I also use Doxygen to create to .HTML and Unix man documentation.
6) The make.inc file generated by cmake is copied to the build directories.  The thing is that sometimes, you might be building several different sets of binaries for i686 (32-bit) and (x86_64) 64-bit as well as a static and shared version for inclusion in packages.  IOW, I am generating 4 sets of SuperLU binary files.  Actually, I think the Makefile build system is not adequate for building stuff on Windows using GCC because you use a command such as “g++ -shared -o example_dll.dll example_dll.o -Wl,--out-implib,libexample_dll.a”
7) In Windows, it’s usually a good idea to embed version information right into the .dll run-time so that if you right click it, you can get information such as the copyright and .DLL version in Windows explorer.
8) I had to fix the library install so that the .dll file is always installed in the bin directory instead of the lib directory.  Windows will search first for .dll’s in the same directory as the executable.  MINGW, Cygwin, and MSYS packages also place the .DLL’s in the appropriate directory to take advantage of this.
9) When building the interal CBLAS library, Cmake will check for the f2c package and fail if that is not located.
Remove tab characters from comment sections  so that they will effect the HTML documentation.
…mplain. I also hope that this would also be helpful for others to make it workable. I don't have MATLAB from Mathworks.
Fix doxygen errors.  It turns out that "-" at the start of a line terminates a text-block.  < and > need to be escaped.  A few other comment blocks needed minor editting to work with Doxygen 1.8.14 properly
Added "getopt.h" to some i.c files to take advantage of MINGW64 getopt support.
Added external function protypes to C examples and stuff in the FORTRAN directory to avoid implicit dunctions from being defined for some things.  That can have unintentional consequences.
ftp in examples has to be defined to the EXACT size of pointers.  Thus, the pointers in 32-bit Windows MUST be 32-bits wide and 64-bits in Win64.  This also would apply in Linux.  The old code assumed that the pointer-type cast always 64-bits wide.  GCC warns about this stuff for good reason.\
The main function in programs must be defined with an "int" return type.
This removes most code warnings.  There still are Doxygen warnings about undocumented stuff.
… package installs. Integate a FreeBSD patch to put the includes into a superlu subdir for consistancy with Linux distributions that do this as well such as Archlinux and Debian. See: https://svnweb.freebsd.org/ports/head/math/superlu/files/patch-SRC_CMakeLists.txt?revision=423876&view=markup for the original patch.
@gruenich
Copy link
Contributor

  • I like your changes and I would like to see them accepted and integrated.
  • You are addressing a couple of important, but different things. Every bullet point above should be a merge request on its own. That make the author of SuperLU easier to review, test, and integrate. And it also make easier to reject an idea from you. Take for example b466c22: Not everybody cares about line endings. I would accept your commit, but it changes a lot of files and line and makes using git blame harder. So I would understand if it is not accepted.

Some comments regarding your individual commits:

  • 3f2dbef: You have nine different point within your commit message. That is a clear sign you should split this commit into at least nine commits, probably even more. And make nine merge request out of them.
  • b466c22: Spelling mistakes in the first line of commit message. Again, MR of its own.
  • 0cb29b9: Including untested code isn't a good idea. Maybe drop it altogether.
  • 47595: Again, you are mixing a couple of different things. Split them into separate commits and merge requests. Having doxygen warnings fix, will be accepted in no-time!

Disclaimer: I am just a random guy in the internet. My opinion might differ from the author of SuperLU. But I like the changes and want to give my advice to get them included. If you feel unsure, please ask for more details. I am willing to help you getting things included and splitting your commits.

eschnett pushed a commit to eschnett/superlu that referenced this pull request Oct 13, 2022
support building shared and static libraries simultaneously
gruenich pushed a commit to gruenich/superlu that referenced this pull request Apr 11, 2023
gruenich pushed a commit to gruenich/superlu that referenced this pull request Apr 11, 2023
@gruenich gruenich mentioned this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants