search

denniskb / spice2

The Spice SNN Simulator
Other
6 stars 0 forks source link

Improve CMake configuration #33

Closed denniskb closed 2 years ago

denniskb commented 2 years ago

according to Mateusz Pusz' feedback:

  1. Unless your source code uses both C and C++ it is good to provide LANGUAGES in project()
  2. For BOOL type cache variable you may prefer to use option()
  3. Why do you add NDEBUG flag manually? Anyway, it will not work for a multi-config generator so please never do a branch like if (CMAKE_BUILD_TYPE ...
  4. Warning flags should be used only in development and CI build. They should not be enforced on the customers.
  5. Handling dependencies with add_subdirectory() does not scale and does not even work correctly in some cases. Please prefer using a packet manager like Conan. For more info see my talks on this subject.
  6. You could add CTest support
  7. Why do you force spice to be a shared library? It is better to not provide anything here and leave the choice to the user via cmake configuration command line.
  8. There is no installation step. You probably want your library to be reused, right?
  9. Talking about reuse it could also be distributed as a Conan package.
  10. Why compile options and include directories a PRIVATE thus not transitive?
  11. CMAKE_SOURCE_DIR is not a good variable to use in most cases. It points to the CMake entry point and if someone will add you as a third party dependency through add_subdirectory() the include directory will not be found
  12. You should not explicitly provide include directories of dependencies. They should be transitively added with target_link_libraries.
  13. The same applies to include directory of spice in test directory