pboettch / json-schema-validator

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

Modernize cmake script #262

Closed LecrisUT closed 1 year ago

LecrisUT commented 1 year ago

Mainly switched to FetchContent format

Fixes: #250

LecrisUT commented 1 year ago

Any chance to transform tabs in CMake-files to 4 spaces? Otherwise great work.

No problem at all, I'll change my ide settings to make sure it doesn't overwrite in the future. There is cmake-format that could be enforced, but it does not have nice formats like those generated by clion. But if you find a template useful there, we can go with that.

You seem to remove the Hunter-stuff. Is that intended?

Yes, a few reasons for that. The functionally overlaps with FetchContent afaiu, but the latter is built-in and has better integration. E.g. when you recursively FetchContent, all required dependencies are downloaded and included only once. Makes it easier for the end-user to customize the behavior of the dependencies as well, e.g. setting JSON_Install on this project or further up.

LecrisUT commented 1 year ago

Ok, good for review now. (probably you got bombarded with push commit notifications?). Anyway I've found that intel compiler + v3.8.0 tests are failing (hooray for CIs).

LecrisUT commented 1 year ago

(This one had some internal github action issue. Can restart the ci to make it green. I can only do that by pushing commits)

pboettch commented 1 year ago

Does the README.md needs to be updated in order to reflect the changes made here?

pboettch commented 1 year ago

It seems we are running nlohmann-json-unit-tests as well. Why?

LecrisUT commented 1 year ago

Does the README.md needs to be updated in order to reflect the changes made here?

For this stage I did not make any api changes, just added the options:

I guess those should be documented. For that, I would prefer to refactor the README a bit to keep the information to a minimum there and move the details to cmake/README.md (e.g. configuring json_validator as a shared/static library and specifying a specific nlohmann_json version to fetch). For me the minimum cmake related documentation to be had in the README are:

On my projects I have designed the documentations such that the documentation in the plain markdown syntax are synced with shpinx/rtd one. I can port that structure here if you want.

It seems we are running nlohmann-json-unit-tests as well. Why?

This is weird. According to the flow those tests should not appear when it is FetchContet-ed. Good catch, I'll investigate this a bit

LecrisUT commented 1 year ago

Ok, I see the issue. nlohmann/json cmake file was misconfigured in v3.8.0. This is only fixed in v3.10.0. What do you want to do on this? I can disable the tests manually in the preset file, but do we want to support v3.8.0 given that the upstream tests fail anyway, or can we bump it to v3.10.0 to keep the configuration simple.

pboettch commented 1 year ago

There are quite some users of 3.8.0 and this library. If we can hack away running nlohmann-json's tests it would be the best solution.

LecrisUT commented 1 year ago

If we can hack away running nlohmann-json's tests it would be the best solution.

It's not that it is a hack, it is that I prefer the configurations to be kept minimal so that they are easily reproducible by others. Anyway here you are ;)

pboettch commented 1 year ago

Thanks a lot for this work. It's highly appreciated.