Use jsoncpp as a submodule #1462
Conversation
lbonn
requested review from
pattivacek,
eu-siemann,
mike-sul,
Zee314159,
xcheng-here and
kbushgit
| # amalgamate jsoncpp source at compile time | ||
| # note: jsoncpp has a CMake support that they intent to deprecate and is hard to | ||
| # integrate with. | ||
| execute_process(COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/scripts/amalgamate-jsoncpp.sh ${CMAKE_CURRENT_SOURCE_DIR}/third_party/jsoncpp ${CMAKE_CURRENT_BINARY_DIR}/jsoncpp) |
There was a problem hiding this comment.
Why do we have to do this amalgamation?
How else do they build if they are deprecating their CMake support?
There was a problem hiding this comment.
Mainly meson https://github.com/open-source-parsers/jsoncpp/wiki/Building.
I've made another attempt by using CMake the "proper" way and mostly just add_subdirectory() but it was quite hard to control the compilation flags and I think I did not manage to remove the compilation of some internal test executable.
Just using the amalgamated source, like we were doing before, gives more flexibility.
Note that the amalgamation process is done at configure time and that cmake would have to be re-run after updating the submodule.
There was a problem hiding this comment.
Yeah, this seems fine, it's just a bit weird.
src/libaktualizr/CMakeLists.txt
Outdated
| $<TARGET_OBJECTS:jsoncpp> | ||
| $<TARGET_OBJECTS:http> |
There was a problem hiding this comment.
Why break the alphabetical ordering?
There was a problem hiding this comment.
I will remove this, that's a relic of a previous version.
| @@ -46,6 +46,23 @@ TEST(Utils, PrettyNameOk) { | |||
| EXPECT_FALSE(PrettyNameOk("foo-bar-123&")); | |||
| } | |||
|
|
|||
| TEST(Utils, parseJSON) { | |||
| // this should not cause valgrind warnings | |||
There was a problem hiding this comment.
Agreed... what warnings might it throw?
There was a problem hiding this comment.
There were some uninitialized memory accesses until 1.8.2 (but not in the ancient version we were using).
See open-source-parsers/jsoncpp#651 and open-source-parsers/jsoncpp#578.
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
There is a valgrind issue in 1.7.x Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
lbonn
force-pushed
the
fix/jsoncpp-submodule
branch
from
dd73a67
to
371aab7
Compare
Codecov Report
@@ Coverage Diff @@
## master #1462 +/- ##
==========================================
+ Coverage 80.61% 80.61% +<.01%
==========================================
Files 184 184
Lines 11068 11079 +11
==========================================
+ Hits 8922 8931 +9
- Misses 2146 2148 +2
Continue to review full report at Codecov.
|
No description provided.