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

Replace hand-rolled FFI with cxx #3426

Closed
djmitche opened this issue Mar 1, 2024 · 14 comments
Closed

Replace hand-rolled FFI with cxx #3426

djmitche opened this issue Mar 1, 2024 · 14 comments
Assignees
Milestone

Comments

@djmitche
Copy link
Collaborator

djmitche commented Mar 1, 2024

There's a heavy load of both Rust and C++ code interfacing between Taskchampion and Taskwarrior. It's very tricky to get that stuff right, and easy to add UB.

One of the popular alternatives is https://cxx.rs/, which generates Rust and C++ glue for you. It's a little limited in that it only supports some basic types, but most of what we need is vectors and strings, so that should be OK.

@djmitche djmitche self-assigned this Mar 1, 2024
@djmitche
Copy link
Collaborator Author

djmitche commented Mar 1, 2024

WIP in my cxx-experiment branch

@djmitche
Copy link
Collaborator Author

djmitche commented Mar 1, 2024

@aktaboot suggests https://codeberg.org/pitbuster/dolphin-rom-thumbnailer/src/branch/main/src/CMakeLists.txt as an example that uses CMake, Corrosion, and the Corrosion CXX support (corrosion_add_cxxbridge).

@djmitche
Copy link
Collaborator Author

djmitche commented Mar 7, 2024

More updates in the linked branch, if anyone wants to follow along.

@djmitche
Copy link
Collaborator Author

djmitche commented Mar 7, 2024

In particular, some notes here

@ryneeverett
Copy link
Collaborator

This approach looks really promising!

some creativity is required in writing the bridge, and I think it can
safely be considered TW-specific. But, that means it can also omit all of the
functionality that TW doesn't use!

Am I correct in interpreting this to mean that you plan to vendor the cxx integration from corrosion?

I was initially curious about the stability of this feature given the ⚠️⚠️⚠️ EXPERIMENTAL ⚠️⚠️⚠️ warning in the docs but if we're vendoring this part I suppose it's less relevant.

@djmitche
Copy link
Collaborator Author

djmitche commented Mar 7, 2024

It seems to work even without vendoring it. I suspect we're not doing anything too exciting from corrosion's perspective, so maybe the EXPERIMENTAL bits are not problematic. Or, maybe it won't work on some other platform than mine :(

@aktaboot
Copy link

aktaboot commented Mar 9, 2024

I can test if there's PR :)

@djmitche
Copy link
Collaborator Author

Hey @n8henrie, I saw your issue in dtolnay/cxx#1327 -- I'm also deep in the weeds of CXX, although going the other way (supporting calling into Taskchampion from Taskwarrior). Would you be interested in joining forces?

@n8henrie
Copy link

Yes, would love to pitch in as much as I can -- I gave a little more background in an email reply I just sent.

My greatest interest / dream would be providing a rust library for TaskWarrior that would enable downstream work to reimplement the CLI and potentially other front-ends in rust (potentially cross-compiling to other architectures, etc.). As a disclosure, part of my motivation is that I don't know C++, which will limit the amount of help I can be!

As we've mentioned in the past, my goal for https://github.com/n8henrie/taskwarrior-rs/ was to create FFI bindings to fulfill this purpose (and potentially could act as a bridge to progressively migrate the TW codebase to rust -- though I'm not sure how rust-friendly/interested the rest of the TW team is), but obviously my limited understanding of C++ makes that an extremely slow-going task.

And as mentioned before, I'm more than happy to release the taskwarrior crate to the official team once / if there is working rust interop (or perhaps a frontend)!

@jschwe
Copy link

jschwe commented Mar 27, 2024

I was initially curious about the stability of this feature given the ⚠️⚠️⚠️ EXPERIMENTAL ⚠️⚠️⚠️ warning in the docs but if we're vendoring this part I suppose it's less relevant.

Corrosion Maintainer here. The experimental here means:

  1. This function does not have any SemVer guarantees, and I reserve the right to make breaking changes even outside of major releases. I would document such changes in the release notes though. The reason for this is point 2:
  2. I have received basically 0 feedback from users on how well or not well this function is working for them. I haven't really worked much with C++ / cxx, so it's hard for me to say if there is still something important missing in this function.

Implementation wise, I haven't touched this function in a while. As long as you read the changelog before upgrading corrosion, you should be fine.

@djmitche
Copy link
Collaborator Author

Thanks for having a look at Taskwarrior, @jschwe!

@djmitche djmitche transferred this issue from GothenburgBitFactory/taskwarrior Apr 21, 2024
@djmitche djmitche transferred this issue from GothenburgBitFactory/taskchampion May 2, 2024
@djmitche
Copy link
Collaborator Author

djmitche commented May 2, 2024

I was mistaken in thinking this should be solved in the Taskchampion crate. That crate should be pretty much pure Rust, with other applications left to figure out how to link to it. That's how I've just set things up in #3209.

@djmitche djmitche added this to the v3.1.0 milestone Jun 4, 2024
@djmitche djmitche moved this from Ready to Backlog in Taskwarrior Development Jul 6, 2024
@djmitche djmitche removed this from the v3.1.0 milestone Jul 6, 2024
@djmitche djmitche moved this from Backlog to Ready in Taskwarrior Development Aug 5, 2024
@djmitche
Copy link
Collaborator Author

djmitche commented Aug 5, 2024

Once GothenburgBitFactory/taskchampion#372 is done and 0.7.0 is released, I will push the cxx-experiment branch and make a PR for this. It's passing tests!

@djmitche djmitche added this to the v3.2.0 milestone Aug 5, 2024
@djmitche
Copy link
Collaborator Author

That was #3588.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

5 participants