2
\$\begingroup\$

I'm prototyping/debugging some C++ code that I may turn into a longtime project. The build system is ready for review, the prototype is not.

The GitHub repository for the code listed below.

Like all my other questions, this is a learning self teaching project.

The project will be planner management similar to the paper based Franklin Covey planner system, which I use personally.

The project will eventually target Windows, and possible MacOS, but for now I am only interested in Linux development. The project will also eventually include a QT user interface, but it isn't even close to ready for that, what it is currently building is test/debug code for 2 of the models that will be used in an MVC or MVVM architecture.

CMake is working fine, and the debug program is working.

I decided to use code I had in another git repository which is what the FetchContent portion of the CMakeLists.txt file is pulling in.

Development Environment

  • Ubuntu 24.04 running on a Lenovo P50
  • CMake 4.0.1
  • C++23
  • Boost 1.88
  • MariaDB subbing for MySQL

The Code

cmake_minimum_required(VERSION 3.31)

project(protoTaskPlanner LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 23)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

set(CMAKE_COMPILE_WARNING_AS_ERROR ON)

if(POLICY CMP0167)
    cmake_policy(SET CMP0167 NEW)
endif()

find_package(Boost 1.87.0 REQUIRED COMPONENTS system charconv)

include(FetchContent)

FetchContent_Declare(
    genericDictionary
    GIT_REPOSITORY https://github.com/pacmaninbw/GenericDictionaryHeaderLibraryGeneric.git
    CMAKE_FIND_PACKAGE_NAME GenericDictionaryHeaderLibraryGeneric
)

FetchContent_MakeAvailable(genericDictionary)

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")

if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/PrivateData/dbadmindata.h")
    add_compile_definitions(USINGPRIVATECONNECTIONDATA=1)
endif()

add_executable(protoTaskPlanner
    main.cpp 
    PTS_DataField.h PTS_DataField.cpp 
    UserModel.h UserModel.cpp TaskModel.h TaskModel.cpp ModelBase.h ModelBase.cpp
    DBInterface.h DBInterface.cpp
)

target_include_directories(protoTaskPlanner PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/build/_deps/genericdictionary-src)

target_link_libraries(protoTaskPlanner  ${Boost_LIBRARIES} ssl crypto)

The code I'm pulling in through FetchContent was reviewed in my GenericDictionary question. The CMakeList.txt file for that was review as well.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$
set(CMAKE_CXX_STANDARD 23)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

You should not be setting the global CMAKE_CXX_STANDARD, it can cause subtle issues. The modern CMake way of doing this is technically figuring out each individual feature you actually require, though libraries/functions from stdlib are gated through version numbers anyway, so normally it's something like:

target_compile_features(protoTaskPlanner PUBLIC cxx_std_23)

You can find a list of features here: https://cmake.org/cmake/help/latest/command/target_compile_features.html

include(FetchContent)

FetchContent_Declare(
    genericDictionary
    GIT_REPOSITORY https://github.com/pacmaninbw/GenericDictionaryHeaderLibraryGeneric.git
    CMAKE_FIND_PACKAGE_NAME GenericDictionaryHeaderLibraryGeneric
)

FetchContent_MakeAvailable(genericDictionary)

You should not be attempting to create your own buildsystem here, you own the repo, you own the project, make your project installable in CMake, so that you can just cmake install your library, and use

find_package(genericDictionary CONFIG REQUIRED)

your own package, which will also allow you to integrate it into VCPKG (or conan) so you can just use a package manager to manage everything including your boost installation. Fetch content causes issues in offline environments as well and makes build replication much more difficult.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")

This can cause issues with third party libraries (unless you specify system libraries in CMake, which only kind of works in some cases). Do this instead:

target_compile_options(protoTaskPlanner PRIVATE -Wall -Wextra -pedantic)

Additionally, as it stands, this will cause warning messages and could fail to compile on platforms with out these compile arguments. So you'll want to use generator expressions to stop this from being an issue:

    target_compile_options(protoTaskPlanner PRIVATE
        $<$<OR:$<CXX_COMPILER_ID:GCC>,$<CXX_COMPILER_ID:Clang>>: -Wall -Wextra -pedantic>
        #roughly equivalent
        $<$<CXX_COMPILER_ID:MSVC>: /Wall>
    )

This does not pass the sniff test.

if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/PrivateData/dbadmindata.h")
    add_compile_definitions(USINGPRIVATECONNECTIONDATA=1)
endif()

Why would, in a repo you control, a header file just seemingly not exist sometimes? This is basically something that should never happen. Is it supposed to be a library? Well then it should be find_package-able and not be conditionally included in the source of your library. Is it generated? Where is the generator commands in your CMake file, something CMake is wholly capable of doing and doesn't need random bash scripts to make happen? Is it a version thing? That needs to be handled by Git, not by CMake.

Regardless, you should be using target compile definitions, not global versions.

target_compile_definitions(protoTaskPlanner PRIVATE USINGPRIVATECONNECTIONDATA=1)

And if this is a legit optional thing (not some code source horror thing going on that should never happen with conditional source files), then you need to use CMake options.

option(PROTO_TASK_PLANNER_USING_PRIVATE_CONNECTION_DATA "your text telling people what this does" OFF)

