Complete bindings with Bindgen #1
Conversation
|
@tmfink this looks totally awesome! I guess I'm sort of an old timer and like writing the bindings by hand, but you're right bindgen is the way to go for the Future. So before merging:
Also I'm fine with feature renames, but in general should try to avoid in the future. So make sure these are the names you want! Otherwise this is great work! |
|
Good to hear that you are receptive! I made no attempt to keep function/type names the same (bindgen likes to do its own thing), so I would not be surprised if m4b/capstone-rs does not work with this code (yet). I'm receptive to changing the name of features, but I did not think that |
|
That's a pretty huge problem if the symbol names are different... I would expect modulo feature names and some other minor details for capstone-rs to work completely out of the box? How do I use the bindings I wrote; it's not quite clear to me from the feature names/ I can't get it working? |
|
So I can't even compile the branch... Also when I try to actually use this in So I didn't realize that bindgen pulls down a bunch of deps, which is a huge bummer. Did you need to delete the handwritten bindings? Seems they could have been easily put in a feature flag, made bindgen dep be optional (like cmake), and then I wouldn't have to update capstone-rs. As it stands, I simply don't have the time to update capstone-rs to new bindings (and test) and etc. |
There was a problem hiding this comment.
I can't merge this until:
- The CI passes
- It builds
- It builds with a downstream client, capstone-rs for example
- The bindgen feature flag thing is decided on
- What the scope of the symbol changes are; it worries me that capstone function names and arguments would differ; that seems directly like an ABI breaking change, which would be terrifying if bindgen does that...
Cargo.toml
Outdated
|
|
||
| [features] | ||
| # use bundled capstone and bindings by default | ||
| default = [] |
There was a problem hiding this comment.
Don't you need to set the default feature here?
There was a problem hiding this comment.
Leaving the comment was a mistake; my intention was to require exactly one of the use_bundled_capstone or use_system_capstone features.
| #![allow(non_camel_case_types)] | ||
| extern crate libc; |
There was a problem hiding this comment.
I don't think you should have deleted these bindings, at least for time being could put behind a feature flag.
There was a problem hiding this comment.
I figured that if you prefer, you could make a new "bindgen-wip" branch. Once you felt that it was in good enough condition, you could merge the changes to master.
It would be confusing to a consumer of this library that the interface is different based on which features are specified.
There was a problem hiding this comment.
Derp, good point. Also I believe it technically breaks cargo. So it's bindgen or nothing then...
| @@ -12,11 +12,17 @@ repository = "https://github.com/m4b/capstone-sys" | |||
| libc = "0.2" | |||
|
|
|||
| [build-dependencies] | |||
| bindgen = "0.26.3" | |||
There was a problem hiding this comment.
This can be optional with undeleted bindings; just need to add a feature flag on the bindings and reexport iiuc
|
The function names/arguments should not be different because bindgen creates them directly from the headers. Assuming the function definitions were originally defined correctly, they should match. However, the variables will be often be different. For example, currently you define pub type cs_arch = u16;
pub const CS_ARCH_ARM: cs_arch = 0;wheras bindgen generates: #[repr(u32)]
pub enum cs_arch {
CS_ARCH_ARM = 0,
// ...
}There are several correct ways to define these. I think that by the C standard, the compiler could use either 16 or 32 bit types. |
|
Ok, yea, I figured enums, etc might be slightly different, but you had me worried when I though you meant the function symbols were different. |
|
I don't have a lot of time to give to testing this, but I added you as collaborator; feel free to push a new branch, say |
|
I agree, and tried a PR but they never acknowledged or responded. I think it's a favorite pasttime these days to write capstone bindings. There are at least 2 more (besides these two) I know about 😆 |
|
I gave you access over there too, knock yourself out ! |
|
Also feel free to try to merge again, or ask if they don't want to maintain maybe they can transfer ownership to you |
|
oh hey. github notifications get lost pretty easily, was the PR on my bindings? I spoke to someone (you?) about having capstone-rs use capstone-sys under the hood, which I'm totally on board with but also don't have time to implement. |
|
@richo-stripe we talked about modifying richo/capstone-rs to use m4b/capstone-sys. However, I think it will be easier to modify m4b/capstone-rs to use the bindgen version of m4b/capstone-sys since it already consumes capstone-sys. Once my changes merged into @m4b's Possible solutions:
We should really reduce the fragmentation of capstone's rust bindings. |
|
/cc @endeav0r I know you have a fork too, so probably interested 😆 |
|
So in case this wasn't clear:
|
|
@m4b I would not mind maintaining the sys and high-level bindings =) |
|
@tmfink I'm ok with that. What needs to be done next, besides getting it passing on CI? I imagine high level bindings need to be updated as well, etc.? I'll let you figure out how you want to integrate/re-merge/whatever with other capstone binding forks/projects :D |
|
@m4b I already have https://github.com/m4b/capstone-rs/tree/bindgen-sys-wip working with this The only other item I want to do is have a separate pre-generated If it turns out that the bindgen-generated bindings are identical between platforms, then I'll just mark the existing work as done. |
|
@m4b it looks like bindgen generates the same bindings for different platforms. I did not see any difference when targetting the following platforms (by passing the |
|
Ok you're saying it's ready to merge ? |
|
Actually I don't understand, why did the submodule need to disappear? Now capstone version/git commit isn't tracked, iiuc? |
|
E.g., I had a submodule before, and CI had no issues with it. I'm not sure getting rid of the submodule is a good idea. |
|
@m4b I would say this is ready to merge--except I want to squash some of the commits to minimize history pollution. The capstone version is tracked with in the Git submodules cause the following problems:
The following is a good read: |
Using git submodules caused the following problem: * Travis CI ran into a hard-to-diagnose "error: Server does not allow request for unadvertised object" * If the repository was not cloned recursively, then the capstone library would need to be fetched over git. This requires git and network access. `scripts/update_capstone.sh` can be used to update the bundled `capstone`.
Also reformatted code with rustfmt.
|
Okie if you think it's best. |
|
@tmfink You can merge the branch now if you think its ready (once CI passes) |
|
Do you want me to publish a new version on crates.io or you want to improve more stuff ? |
|
@m4b I think publishing a new version would make sense. The heavy lifting will be done for |
|
k, will publish right now; should i transfer this repo to the org you just made? (i assume this is possible) |
|
hum, why wouldn't this be a fast-forward? |
|
it's probably too late but i also realized that bundling capstone forces users who want the system bindings to still download a 24mb repo; just a point in favor of git submodules |
|
alright published 0.3 on crates.io |
|
@m4b I think the error you saw was because the individual files under I think the 24MB size is regrettable, but it's actually less data than normal. A git checkout of Please transfer this repo under https://github.com/capstone-rust =) Thanks for the crates.io publish! |
This wraps the entire capstone interface by generating bindings with bindgen. I think bindgen is the way to go because capstone will change over time and we need to be able to change the bindings quickly without manual effort.
For now, bindgen's wierd union type is used, but a pull request is in the pipeline to use Rust untagged unions.
I would love to hear feedback.
For example, I changed around the names of some features and added some new ones. The
use_bundled_capstone_bindingsfeature does not require bindgen to run at build time and eliminates the dependency on Clang >= 3.9.Todo: