Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add yyjson support to avoid rapidjson on Debian #1329

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mzf-guest
Copy link

Conduit uses rapidjson as thirdparty but its license is not compatible with Debian Free Software Guideline because of the additional clause "The Software shall be used for Good, not Evil.". More information here: https://wiki.debian.org/qa.debian.org/jsonevil

As I'm working on packaging conduit for Debian, this Pull Request suggests to use yyjson as a replacement.
I've implemented a thin wrapper to call yyjson throught the rapidjson API, in order to keep the two json backends working.
The wrapper is defined in src/thirdparty_builtin/yyjson/conduit_yyjson.h and the CMake variable USE_SYSTEM_YYJSON enables yyjson.

All non-regression tests are passing on my machine with yyjson and rapidjson, but it likely needs additional testing with real cases.

Let me know if you agree this way to push conduit to Debian, and feel free to advise/modify/contribute!

Best,
François

@cyrush
Copy link
Member

cyrush commented Oct 22, 2024

@mzf-guest this is great!

Thanks for bringing this to my attention.

We are on a very old version of rapidjson & I am open to replacing with yyjson.

I need a little more time to check our the project, I'll get back to you soon.

@cyrush
Copy link
Member

cyrush commented Oct 28, 2024

@mzf-guest

I would like to include yyjson as a vendored TPL (just like rapidjson)

Then the option can be:

ENABLE_YYJSON = default to off, to keep rapidjson as the default. 

Over time we would switch off rapidjson, but still need it as an option if there is a pick compiler somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants