pboettch / json-schema-validator

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

add back JSON_VALIDATOR_INSTALL and disable it if not top level project #185

Closed vrince closed 1 year ago

vrince commented 2 years ago
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX=install ..
make install

returns :

Install the project...
-- Install configuration: ""
-- Installing: ./install/bin/json-schema-validate
-- Installing: ./install/bin/readme-json-schema
-- Installing: ./install/lib/libnlohmann_json_schema_validator.a
-- Installing: ./install/include/nlohmann/json-schema.hpp
-- Installing: ./install/lib/cmake/nlohmann_json_schema_validator/nlohmann_json_schema_validatorTargets.cmake
-- Installing: ./install/lib/cmake/nlohmann_json_schema_validator/nlohmann_json_schema_validatorTargets-noconfig.cmake
-- Installing: ./install/lib/cmake/nlohmann_json_schema_validator/nlohmann_json_schema_validatorConfig.cmake
-- Installing: ./install/lib/cmake/nlohmann_json_schema_validator/nlohmann_json_schema_validatorConfigVersion.cmake
trevor-creator commented 2 years ago

You can test the problem yourself with a couple lines of debug output in CMakeLists.txt. Add these message lines:

# disable tests and examples if project is not super project
if(CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR)
    # I am top-level project.
    set(JSON_VALIDATOR_IS_TOP_LEVEL TRUE)
endif()

message(STATUS "Status before checking top level: build tests ${JSON_VALIDATOR_BUILD_TESTS}, build examples ${JSON_VALIDATOR_BUILD_EXAMPLES}, install ${JSON_VALIDATOR_INSTALL}")

if(NOT JSON_VALIDATOR_IS_TOP_LEVEL)
    set(JSON_VALIDATOR_BUILD_TESTS OFF)
    set(JSON_VALIDATOR_BUILD_EXAMPLES OFF)
    set(JSON_VALIDATOR_INSTALL OFF)
endif()

message(STATUS "Status after checking top level: build tests ${JSON_VALIDATOR_BUILD_TESTS}, build examples ${JSON_VALIDATOR_BUILD_EXAMPLES}, install ${JSON_VALIDATOR_INSTALL}")

Then try setting those variables ON in a higher-level CMakeLists while using as a submodule, and you'll get this:

$ cmake -S . -B build
-- The CXX compiler identification is GNU 9.3.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using the single-header code from /home/user/test/json_schema_validator_issue/nlohmann_json/single_include/
-- Status before checking top level: build tests ON, build examples ON, install ON
-- Status after checking top level: build tests OFF, build examples OFF, install OFF
-- Configuring done
-- Generating done
-- Build files have been written to: /home/user/test/json_schema_validator_issue/build
vrince commented 2 years ago

@trevor-creator not sure to get it ... do we want to enable install when used as a submodule? I thought we just want it when json-schema-validator is the main project 😕

trevor-creator commented 2 years ago

@vrince Two people specifically asked for install when used as submodule: #162, #164

Unfortunately, the fix for that removed the ability to not install when used as submodule, which is why I created my issue.

vrince commented 2 years ago

@trevor-creator I just saw you have an MR on the same topic do you already have a fix? We can completely ditch this one, it was just to at least provide proper compile out of the box on master. If you do not have a fix does it make sense I spend some time trying to find one?

trevor-creator commented 2 years ago

@vrince I have a proposed fix, but I've only tested it as a submodule and local build - I'm not familiar with hunter, and my build environment doesn't have CMake FetchContent. Mine also has a number of merge conflicts from pushes to main after I created the MR.

The only practical difference between the MRs, aside from the overriding of options, is that my MR doesn't install the examples unless both JSON_VALIDATOR_BUILD_EXAMPLES and JSON_VALIDATOR_INSTALL are ON.

If you'd like, I could recreate my MR, or make an MR into your branch, or you could make the change in this one.

vrince commented 2 years ago

