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

external json & pybind11_json #11587

Merged
merged 6 commits into from
Mar 20, 2023
Merged

external json & pybind11_json #11587

merged 6 commits into from
Mar 20, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Mar 20, 2023

  • for unit-tests, we need easy way to convert from Python objects (mainly dict) to C++ json
  • pybind11 has a side project called pybind11_json that bridges the two
  • it (correctly) includes nlohmann/json.hpp but we use third-party/json.hpp - bad
  • moved json out of the repo and into an external project, so it's now under nlohmann/json.hpp
  • this means it's now downloaded, which is not so good but acceptable
  • added the interfaces to pyrealdds -- unit-tests coming in next PR

@maloel maloel requested a review from OhadMeir March 20, 2023 08:18
Copy link
Contributor

@OhadMeir OhadMeir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find of the pybind11_json, and good work with the CMake files

@maloel maloel merged commit 80240b1 into IntelRealSense:dds Mar 20, 2023
@maloel maloel deleted the pyjson branch March 20, 2023 10:27
@Nir-Az
Copy link
Collaborator

Nir-Az commented Mar 20, 2023

Nice find.

  1. Why doing it in dds and not dev? (the json only part)
  2. Why using the "download during configure" complex flow and not normal external project?
    We used it on pybind11 because we needed using its functions during cmake here I dont think its the same case..

@maloel
Copy link
Collaborator Author

maloel commented Mar 20, 2023

Why doing it in dds and not dev?

You're supposed to be sick...
Didn't want to rock the boat and dds will be merged into dev soon-enough... too much work with too many PRs to do in dev. :)

Why ... not normal external project?

We usually want to download during configure, not during the build. At least, I like it better that way...
We also do the same for fastdds (which uses FetchContent, which we need to fix...).

@Nir-Az
Copy link
Collaborator

Nir-Az commented Mar 20, 2023

I like downloading during cmake too, but the code for making it happen is very ugly..
But OK..

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.

3 participants