Also, you should be pseudo namespacing not only your CMake variables, but also all you preprocessor definitions. I can count on my hand the number of times that's screwed things up, but when it did, it was awful. Macros can't be trusted to not be pseudo namespaced, so don't act like they can. Also there's no sensible coding style that warrants a macro name being squished with out any semblance of what each word is, just use YOUR_NAMESPACE_USING_PRIVATE_CONNECTION_DATA instead. Then you can check later.

if(${PROTO_TASK_PLANNER_USING_PRIVATE_CONNECTION_DATA})
    target_compile_definitions(protoTaskPlanner PRIVATE YOUR_NAMESPACE_USING_PRIVATE_CONNECTION_DATA=1)
endif()

There's no reason for this:

add_executable(protoTaskPlanner
    main.cpp 
    PTS_DataField.h PTS_DataField.cpp 
    UserModel.h UserModel.cpp TaskModel.h TaskModel.cpp ModelBase.h ModelBase.cpp
    DBInterface.h DBInterface.cpp
)

to not be neatly organized into something like this:

#note you can also separately add sources using target_sources
add_executable(protoTaskPlanner
    main.cpp 
    PTS_DataField.h 
    PTS_DataField.cpp 
    UserModel.h 
    UserModel.cpp  
    TaskModel.h 
    TaskModel.cpp 
    ModelBase.h 
    ModelBase.cpp
    DBInterface.h 
    DBInterface.cpp
)

This:

target_include_directories(protoTaskPlanner PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/build/_deps/genericdictionary-src)

is extremely sketchy. This again, looks like you're doing some code generation outside of CMake (this file path does not show up in your repo, and doesn't make sense regardless), which is not necessary and leads to all kinds of bad issues. Have custom code that needs to be compiled first, then need to have that code run? Cool, CMake can do that all in the same file and can automatically have those files run based on changes to dependent files, no need to have sketchy build scripts to handle that.

Additionally, why is it public? There's zero reason for an executable to have public depenencies of any sort. You can't link to an executable, it can't be a link-library dependency of another target. So how does it make sense for it to be public?

target_include_directories(protoTaskPlanner  PRIVATE
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/actual/path/to/files/>);

If you want to have generated code, you'll want to attach include directories to a target associated with a cmake side target which can run the command for generating that code. That's outside the scope of this answer, but there's multiple questions about this on Stack Overflow you can check out.

target_link_libraries(protoTaskPlanner  ${Boost_LIBRARIES} ssl crypto)

This will straight up not run. You need to specify PUBLIC/PRIVATE etc... here, also, you should not just do use a targets properties directly like this in target_link_libraries, boost exposes proper targets, use them.

target_link_libraries(protoTaskPlanner PRIVATE Boost::system Boost::charconv ssl crypto)

You also don't appear to actually be using what I assume would be the target associated with genericDictionary anywhere. If you're just using it to somehow get access to SSL and Crypto indirectly, that's horrible. Don't do that. Either actually use the dependency, or pull in the libraries through find_package If you actually need genericDictionary, and you need it's dependencies, target_link_libraries will automatically take care of that for you, and will list it's depenencies as dependencies for your target in the final build.

target_link_libraries(protoTaskPlanner PRIVATE Boost::system Boost::charconv genericDictionary)
\$\endgroup\$
5
  • \$\begingroup\$ There are 2 versions of dbadmindata.h, the one in the repository that contains strings that won't work when you run the program, and the private version. The private version contains the database password and database user name. The program will compile with the public version. If anyone wants to run the program they are going to have to install MySQL and create their own username and password. GenericDictionary is a header only library. Linking is not required. \$\endgroup\$ Commented Jul 9 at 21:22
  • \$\begingroup\$ How does one go about installing a header only library? \$\endgroup\$ Commented Jul 9 at 21:38
  • 1
    \$\begingroup\$ @pacmaninbw For your first comment, that seems insane, you don't need a C++ header to provide private database passwords and user names conditionally, they can be loaded from a separate file entirely. Regardless, fully answering what you should properly do there is a stackoverflow/softwareengineering stackexchange question, I would highly suggest you do that, what you're doing is almost certainly not the right way to do that. (will answer the header only thing in a second). \$\endgroup\$ Commented Jul 9 at 22:14
  • 1
    \$\begingroup\$ @pacmaninbw You're going to have to ask on SO/google for a more comprehensive answer, but Header only libraries are installed basically identically to other libraries. They are used the same way (in target_link_libraries) have include directories, etc.... What you may find is that certain parts of creating a CMake file with proper install interface support may not be necessary, The big difference is that header only libraries are created as interface libraries, not shared/static libraries. You should also familiarize yourself with filesets. \$\endgroup\$ Commented Jul 9 at 22:21
  • 1
    \$\begingroup\$ @pacmaninbw see: cmake.org/cmake/help/latest/command/… and cmake.org/cmake/help/latest/command/… I have dozens of custom header only libraries that I even have interacting with VCPKG, I'm able to find_package(my_header_lib CONFIG REQUIRED) and can target_link_libraries(my_target PUBLIC my_header_lib::my_header_lib) fine. \$\endgroup\$ Commented Jul 9 at 22:22

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.