build a C interface to taskchampion #332
Conversation
There was a problem hiding this comment.
This looks good 🥳
Aside from the one error handling note, the C API looks about like I'd expect such a thing to look. I kept thinking "oh that API looks a bit accident prone, what if.." before remembering "C" 🙄
I guess the main test of this would be in actually trying to integrate it with TW (obviously with how it works, but also things like how it integrates with build-systems)
lib/src/replica.rs
Outdated
| /// undone. | ||
| #[no_mangle] | ||
| pub extern "C" fn tc_replica_undo<'a>(rep: *mut TCReplica) -> i32 { | ||
| wrap(rep, |rep| Ok(if rep.undo()? { 1 } else { 0 }), -1) |
There was a problem hiding this comment.
Could maybe return some kind of enum instead of numbers? So on C side it would be end up being roughly identical, but would allow doing tc_replica_undo(...) == TC_UNDO_SUCCESS
There was a problem hiding this comment.
..although it probably makes sense to experiment with error handling on tc_replica_sync as that has more interesting set of errors to deal with (mainly "errors with with error messages")
There was a problem hiding this comment.
I like the enum idea. I don't know how many functions will have the equivalent of Result<bool>, so I'll need to think about how to generalize that. In any case, the error message for any of these is handled with tc_replica_error, so that should be OK.
|
Thanks for the check! How does the |
There was a problem hiding this comment.
Not sure how tedious it to implement but would be better to include some kind of plain-text .sql file and generate the .sqlite3 file from this (or maybe even generate it from the main Rust lib?), instead of checking in the slightly-opaque binary integration-tests/test-db/taskchampion.sqlite3
Oops, that's not supposed to be checked in :) |
It looks perfectly reasonable on first glance, and I agree it definitely seems like something that probably already exists (but such crates can be really hard to search for). @thomcc has a lot more experience with Rust strings+FFI things and may know of something? |
|
Thanks! I got directed toward |
|
I don't know of a great utility crate for strings in FFI (I was pretty happy with the string support I wrote for https://crates.io/crates/ffi-support, but it doesn't support everything your TCString does). I think what you have here is probably not something you'll find off the shelf on crates.io. I'm sick now, so can't give a thorough review, but did a quick skim, and I will note a lot of the code here is unsound, at least technically — Many safe functions accept a raw pointer, and dereference it (or Box::from_raw it). I'm not sure how much that matters to you, though, It's possibly fine given that this seems like an application not a library, and it wasn't clear if these cases are ever public. P.S. Surprised to see two folks I (vaguely) know interacting :) |
|
:waves: I suppose Rust is a bit of a small world :) My understanding is that FFI necessarily involves unsafe Rust, but I'm not sure how what I've done is unsound. Is it specifically functions that aren't impl<'a> TCString<'a> {
pub(crate) fn from_arg(tcstring: *mut TCString<'a>) -> Self {
debug_assert!(!tcstring.is_null());
*(unsafe { Box::from_raw(tcstring) })
}
..., and would marking that function |
Yes. Unsoundness is more or less "you can cause undefined behavior with this safe function". The issue I'm describing is just that the function isn't marked as unsafe, which is why I said it might not matter to you. |
|
OK, thanks -- that's easy to fix! I'm also planning to go back to every |
|
OK, updated to make TCString pass-by-value (which was a major change!) |
|
Looks like I need to work on build-system compatibility on mac, but otherwise I'd love a review of this. |
Hey Apple, maybe "I didn't link the thing you asked me to link" should be more than a warning? |
|
Looks like this is rust-lang/cc-rs#250 -- it just happens to work on linux :( |
it looks like, on windows, a dependency on bcrypt has to be included. Maybe something similar to corrosion-rs/corrosion#120, but in this case it's occuring on Rust 1.47.0. |
|
Woo, all green (except lint, somehow) |
djmitche
changed the title
|
Had a look over the code and all looked good, although it's a lot of changes so I ended up skimming a lot! Two things, one trivial and one maybe less so:
|
|
Oh, cool! I thought about trying to run things under valgrind but assumed it would be much more complicated than that. It looks like that is, indeed, a bug in the test (it doesn't call tc_replica_free) rather than an issue in the API. Your first point is a good one, too -- in general, I don't know how best to go about getting all this linking stuff lined up, and I haven't found any good packages to copy from. So I'm hoping we can land it and find out by people trying to link to it :) Figuring out how to run the tests under valgrind in CI would be amazing, too -- at least worth an issue. |
|
oops, i meant to squash that :/ |
I thought I'd get a start on this, and so far it's going OK.
Among other things, this would make it possible to use TaskChampion as the "backend" to TaskWarrior. It's not clear that's the best plan yet, but even if we don't do so I think this will be a nice addition.