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 an FFI for temporal_rs #104

Open
2 tasks
nekevss opened this issue Oct 31, 2024 · 6 comments
Open
2 tasks

Add an FFI for temporal_rs #104

nekevss opened this issue Oct 31, 2024 · 6 comments
Labels
C-enhancement New feature or request

Comments

@nekevss
Copy link
Member

nekevss commented Oct 31, 2024

We need to add an FFI for temporal_rs to make it available for use in other languages.

Languages that should be supported (Feel free to add more to the list):

  • C
  • C++

Libraries that may be useful in this effort would be rust-diplomat/diplomat.

@nekevss nekevss added the C-enhancement New feature or request label Oct 31, 2024
@Manishearth
Copy link
Contributor

Manishearth commented Dec 5, 2024

From the ICU4X/Diplomat side I'm quite happy to try and help with this, at least getting y'all set up and contributing APIs when we need them. We are interested in C++ bindings for temporal_rs for V8, and I'd happily contribute Diplomat bridge code as we need it, though I'd love to have help from the Temporal team doing this. Either way, I can get the initial setup written out.

Diplomat should support everything you need here, and if it doesn't it can be added.

Diplomat right now can generate C and C++, as well as Dart, JS/TS, and Kotlin. Kotlin is still actively under development. JS has some incoming big changes but is close to stable.

I'd start with just C/C++ and add others if people ask.

Some questions:

Where should this live?

The way Diplomat works is that you write a crate, typically a separate crate, that contains a lot of mod ffi declarations that have a macro applied, and write thin wrappers in the crate. This would be a temporal_capi crate. It could live in this repo, or a separate one. A separate one would probably be more work every time Temporal does a release.

An issue is that this repo is currently structured such that it has only one crate, so adding another may involve turning it into a workspace or something. We can also just add a capi folder and fix up a proper workspace setup.

Until temporal_rs stabilizes more I do think we ought to use the same repo for this. It's a pain updating code for less-stable dependencies whereas it's typically trivial for the people who make those changes.

Recommendation: Same repo

Do we want checked in generated bindings, or something else?

ICU4X checks in the generated bindings and ensures they are fresh with a CI job. This means anyone who changes temporal_capi would need to rerun Diplomat and check in the updates. This can be made convenient with a cargo-make script or a utility binary you can cargo run (ICU4X does the latter, it means we can depend on prereleases if we want and don't need to have a separate "install the diplomat binary" step).

One really nice property of checking in the bindings is that they get packaged as a part of the crates.io package. You can still do that but you have to remember to run diplomat at release time and you may find some issues when doing so.

Recommendation: check in the bindings

Do we want to enforce all public API is covered by Diplomat bindings?

ICU4X has a script that ensures that all public API is covered by Diplomat bindings (or has an exception). We could use the same in Temporal.

Worth considering in the long run, probably unimportant in the short run. It will mean that everyone adding APIs to Temporal will need to potentially write Diplomat bridge code. Which is quite easy, but still it is different.

Recommendation: Not for now, revisit in the future.

@jedel1043
Copy link
Member

@Manishearth Agreed with all your recommendations.

About the workspace, we're planning on adding a test suite crate like Boa's that pulls test262 and runs the related Temporal tests, so I'd say we should transition into a workspace asap.

@nekevss
Copy link
Member Author

nekevss commented Dec 6, 2024

Agreed with all of the recommendations as well!

@Manishearth
Copy link
Contributor

Sounds good. If you do that soon I can add a stub capi crate and start adding stuff for it!

@jedel1043
Copy link
Member

@Manishearth We merged #126, so we can start integrating diplomat into the project.

@linusg
Copy link

linusg commented Jan 17, 2025

I'm planning to use this library in Kiesel to implement Temporal, just like I already use the ICU4X C bindings for Intl.

nekevss pushed a commit that referenced this issue Jan 21, 2025
Progress on #104

This adds a simple diplomat integration, but does not use it yet. When
we start using it we should add CI that runs `cargo run -p diplomat-gen`
and ensures there is no diff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants