nathan-at-least commented 2 years ago

Is your feature request related to a problem? Please describe.

It seems like the headers in ./src/rust/include are hand-written. This can be tedious and error-prone.

Describe the solution you'd like

Use an automated tool to generate header files directly from rust source to reduce maintenance burden, if we can find a suitable one.

Alternatives you've considered

Status Quo: The current approach of header maintenance has been successful so far.

I'm putting my effort on this ticket into evaluating tools.

Tool Evaluations

[x] cbindgen 👎
[ ] rust-bindgen
[ ] cxx

`cbindgen` evaluation

Effort: a couple of hours. Work-in-progress here: https://github.com/nathan-at-least/zcash/tree/cbindgen

My assessment is that cbindgen doesn't seem tenable:

It has noisy false positive/negative log Warnings. Some of those should be fine and should not affect the results.

Example false positive: complaining about all non-pub items which don't appear in the header anyway.
Example false negative: it skips translation of pub const SPROUT_WITNESS_PATH_SIZE: usize = sprout::PATH_SIZE, but happily inserts the usage of SPROUT_WITNESS_PATH_SIZE into the header, leading to C++ undefined symbol failure.

It doesn't support types like * mut SaplingProvingContext out of the box, and I don't care to figure out if it's possible to massage the types to work with cbindgen.

Overall, I have a feeling it's very "heuristic"-oriented and isn't using the rust compiler to the full effect (as the false negative example illustrates), so I don't have confident in the safety/correctness of its results.

nathan-at-least commented 2 years ago

After exploring cbindgen a bit, I decided it probably wouldn't pay off over the status quo, so I changed this ticket to a more general suggestion for any automated tool, and changed the format to a tools-evaluation, then provided by cbindgen evaluation.

str4d commented 2 years ago

cxx is the approach I've investigated on a few occasions. Each time, I've been blocked on one thing or another, either within the build system integration or the type support. I'm hopeful that as time goes on, it gets more likely that this space will at some point Just Work for us.

str4d commented 2 years ago

5972 is now the issue for integrating `cxx` (which I finally succeeded at doing in #5971). This issue can be closed once we've finished migrating the hand-wired FFI to use `cxx`.

zcash / zcash

Use an automated tool for generating correct FFI headers. #5716

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Alternatives you've considered

Tool Evaluations

`cbindgen` evaluation

5972 is now the issue for integrating `cxx` (which I finally succeeded at doing in #5971). This issue can be closed once we've finished migrating the hand-wired FFI to use `cxx`.

zcash / zcash

Use an automated tool for generating correct FFI headers. #5716

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Alternatives you've considered

Tool Evaluations

cbindgen evaluation

5972 is now the issue for integrating cxx (which I finally succeeded at doing in #5971). This issue can be closed once we've finished migrating the hand-wired FFI to use cxx.

`cbindgen` evaluation

5972 is now the issue for integrating `cxx` (which I finally succeeded at doing in #5971). This issue can be closed once we've finished migrating the hand-wired FFI to use `cxx`.