-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support for external records when generating C wrapper #139
Comments
Yes, time. And a lack of people who can and are willy to help. We started this project to maintain the original djinni implementation. Over time people added new features, like the C/Python bindings. Or the C# bindings. This is a tricky situation because what to do? Not accepting new features? Please keep in mind we all, the active maintainer, maintain this project in our spare time. I can not talk for others, but I guess it's pretty similar. |
Understood and acknowledged. |
Yes, of course, there is! For discussion and Q&A
|
Apologies for the late reply. I've sent an email requesting an invite. |
…extension if needed (cross-language-cpp#139)
Thanks for your contribution, @gibutm! As the person responsible for adding support for C# and the partially implemented support for Python through the C wrapper I take full responsibility for the stagnation on those two fronts. On the C# side I think the implementation is fairly complete and we continue to use it at my company. The story is different when it comes to Python, though. The reason I wanted to add the Python support was to make sure no work was lost when Dropbox decided to drop Djinni. They had their Python implementation on a separate branch exactly because it wasn't finished and I thought I'd be able to spend some time and bring it to the finish line. Unfortunately, the task turned out to be more challenging than expected and the fact that we do not consume the Python bindings in our projects makes it harder for me to find the drive to move it forward. At this point, for anyone considering taking over the challenge I'd say that the way forward is to invest automated testing as an indication of how complete the implementation of a binding is, making sure all of Djinni's features are exercised. |
Hi @freitass, thanks for the context on the python implementation.
What i've had success with so far:
Where I need help presently is how to access fields defined inside a record via the C wrapper. I see setter methods for callbacks for these fields, but I don't understand specifically what the purpose of it is. The generated hpp from the C wrapper generator contains a struct representing the record, but it does not contain any members. Maybe you could provide some information on how this works? |
@gibutm I was the reviewer on the initial Python prototype in Djinni, but it was enough years ago that I can't remember full details. I can suggest where you might go to try to figure out the details, which would be to look at the generated code on some of the Python examples/tests. I've been separated from maintenance of Djinni long enough to not know where the Python test suite lives in the new multi-repo setup, but I just found some relevant code in the original Dropbox repo here: https://github.com/dropbox/djinni/blob/python/test-suite/generated-src/cwrapper/dh__client_returned_record.cpp#L60 That's the generated C++ code which uses CWrapper pointers and callbacks into Python to fetch the individual fields of a record and put them into a C++ struct. It's not particularly simple or readable, but gets the job done. It needs quite a bit of support from the Python side to set up all those callbacks in advance. There's also a lot of support structure elsewhere for reference counting to properly handle ownership of interface objects which I can vaguely remember. Note that the CWrapper generator wasn't originally designed to be directly used from pure C code, but only as an intermediate interface between two other generated layers of code. That's why there's no C struct with all the fields you'd expect, and no easy-to-understand interface in C. In the case of Python, the CWrapper was wedged between generated Python (cffi) and generated C++ (pycpp) which performed coordination through the C interface. It would use that interface in order to copy fields from the Python object representing the record, into a C++ struct representing the same record, one field at a time. Defining a C struct to represent a full Djinni record would've been complicated by variable-width fields, containers, strings, etc. so we didn't. Our hope at the time was that the CWrapper generator layer would eventually become a common component used for multiple language generators which used C as an intermediate layer. However, as the project was never taken beyond prototype phase there wasn't any effort to clean up the CWrapper generator for reuse. All that is to say, the details are there, but will take some work to figure out and make usable for your needs. I apologize for my part in leaving the code in such a messy state, but wish you luck if you take the effort to understand it and make it better. PS: Related to the original topic of this issue, the experimental Python branch did originally contain a README file which described which features were still missing at the time: https://github.com/dropbox/djinni/blob/python/README.Python.md#known-limitations |
It seems we miss a few of the original tests. They must have gotten lost when the original code was migrated from the python branch. Some time ago, I tried to find out and understand what is missing to use the Python bindings in a real-world scenario outside of the unit tests. That got interrupted by work ... I wish I could find the time to catch up on that, but I also have to support a family, so maybe better not to have too much spare time ... it's difficult |
This issue is stale because it has 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Will keep this open for now as a reminder that we miss some of the original existing tests for the C Wrapper |
This issue is stale because it has 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
If the C wrapper generation is enabled when trying to consume a record defined in another module the generator will run into an exception
Looking into the source for
CWrapperMarshal.scala
it looks like this feature is not implemented.Looking at the doc Modularization and Library Support it clearly states that it is not supported for Python yet, though it was not obvious just from the docs that this meant it was not supported in the C Wrapper as well.
When can we expect this to be implemented? Are there challenges or obstacles preventing this from being implemented?
The text was updated successfully, but these errors were encountered: