search

pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
470 stars 135 forks source link

Rename cmake options #163

Closed sgaisser closed 2 years ago

sgaisser commented 2 years ago

During testing I found a Problem with a non-prefixed cmake option "BUILD_TESTS". This is a very common name which might collide with user options. Other libraries usually use something with the library name e.g. nlohmann/json uses JSON_BuildTests.

pboettch commented 2 years ago

Great find. Please feel free to issue a PR changing that variable-name.

Leon0402 commented 2 years ago

Which is not a bad thing though. Actually the standard name is BUILD_TESTING, which is made available automatically when using include(CTest) -> This also sets enable_testing(). So this should be rather used.

Using the standard names is a good thing, because then people don't have to relearn options for each lib. It's the same with BUILD_SHARED_LIBS. Most libraries use this variable for determining if the library should be static or shared. In fact it's kinda baked into cmake, if you specify just add_library() without STATIC / SHARED.

If you have name clashes with say Fetch_Content then use scopes. For instance: 

# This is a new file dependencies/the_lib_I_use
set(BUILD_TESTING OFF) 
FetchContent_MakeAvailable(lib_i_use)

This will just set the variable in this scope, so in this file. Also works with functions.

So I would recommend:

  1. Use the default names! It makes it easier for everyone
  2. Disable tests by default for fetchcontent (if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING))
pboettch commented 2 years ago

I'm totally agreeing with you.

I someone would make a pull-request, I gladly integrate it.

vrince commented 2 years ago

Hi @pboettch, I think this could be closed.