@trevor-creator I hope you do not mind I cherry-picked your last commit, put it on top of small fixes I made, fix conflicts. Currently, testing, install as a main project + hunter + install works great. On my way to test with fetch content as a sub-project.

To test it, check out this branch then :

cmake -B build -DJSON_VALIDATOR_HUNTER=ON -DCMAKE_INSTALL_PREFIX=install . 
cmake --build build --target install
vrince commented 2 years ago

Tested with Fetch_content, install is skipped, and build passes correctly.

pboettch commented 2 years ago

Is this ready for review? Sorry for seaming absent. I'm really happy someone takes this problem in his hands.

vrince commented 2 years ago

I think, at least it will fix the default build procedure as readme states it. After I'm willing to help continue to simplify cmake stuff but I need to understand more all use cases you'd like to support.

pboettch commented 2 years ago

IMO, we should limit use-cases to not trying to solve problems of other people's projects here with a CMakeLists.txt which is bigger than the actual validator-code. Kidding.

In reality I'm fed up with a complex-CMakeLists.txt. I'd love to limit the use-case to one: install this library and optionally build and run the tests and that's it. The dependency to nlohmann::json could be influenced via a given dir (with nlohmann_json_DIR) where it will look for either nlohmann/json.hpp or even it's cmake-files.

The fact that we can use it as a CMake subdirectory is nice, but actually superfluous IMHO: the project can very well be integrated as an ExternalProject into another CMake-Project. I'd like to orient the CMakeLists.txt into this direction.

What do you, or other people think? Maybe that's what's already done with your PR? I still see TOPLEVEL however.

vrince commented 2 years ago

I do agree with you, I think this project is like nlohmann_json it's meant to be "included" and then forget about it :D ... So if we drill down to the 2 uses cases :

In both cases, we can assume nlohmann_json is provided by the user and relied on find_package.

The real question is: do you want to support the installation of the library and find_package for json-schema-validator itself. This is where everything cmake related gets complicated... If not we can reduce the usage (user "include" use case) to a single line of FetchContent and that's it.

If going that direction we could do :

For the user is the "only" promoted way to use json-schema-validator could be something like :

FetchContent_Declare(json_schema_validator 
GIT_REPOSITORY https://github.com/pboettch/json-schema-validator.git 
GIT_TAG v2.1.0)
FetchContent_MakeAvailable(json_schema_validator)

The only drawback is that it takes cmake 3.16+

vrince commented 2 years ago

Forgot to mention going that route we can ditch JSON_VALIDATOR_IS_TOP_LEVEL no more export crazy stuff the depends on nlohmann_json and JSON_VALIDATOR_INSTALL cause if someone wants to install he will build the install target ...

The last comment, if we bump to make 3.16+ we could also replace HUNTER with build-in FetchContent to fetch proper version of nlohmann_json in developer mode. A little bit more explicit cause it actually use nlohmann_json version.


FetchContent_Declare(json 
GIT_REPOSITORY https://github.com/ArthurSonzogni/nlohmann_json_cmake_fetchcontent 
GIT_TAG v3.10.4)
FetchContent_MakeAvailable(json)```
pboettch commented 2 years ago

I'm OK with a cmake 3.16 bump. If you could do a CMake-rework in that direction let's get it done and see if someone complains afterwards. I'd make a new version with that thing integrated.

trevor-creator commented 2 years ago

Sorry for the delay. A few issues with the way this went, but #193 will resolve these for anyone with CMake 3.16+. (Anyone stuck on an older CMake version, like me until two weeks ago, will have problems.)

That said, worth noting that when statically building artifacts for deployment on another system, such as when building for an embedded device, dependencies such as nlohmann_json will often not be installed on the build system, and find_package won't work. I've used nlohmann_json in multiple embedded/robotics projects, and the only time I've installed it is when I've had access to Conan to manage dependencies.

HpLightcorner commented 2 years ago

Hi guys, is there any ongoing effort in merging the draft?

pboettch commented 1 year ago

All PRs have been closed due to me changing the main-branch to main. Please reissue your pull-request to that branch. Sorry and thanks.