Use CMake to build libraries and project #23
Conversation
There was a problem hiding this comment.
Thanks, that's already taking us pretty far! I noted a few things mostly related to writing modern CMake
src/CMakeLists.txt
Outdated
| file(GLOB sources *.cpp) | ||
| add_executable(transfil_N ${sources}) | ||
|
|
||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -g") |
There was a problem hiding this comment.
A few comments here:
- Is
-std=c++17required sinceCMAKE_CXX_STANDARDis already specified in the top-levelCMakeLists.txt? - The debug flag doesn't have to be hardcoded as a compiler option here. Debug mode can be set at
cmakeinvocation using-DCMAKE_BUILD_TYPE(docs) - The variable
CMAKE_CXX_FLAGSsets flags globally for all targets. Not a problem in this particular case, but if the project grows, we might be better off setting flags usingtarget_compile_optionsinstead.
There was a problem hiding this comment.
This set(CMAKE_CXX_FLAGS ...) is now totally removed.
src/CMakeLists.txt
Outdated
|
|
||
| find_package(GSL REQUIRED) | ||
| # Include the library directories for linking | ||
| target_include_directories(transfil_N PUBLIC |
There was a problem hiding this comment.
I'd use PRIVATE for clarity. As far as I know, PUBLIC doesn't mean much for an executable target.
| -lstdc++fs \ | ||
| -Wall -O3 -std=c++17 | ||
| cmake . | ||
| cmake --build . |
| $( $(brew --prefix)/bin/gsl-config --libs ) \ | ||
| $( $(brew --prefix)/bin/gsl-config --cflags ) \ | ||
| -Wall -O3 -std=c++17 | ||
| cmake --build . |
There was a problem hiding this comment.
It's usually good practice to use out-of-source builds:
mkdir build && cd build/
cmake --build ..
src/CMakeLists.txt
Outdated
| find_package(GSL REQUIRED) | ||
| # Include the library directories for linking | ||
| target_include_directories(transfil_N PUBLIC | ||
| ${PROJECT_BINARY_DIR} |
There was a problem hiding this comment.
I would encourage you to use an out-of-source build layout, and specify location of private headers using CMAKE_CURRENT_SOURCE_DIR. Using PROJECT_BINARY_DIR like this would break as soon as an other project call is used below the top level one.
src/CMakeLists.txt
Outdated
| # Include the library directories for linking | ||
| target_include_directories(transfil_N PUBLIC | ||
| ${PROJECT_BINARY_DIR} | ||
| ${PROJECT_SOURCE_DIR}/lib/tinyxml |
There was a problem hiding this comment.
The keyword PUBLIC was used in the call the target_link_directories for the tinyxml target. Therefore its LINK_DIRECTORIES property is inhereted by targets that link to it and lib/tinyxml doesn't need to be re-specified here.
src/CMakeLists.txt
Outdated
| @@ -0,0 +1,12 @@ | |||
| file(GLOB sources *.cpp) | |||
There was a problem hiding this comment.
Setting sources using glob patterns is usually discouraged in favor of specifying file names explicitly.
The reason is that, using the glob pattern, the generated build system (e.g. make) won't be able to trigger a (re)generation if files are added or removed, for instance when an older revision of the project is checked out.
See the note at https://cmake.org/cmake/help/latest/command/file.html#filesystem
This is already set in the top-level cmake file.
jeannielynnmoulton
force-pushed
the
jeannie/usecmake
branch
from
e0af9e7 to
92bd1dc
Compare
Using GLOB is against recommend CMake best practices.
jeannielynnmoulton
force-pushed
the
jeannie/usecmake
branch
from
92bd1dc to
7244529
Compare
This changes to using CMake to build. Now instead of compiling with g++ directly, we can use CMake commands. This is better because we don't have to worry about OS dependent commands.
This also moves tinyxml out of the src folder and into lib/tinyxml and links it as a library.