cross-language-cpp / djinni-generator

Command-line tool that generates gluecode from a djinni-IDL file
https://djinni.xlcpp.dev/djinni-generator/setup
Apache License 2.0
91 stars 19 forks source link

Support for external records when generating C wrapper #139

Open gibutm opened 1 year ago

gibutm commented 1 year ago

If the C wrapper generation is enabled when trying to consume a record defined in another module the generator will run into an exception

Exception in thread "main" scala.NotImplementedError: an implementation is missing
    at djinni.CWrapperMarshal.references(CWrapperMarshal.scala:91)
    at djinni.CWrapperGenerator$CRefs.collect(CWrapperGenerator.scala:1097)
    at djinni.CWrapperGenerator$CRefs.collect(CWrapperGenerator.scala:1070)
    at djinni.CWrapperGenerator$CRefs.collect(CWrapperGenerator.scala:1066)
    at djinni.CWrapperGenerator.$anonfun$generateInterface$3(CWrapperGenerator.scala:2121)
    at djinni.CWrapperGenerator.$anonfun$generateInterface$3$adapted(CWrapperGenerator.scala:2121)
    at scala.Option.foreach(Option.scala:437)
    at djinni.CWrapperGenerator.$anonfun$generateInterface$1(CWrapperGenerator.scala:2121)
    at djinni.CWrapperGenerator.$anonfun$generateInterface$1$adapted(CWrapperGenerator.scala:2119)
    at scala.collection.immutable.List.map(List.scala:250)
    at scala.collection.immutable.List.map(List.scala:79)
    at djinni.CWrapperGenerator.generateInterface(CWrapperGenerator.scala:2119)
    at djinni.Generator.$anonfun$generate$2(generator.scala:630)
    at djinni.Generator.$anonfun$generate$2$adapted(generator.scala:623)
    at scala.collection.immutable.List.foreach(List.scala:333)
    at djinni.Generator.generate(generator.scala:623)
    at djinni.generatorTools.package$.generate(generator.scala:396)
    at djinni.Main$.main(Main.scala:947)
    at djinni.Main.main(Main.scala)

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?

a4z commented 1 year ago

Are there challenges or obstacles preventing this from being implemented?

Yes, time. And a lack of people who can and are willy to help.

We started this project to maintain the original djinni implementation. Add fixes and adoptions for new Android and iOS versions. This goal was and is reached.

Over time people added new features, like the C/Python bindings. Or the C# bindings. As in open-source projects often, people join, contribute something, and sometimes disappear afterward.

This is a tricky situation because what to do? Not accepting new features? We decided to accept them as long as the main functionality, the Java and Objective-C interop, works as it did. And carry the risk of problems with the new features, like the Python/C bindings, which require more work to be fully useful.

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. For me, the main obstacle to implementing/finalizing the Python/C binding that someone else started (or any other new functionality) is having a few days with the possibility to focus on the topic and do the work.

gibutm commented 1 year ago

Understood and acknowledged. Is there any way I could help/contribute towards implementing this for the C wrapper layer?

a4z commented 1 year ago

Yes, of course, there is! https://djinni.xlcpp.dev/contributing/

For discussion and Q&A

gibutm commented 1 year ago

Apologies for the late reply. I've sent an email requesting an invite.

freitass commented 1 year ago

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.

gibutm commented 1 year ago

Hi @freitass, thanks for the context on the python implementation. I've been experimenting with using the C wrappers to work with dart. Dart also has an ffi feature that allows it to interop with C.
Dart unfortunately has limitations when it comes to interop with C++ code and this is where a generator which can generate C wrappers over C++ finds a lot of utility. The biggest gain from trying to do this is that it allows apps that use the Flutter Framework to be able to use libraries implemented in C++ . While there exists other methods to achieve this consumption, this approach is desirable because:

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?

artwyman commented 1 year ago

@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

a4z commented 1 year ago

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

github-actions[bot] commented 1 year ago

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.

a4z commented 1 year ago

Will keep this open for now as a reminder that we miss some of the original existing tests for the C Wrapper

github-actions[bot] commented 1 year ago

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.