mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
1.07k stars 85 forks source link

[WIP] CMake Refactoring and Option Cleanup #456

Closed COM8 closed 1 year ago

COM8 commented 1 year ago

Up until now CMake options where scattered all around the project. This PR moves them to a central place inside the main CmakeLists.txt file.

Changes

With the new way of handling CMake options a summary will be printed during configuration:

[cmake] -- mp-units CMake Options
[cmake] -- =======================================================
[cmake] --   UNITS_BUILD_TESTS: OFF
[cmake] --   UNITS_BUILD_DOCS: OFF
[cmake] --   UNITS_BUILD_EXAMPLES: OFF
[cmake] --   UNITS_IWYU: OFF
[cmake] --   UNITS_USE_LIB_FMT: OFF
[cmake] --   UNITS_AS_SYSTEM_HEADERS: OFF
[cmake] --   UNITS_WARNINGS_AS_ERRORS: OFF
[cmake] --   UNITS_DOWNCAST_MODE: ON
[cmake] -- =======================================================
gitpod-io[bot] commented 1 year ago

mpusz commented 1 year ago

I am sorry, but I will not merge this PR. As I stated several times in my talks as well as in other issues and PRs, I believe that the current approach is the way to go. My observation is that people tend to be focused on having/adding configure options for every subdirectory, but they are missing the most important point, which is the separation of the customer and development environment.

Customers of the project should not be trashed with our compilation warnings and errors, their ctest should not expose our unit tests, and their CMake target names should not potentially conflict with ours. They simply do not (and should not care) about it, and we should not modify their development environment in any case. They also should not be forced to install any of our additional dependencies to compile the core code (i.e. catch2).

This is why this project has two entry points:

With such an approach, all the custom CMake options are not necessary (maybe besides the documentation). You are either a project developer and need to compile everything every time to ensure its correctness, or you are a project user, and the src directory is the only thing you are interested in. Providing such options will only cause confusion to the users and broken PRs by the dev team.

Anyway, in the world of Modern C++ and CMake we should stop using add_subdirectory() for dependencies handling as it has many additional issues as well. We have great package managers and we should benefit from those. In such a case, all the changes proposed here are of no use.

COM8 commented 1 year ago

OK, thanks for your response! This PR came into existence since the beta 0.8.0 package for conan uses a version of fmt that is too old for our use case so we had to update it manually leading to including this lib via fetch_content, resulting in issues.

mpusz commented 1 year ago

Try including only the src subdirectory. It should help.

Also, you can always easily force/override another (i.e. newer) version of the dependency to be used in Conan (assuming that all the code compiles fine with such a version).