Modernize and clean up CMakeLists.txt for streets_service_base
#237
Conversation
For repeatability, dependency versions should be explicitly listed. Exact dependency versions are listed now, but they could be relaxed in the future to allow for compatible major versions. However, this assumes dependency developers strictly adhere to semver conventions. The spdlog package, for example, had API breaking changes between versions 1.5.0 and 1.10.0. streets_utils/streets_service_base/CMakeLists.txt * add version for Boost * add version for spdlog * add RapidJSON as required package since project links to it * add version for RapidJSON
The streets_service_base package requires code from the RapidJSON and spdlog packages that are not available in the Ubuntu 20.04 repositories. For RapidJSON, the streets_service_base package requires code not available in any release version. Therefore, both of these dependencies are vendored through CMake's FetchContent feature and built from source as subprojects. Additonally, the GTest dependency is explicitly added and linked through the provided import target. streets_utils/streets_service_base/CMakeLists.txt * explicitly add GTest dependency * include dependencies.cmake file * link to GTest::GTest streets_utils/streets_service_base/dependencies.cmake * add RapidJSON repo version locked to a commit hash * add spdlog repo version locked to latest release
The previous version used ${PROJECT_NAME} all over the project's
CMakeLists.txt file, which made reading and understanding challenging.
Craig Scott in his Professional CMake book discourages using
${PROJECT_NAME} for this very reason.
streets_utils/streets_service_base/CMakeLists.txt
* replace ${PROJECT_NAME} with the actual name
The spdlog library will not generate export targets if it is a subproject, so it needs to be added to the top-level project's export target set. streets_utils/streets_service_base/CMakeLists.txt * add spdlog target to export set for top-level project
The previous formatting was inconsistent and difficult to read. The new formatting has tabs sizes of 2, among other small changes. streets_utils/streets_service_base/CMakeLists.txt * update formatting
External dependencies and vendored dependencies all now live in the dependencies.cmake file. The top-level CMakeLists.txt file includes dependencies.cmake, a project organization recommended in the Professional CMake book. The spdlog dependency was changed from a source dependency to a binary one. There was really no need to build the project from source. The only requirement was a minimum version. streets_utils/streets_service_base/CMakeLists.txt * move find_package calls to dependencies.cmake * remove spdlog install target since it is no longer a subproject streets_utils/streets_service_base/dependencies.cmake * add find_package calls for Boost, spdlog, and GTest * remove FetchContent call for spdlog. It is now the developer's responsibility to acquire it
Move compile definitions (spdlog level) and compiler flags to target specific commands. This avoids polluting the global project space. streets_utils/streets_service_base/CMakeLists.txt * move global compile definitions to target specific scopes * move global compile flags to target specific scopes * elevate spdlog level to ERROR on unit tests. The TRACE level before was cluttering the output, and in my opinion, trace-level outputs on unit tests are unnecessary.
Install commands were using hard-coded installation paths, which while not technically wrong, would make it harder for consuming packages to change the install location. The install commands were changed to use the recommended GNUInstallDirs variables. streets_utils/streets_service_base/CMakeLists.txt * update install commands to use the install variables defined in GNUInstallDirs * replace header and template file installation to use DIRECTORY variant because it is a bit easier to read and doesn't involve globbing files
The RapidJSON library was previously built from source as a subproject using FetchContent. However, while testing installation to non-standard locations, it was discovered that overriding RapidJSON's install location is not easy (if you want to do it properly). RapidJSON build architecture does not seem conducive to subprojects, so it is probably better to let the developer handle its building/installation. Note: We should add a notice to builders that they need to install RapidJSON from source using a version beyond a specific commit. This is because the streets_service_base uses functionality not found in the latest release.
Clean up and modernize the unit testing section of the CMakeLists.txt file. The file globbing was removed, and target includes and links were made private since there's no need for anything to link against the unit tests. Additionally, the manual unit test commands were replaced with the modern alternative gtest_discover_tests streets_utils/streets_service_base/CMakeListst.txt * remove source file globbing. There were only three files, so it seemed excessive * make target includes and links private since we're building an executable, not a library * replace add_test with gtest_discover_tests so GTest can automatically find unit tests
The streets_service_base project's CMake layout has been broken up and cleaned up. The top-level CMakeLists.txt file now simply calls add_subdirectory on the rest of the project. It is more like a table of contents, a structure recommended in the Professional CMake book. streets_utils/streets_service_base/CMakeLists.txt * library building has been moved to src directory * library install targets have been moved to src directory * unit testing targets have been moved to test directory * an option to build unit tests has been added, and unit tests will only be built if the project is top level or explicitly enabled. This new structure makes the streets_service_base project more amenable to being consumed as a subproject. streets_utils/streets_service_base/cmake/streets_service_base_libConfig.cmake.in * file has been deleted in place of a fully manually created one. Using the CMakePackageConfigHelpers features didn't really make sense in this context because the package has a simple structure and dependency hierarchy. streets_utils/streets_service_base/cmake/streets_service_base_libConfig.cmake * This file was added to replace the one used with CMakePackageConfigHelper. It has the same structure. * add required dependency for RapidJSON * add optional dependency for GTest streets_utils/streets_service_base/src/CMakeLists.txt * move library building and installation to this file. It is included in the main build from the top-level CMakeLists.txt file streets_utils/streets_service_base/test/CMakeLists.txt * move unit test configuration to this file. It is included in the main build from the top-level CMakeLists.txt file * replace manual add_test command with gtest_discover_tests because it is the recommended modern alternative
Instead of hard coding the spdlog log level, it has been moved to a configuration option. The configuration option defaults to a WARN level. streets_utils/streets_service_base/src/CMakeLists.txt * add spdlog logging level configuration option * set list of allowable log levels to make the configuration option appear as a listbox in ccmake/cmake-gui * set default logging level to WARN instead of TRACE. This is likely the desired level for release builds streets_utils/streets_service_base/test/CMakeLists.txt * remove spdlog level definition because it is not used directly in the test code and it is a compile-time definition for the library code. Therefore, there is no way to change it after the library has been built.
If the package does not build unit tests, then GTest is not actually required. However, the CMake configuration previously required GTest in all instances. Now, requiring GTest is conditioned on tests being built. streets_utils/streets_service_base/dependencies.cmake * make GTest being required conditioned on unit tests being built
Remove include directories for unit tests because those header files are provided by linking the streets_service_base library. streets_utils/streets_service_base/test/CMakeLists.txt * remove target_include_directories because those header files are included when linking with the streets_service_base_lib library. There are no test-specific headers
|
@adamlm currently our docker images are built using In either case we would probably wait to merge any non-functional PRs like this one until after our up coming release. Super helpful though, will look through the changes in detail to understand a little more. |
CARMA Streets uses Ubuntu 18.04 Docker containers, which only have CMake version 3.10.2 and Boost version 1.65. Therefore, the versions previously required need to be reverted back so that they are compatible with the production environment. streets_utils/streets_service_base/CMakeLists.txt * revert CMake version to 3.10.2 instead of 3.16 streets_utils/streets_service_base/dependencies.cmake * remove Boost version requirement. Some minimum version should be established in the future.
|
I reverted the CMake version back to 3.10.2. As far as I can see, none of the change I made used features introduced in the newer releases. I successfully built the I also built the I have similar changes for other CARMA Streets packages, specifically those under |
You can submit the PRs whenever. I think we will just hold of on merging any PRs that are not related to new release functionality until after our release to avoid any potential bugs/issues resulting from non functional changes. For oversight it might be helpful if we leave PRs like this in draft to avoid any miscommunication about when they will be merged. I do think this is a necessary improvement for our CARMA-Streets packages in general. Another CMake related change that would be nice to have is to install all the streets_utils libraries/packages under a common namespace in CMake so that we could include them more like this instead of including each library as if it were a completely independent dependency. I am still a novice with CMake but I am learning. |
I will make this one a draft until it's ready for review.
I was thinking about this too. I think Boost has a good components mechanism implemented. I will give it more though and submit an issue/PR when ready. |
|
@adamlm When we get some time it would be nice to revisit these CMake improvements. |
PR Details
Description
This PR addresses issue #236 by modernizing and cleaning up the project's CMake configuration.
The biggest change is architecture. Package dependencies have been moved into a
dependencies.cmakefile to clean up the top-levelCMakeLists.txtfile. All commands related to building thestreets_service_baselibrary have been moved tosrc/CMakeLists.txt, again to clean up the top-level file. Finally, all test-related commands have been moved totest/CMakeLists.txt. The architectural changes are based on the recommended project structure in Craig Scott's Professional CMake book (Section 34.2 in the 13th edition).Other general changes include moving global configurations to target-based ones to avoid unintended and undesirable behaviors. For example, setting compiler flags.
The CMake version is updated to 3.16, which is the version available in Ubuntu 20.04's
aptrepos. With an upgraded version, we can better utilize new CMake features. Configuration options were added to allow consumers to decided on building tests and setting the spdlog logging level. Additionally, tests will not build by default if the project is consumed as a subproject into another project. Consumers can override this if they desire.If there is a design decision that seems unclear, please check my commit messages. I tried to explain my reasoning/justification in them.
Related Issue
Closes #236
Motivation and Context
CMake brings a lot of historical baggage and complexities that developers are trying to address in newer releases. Therefore, it is good project hygiene to periodically review and clean out old CMake usages for better practices. This PR is an attempt to do that.
For a non-technical motivation, I wanted to work on a non-trivial CMake project while also contributing to an open-source project.
How Has This Been Tested?
I built the project in a clean environment and ran the unit tests. Everything seemed to work fine.
I built some consumer projects, and those seemed to build without issues. I have not tested final
_servicepackages due to other dependency/build challenges. However, direct consumers ofstreets_service_baseseem to build fine with the new changes.I took care to ensure the unit test executable is put where the CI scripts expect it to be, so it should not affect that process.
Types of changes
Checklist:
CARMA Contributing Guide