pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
466 stars 134 forks source link

Conflicts when integrating with C++17 project #64

Closed ArkadiuszRaj closed 4 years ago

ArkadiuszRaj commented 4 years ago

While integrating this lib (as static one) with my project I am facing following issue.

Despite of properly configured cmake for my C++17 code add_subdirectory() populates c++11 requirement , thus -std=gnu++11 is added. That destroys complication of my code.

pboettch commented 4 years ago

In your project, how do your request CMake to compile your code with C++17? Which version of CMake are you using?

ArkadiuszRaj commented 4 years ago

cmake version 3.10.2

I am defining support in my cmake like:

set_property(GLOBAL PROPERTY CXX_STANDARD_REQUIRED ON)
set_property(GLOBAL PROPERTY CXX_STANDARD 17)

Temporarily I have changed code in yours cmake in such a way:

#target_compile_features(json-schema-validator
    #PUBLIC
        #cxx_range_for) # for C++11 - flags
# C++11 requirement

set_property(TARGET json-schema-validator PROPERTY CXX_STANDARD_REQUIRED ON)
set_property(TARGET json-schema-validator PROPERTY CXX_STANDARD 11)

which makes my project compiling again, with properly linked static libjson-schema-validator.a

pboettch commented 4 years ago

Instead of setting the global property of CMake, could you try

target_compile_features(<your-target> PRIVATE|PUBLIC cxx_std_17)

and leave the line as in json-schema-validator's CMakeLists.txt?

In fact we should not mix (GLOBAL) PROPERTY and compile-features in CMake. Compile-features are more flexible than PROPERTY, so I'd recommend using them.

The cxx_std_ features are new with CMake 3.8 which is the reason for me not using them as I want to support CMake 3.2.

ArkadiuszRaj commented 4 years ago

Originally I had no global properties, but related to my target. However problem with unit tests arose, as -std=gnu++11 was added later due to including json-schema-validator code.

Will check your suggestion and report result here.

pboettch commented 4 years ago

Just one more word on compile-features. They transitive!

If a libA requires PUBLIC cxx_std_17 and libB PUBLIC cxx_std_11 they will individually build with 17 or 11 respectively. But a target depending on A and B will build with cxx_std_17.

This is usually what one wants.

ArkadiuszRaj commented 4 years ago

True, the problem is that properties works globally while target_compile_features must be defined per target. Imagine I have 200+ unit test executables. For every single of them I need now add this clause.

Well, I did update to my cmake files. For now we can assume case is closed.

pboettch commented 4 years ago

No no, not for every single one.

If you have a library use target_compile_features(lib PUBLIC cxx_std_17) and then link you unit-tests with this library, they will inherit c++17 (transitive as I said above).

If you don't have a library, you could make an interface-library combining all common requirements.

add_library(unit-test-common INTERFACE)
target_compile_features(unit-test-common INTERFACE cxx_std_17)
target_link_libraries(unit-test-common INTERFACE json-schema-validator ...)

macro(add_unit name)
    add_executable(${name} PRIVATE ${ARGN})
    target_link_libraries(${name} PRIVATE unit-test-common)
endmacro()

add_unit(test1 test1.cpp other-files.cpp)
...
ArkadiuszRaj commented 4 years ago

Thx for the tip! And thx for the effort for creation of this superb lib :)