-
Notifications
You must be signed in to change notification settings - Fork 56
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
add support for the fmt library #185
Comments
Do you know why you are using cmake 3.10 as minimum... With 3.11 we could have 'https://cmake.org/cmake/help/latest/module/FetchContent.html'... It would allow to supress doctest from the repo and only download it at configure time With cmake 3.14 we would have an easier syntax for it and we would have the possibility to use https://github.com/cpm-cmake/CPM.cmake The benefits I see to use at least FetchContent is that you can easily change the version of your dependencies and make some checks... Then by just a change of variable change the version of the dependency... And the livrary is not directly included on the repo... Doctest is wonderful but you are using a old version on cpp_terminal and moving need to change all the doctest.h With fetchcontent it should be possible to just change a 'set(USE_DOCTEST_VERSION "...")' The downloading of the dependecies could even be skip if the user have the dependencies already installed on his computer. Personally I love FetchContent and thank CMake people to have included this features 🙏 into CMake Look the magic : cmake_minimum_required(VERSION 3.14 FATAL_ERROR)
project(CPMExampleDoctest)
# ---- Dependencies ----
include(../../cmake/CPM.cmake)
CPMAddPackage("gh:cpm-cmake/[email protected]")
CPMAddPackage("gh:onqtam/doctest#2.4.5")
# ---- Create binary ----
add_executable(CPMExampleDoctest main.cpp)
target_link_libraries(CPMExampleDoctest fibonacci doctest)
target_compile_features(CPMExampleDoctest PRIVATE cxx_std_17)
# ---- Enable testing ----
enable_testing()
add_test(CPMExampleDoctest CPMExampleDoctest) |
I wouldn't include fmt directly into cpp-terminal, just add support so people can add it themselves to their project. The thing with dependencies is that it has to be maintained and managed. Also some users might not want to use fmt for various reasons. There is no exact reason why the cmake version is so low, but out of my personal experience I know that high versions will break the compilation on some systems like Debian, Ubuntu, all those distros with way to old software. I'd like to move from doctest to google test as I have a lot of experience with it and really like it's syntax. For that I'll probably use git sub modules that are just fetches from cmake. While It's also a good practice to disable the unit testing dependencies when the project is used in another project and to use the locally installed version over the bundled (important for packaging, in this case it speeds up compile time). |
But the cmake needs refactoring anyway |
Cmake 3.10 is for ubuntu 18 (2018). In ubuntu 20 (2020) it's cmake 3.16. Yes, I know debian/Ubuntu 🙄😔... |
Yeah, we can implement it only for a specific cmake version though. Otherwise as long as the CI is passing it should be fine. |
We should add support for the fmt library since it advertises a much better performance than iostream.
https://github.com/fmtlib/fmt
The text was updated successfully, but these errors were encountered: