Open ruizpauker opened 1 year ago
Thank you. I check different build combinations on Linux, but not typically on Windows, and even so cJSON is still normally available so I did not catch this.
I've been considering this very topic recently. I'd like to pull control_common into the broker as exported functions to make plugin writing easier. That effectively requires cJSON to exist, or else I could introduce another public header file just for that part and only install it for cJSON builds. But that seems unnecessarily complicated. I don't think cJSON as a hard requirement is a big problem, and I'm keen to promote the dynamic security plugin as the best way to deal with auth, which requires cJSON. I think the upshot is that getting rid of WITH_CJSON is the thing to do.
Compiling in windows with "WITH_CJSON=no" fails:
...\common\control_common.h(4,1): fatal error C1083: Cannot open include file: 'cjson/cJSON.h'
Tested with latest develop branch, commit 2928ca41f41eb42b308da46ab65697914a275264.
IMHO the reason to the problem seems to be the changes on commit 4199e7b2d3b3aecefc1b1b2eec2d960df3d05d73.
common/control_common.h
it seems CJson is mandatory, it's added without#ifdef WITH_CJSON
common/control_common.c
includescontrol_common.h
src/CMakeLists.txt
addscommon/control_common.c
as its sources without conditionals.All that said, the final question I think is whether CJson is now mandatory. In which case, we should get rid of the
WITH_CJSON
flag.