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

Support for external records when generating C wrapper #139

Open
gibutm opened this issue Oct 14, 2022 · 11 comments
Open

Support for external records when generating C wrapper #139

gibutm opened this issue Oct 14, 2022 · 11 comments

Comments

@gibutm
Copy link

gibutm commented Oct 14, 2022

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
Copy link
Contributor

a4z commented Oct 14, 2022

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
Copy link
Author

gibutm commented Oct 17, 2022

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

@a4z
Copy link
Contributor

a4z commented Oct 17, 2022

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

For discussion and Q&A

  • We have the discussion sections in this repo.
    This topic, Python custom types #137 , might relate to what you are looking for.

  • There is the mobilecpp.slack.com ,
    The self-registration link stopped working since Heroku took it down, but when I figure out what is required for the self-invitation, I can add it to another location. But you can tell me your mail, and I will invite you.

@gibutm
Copy link
Author

gibutm commented Oct 21, 2022

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

gibutm pushed a commit to gibutm/djinni-generator that referenced this issue Oct 24, 2022
gibutm pushed a commit to gibutm/djinni-generator that referenced this issue Oct 24, 2022
@freitass
Copy link
Contributor

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
Copy link
Author

gibutm commented Oct 25, 2022

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:

  • It allows developers to write a layer implemented in Dart that can interact with the source base in C++ via the C wrapper.
  • Because of how it works, it is more performant than the alternative.
  • It is also generally speaking, more maintainable than the alternative because there is lesser platform specific glue code that needs to be maintained.

What i've had success with so far:

  • Create an instance of an interface from C++ and consume a pointer of the struct in dart.
  • Invoke methods with an instance of the interface.
  • Consume (in dart) a string returned from invoking a function.
  • Get a handle to a record instance returned after invoking a function.
  • Generate C wrappers when referencing a record defined in another module. (This is what draft PR [Draft] External symbol support for c wrapper  #141 enables)

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
Copy link

@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
Copy link
Contributor

a4z commented Oct 25, 2022

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
Copy link

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.

@github-actions github-actions bot added stale Issues and PRs without activity and removed stale Issues and PRs without activity labels Mar 19, 2023
@a4z
Copy link
Contributor

a4z commented Mar 25, 2023

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

@github-actions
Copy link

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.

@github-actions github-actions bot added stale Issues and PRs without activity and removed stale Issues and PRs without activity labels May 25, 2023
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

No branches or pull requests

4 participants