-
Notifications
You must be signed in to change notification settings - Fork 64
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
Adds support for modern CMake #8
base: master
Are you sure you want to change the base?
Adds support for modern CMake #8
Conversation
e5226f5
to
53617d7
Compare
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:
|
I would be happy to split it up. I envision this split then being the following:
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? |
53617d7
to
654451d
Compare
I split the commit into three commits as follows:
I left out incorporation of pkg-petsc changes as well as the renaming of the |
0831859
to
e9f7867
Compare
@@ -9,7 +9,7 @@ | |||
\version \verbatim $Id: m2mnbrs.c 17699 2014-09-27 18:05:31Z karypis $ \endverbatim | |||
*/ | |||
|
|||
#include <GKlib.h> | |||
#include "GKlib.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change? I think the old form was correct. You use quotation marks for relative paths to the current file and angle brackets for files from the include path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The #include
documentation for GCC states:
Both user and system header files are included using the preprocessing directive ‘#include’. It has two variants:
#include <file>
This variant is used for system header files. It searches for a file named file in a standard list of system directories. You can prepend directories to this list with the
-I
option (see Invocation).
#include "file"
This variant is used for header files of your own program. It searches for a file named file first in the directory containing the current file, then in the quote directories and then the same directories used for <file>. You can prepend directories to the list of quote directories with the
-iquote
option.
The Microsoft docs say something similar.
What distinguishes the two syntax forms is that the quote form first checks the directory of the current file, then falls back to the angle bracket form if nothing is found. With this behavior in mind, I had used the quote form to distinguish headers that were part of the project, despite the fact that the behavior was actually falling back to the angle bracket form for any files in the src
and apps
directories. My rationale is that this allows the developer to quickly identify headers distributed with the project versus those provided by the system.
With that in mind, one could argue that in src
and apps
, these should be using angle bracket form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for quoting the specs. I was arguing like you said in the last sentence and I don't see an advantage in changing it. I wouldn't change it, but it probably is a matter of style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with either. Since it is not my project, I will defer to the primary maintainers if they have a preference. I will gladly change it back to using <>
where appropriate.
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! |
fccdb99
to
99ac691
Compare
xref to the conda forge recipe as it could be useful: conda-forge/staged-recipes#12547 |
99ac691
to
d1eba52
Compare
I added a missing |
The primary purpose of this commit is to incorporate modern CMake (>=3.1) best practices. As a result, a reorganization of the directory structure as well as a few minor code cleanups was required. A summary of those changes follows: * Moves all header files into a separate include directory. * The test directory was renamed to apps, to allow for future unit tests to be written and included in a directory named test. * The win32 shims should only be included when compiling in a Windows environment. Upstream checks this by testing the CMake variable `MSVC`. However, this commit uses generator expressions and the more general test of the CMake Platform ID being equal to `Windows`. * All options and other configurable behavior of the CMakeLists file is implemented using generator-expressions. * Makes 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. * Moves feature availability checks from `GKlibSystem.cmake` to the main `CMakeLists.txt`. This is to separate concerns, `CMakeLists.txt` contains core CMake logic, while `GKlibSystem.cmake` contains common compiler options and the like, which are convenient defaults, but may not be appropriate for all user of the build system.
Adds support for testing successful compilation (w/ and w/o apps) on Travis using Linux, Windows, and Apple.
This primary purpose of this commit is to make some minor changes to the code to clean it up a bit. Notable changes are as follows: * The macro `__OPENMP__` is replaced by `_OPENMP`. `__OPENMP__` was an artifact from the old build system, which defined such a macro when the build was configured with OpenMP support. According to the [OpenMP specification](https://www.openmp.org/specifications/) Section [2.2](https://www.openmp.org/spec-html/5.1/openmpse10.html#x38-370002.2) "the `_OPENMP` macro name is defined to have the decimal value *yyyymm* where *yyyy* and *mm* are the year and month designations of the version of the OpenMP API that the implementation supports." As such, it should be the preferred way to check for OpenMP support during compilation. * The preprocessor condition `defined(__WITHPCRE__)` is replaced by `defined(USE_PCRE) && defined(HAVE_PCREPOSIX_H)`. `__WITHPCRE__` was an artifact from the old build system, which was defined during configuration if the build system user requested PCRE support. This functionality has been replaced by the macro name `USE_PCRE` to be consistent with the other feature availability checks done in the core CMake logic and the condition has been extended to not only check for the request (`USE_PCRE`), but also the availability of the necessary header file, namely `pcreposix.h` indicated by the macro name `HAVE_PCREPOSIX_H` being defined during CMake configuration.
d1eba52
to
fd87121
Compare
Hi @jiverson002, I plan to address these again in a form like KarypisLab/METIS#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? |
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
test
directory 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_ID
being equal toWindows
.All options and other configurable behavior of the
CMakeLists.txt
is implemented using generator-expressions.Replaces the following project defined macros:
__OPENMP__
->_OPENMP
WIN32
->_WIN32
with predefined macros for conforming systems.
Adds the file
GKlibSystem.cmake
which includes lots of nice default settings for different things.Along with this, the CMake variableThis file can be included using the CMake variableGKLIB_SYSTEM
is added. This variable, when defined, will point to a CMake script, likeGKlibSystem.cmake
, where compiler options and the like can be specified.CMAKE_PROJECT_GKlib_INCLUDE
at 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-petscThere 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.