GKlib
GKlib copied to clipboard
Adds support for modern CMake
The primary purpose of this PR is to incorporate modern CMake (>=3.1) best practices into GKlib. As a result, a reorganization of the directory structure as well as a few minor code cleanups were required. A summary of those changes follows:
-
Moves all header files into a separate include directory.
-
The
testdirectory was renamed toapps, to allow for future unit tests to be written and included in a directory namedtest. -
Makes building of the GKlib apps disabled by default if the GKlib library is not the top CMake project and enabled if it is. This way, when GKlib is a dependency of another project, the apps are not build by default. An option,
GKLIB_BUILD_APPS, was added to allow projects depending on GKlib to also build the apps. -
The win32 shims should only be included when compiling in a Windows environment. Upstream checks this by testing the CMake variable
MSVC. However, this PR uses generator expressions and the more general test of the CMakePLATFORM_IDbeing equal toWindows. -
All options and other configurable behavior of the
CMakeLists.txtis implemented using generator-expressions. -
Replaces the following project defined macros:
__OPENMP__->_OPENMP- ~~
WIN32->_WIN32~~
with predefined macros for conforming systems.
-
Adds the file
GKlibSystem.cmakewhich includes lots of nice default settings for different things. ~~Along with this, the CMake variableGKLIB_SYSTEMis added. This variable, when defined, will point to a CMake script, likeGKlibSystem.cmake, where compiler options and the like can be specified.~~ This file can be included using the CMake variableCMAKE_PROJECT_GKlib_INCLUDEat configure time. This allows the user of the build system to have a file where they manage common CMake settings without affecting the core CMake logic and thus imposing those settings on other users of this build. -
Adds support for testing successful compilation (w/ and w/o apps) on Travis using Linux, Windows, and Apple
-
~~Incorporates changes from pkg-petsc~~
There are two sample projects here and here, which show how GKlib can be incorporated into a project using more modern CMake features. The add_subdirectory() will still work as well, but these examples highlight some of the convenience of modern C/C++ tooling. The first example uses only CMake, while the second uses the C/C++ package manage Conan.
I love this merge request and would like to see most of to get merged. But I find it difficult to review all changes, name the controversial ones from the obvious ones, and in the Git history it will be not obvious, why a line was modified.
Would you mind to split your commit in at least a dozen smaller ones?
Two notes I could already figure out:
- Why having names _WIN32 and _OPENMP. Usually you have the prefix of the project name to have unique variable names. Like
GKlib_WIN32andGKlib_OPENMP. - You increased the required CMake version from 2.8 to 3.1. I am not against it, but is has to be clear, that this will cut of some users. I think it is ok, CMake 3.1 is from December 2014.
I would be happy to split it up. I envision this split then being the following:
- CMake changes - this includes bump to CMake 3.1 and reorganization of the directory structure.
- Travis changes - this adds support for test compiling on Travis platform - should have been separate from the beginning since this may not be necessary with GitHub Actions, although I am less familiar with that.
- pkg-petsc changes.
- Other changes to files, like renaming of macros, changes from <> to "" for includes, etc.
However that is only four indivisible units. My guess is that you would like to see (1) above broken up further, since the others are quite small and self-contained. In this case, I am not sure how much I can break that down, since many of those changes are interdependent. If I really tried, I might be able to extract changes that apply to the feature availability checks. I could also break (4) into separate commits, one for each change.
Can you elaborate on what you have in mind?
I split the commit into three commits as follows:
- CMake upgrade
- Travis support
- Code cleanup
I left out incorporation of pkg-petsc changes as well as the renaming of the WIN32 macro. After looking into the WIN32 macro more, it appears that there could be more improvements than the simple ones that I had originally made in terms of handling the Windows platform; enough that they may warrant their own pull-request.
I pretty much like your changes. Especially the move in the readme from make, make config etc. to proper CMake. Fingers crossed, that Mr. Karypis will merge it!
xref to the conda forge recipe as it could be useful: https://github.com/conda-forge/staged-recipes/pull/12547
I added a missing RUNTIME clause in the install command in the main CMakeLists.txt.
Hi @jiverson002, I plan to address these again in a form like https://github.com/KarypisLab/METIS/pull/79. I plan to do more structural changes first to make the projects importable one in another. Could you then help address the other issues like the naming conventions on top of that?