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

Redo FFI bindings using rust-bindgen and move them into a unique portaudio-sys crate along with the build script. #137

Open
mitchmindtree opened this issue May 20, 2016 · 8 comments

Comments

@mitchmindtree
Copy link
Member

Many users have been experiencing issues with the bindings, particularly with the Linux ALSA backend. As the ffi.rs module was written by hand quite a while ago, there's a chance that some error might have been introduced due to human error. By generating the bindings using rust-bindgen we can have higher assurance that the struct layouts are correct and that we aren't missing bindings to any parts of the API.

By moving the bindings into their own crate portaudio-sys we can isolate FFI related issues and hopefully achieve slightly faster compilation. We could also move the build script into this crate in order to keep rust-portaudio clean and focused on the rust-esque abstractions, while portaudio-sys takes care of building the C portaudio lib and FFI. All bindings to platform-specific extensions could be done in this crate also.
#128, #136 and #133 may be related.

@joe-hauns
Copy link

I guess this can be marked as resolved now due to merging #152 .

@mitchmindtree
Copy link
Member Author

Almost, I just realised we won't be able to publish the changes without first publishing the portaudio-sys crate. I believe that in order to publish using cargo, all dependencies must also be on crates.io. I'd publish it as portaudio-sys, however it already exists.

Our options are:

  1. Publish our generated bindings under a different name like rust-portaudio-sys.
  2. See if we can use the existing portaudio-sys crate. If they too used rust-bindgen, then I imagine their bindings may almost be identical.

I think option 2 would be nice to avoid more fragmentation and share the maintenance burden, however they don't have a linked repository on crates.io so I'm unsure how easy it would be to collaborate or add patches if we need them.

@cdghibaudo
Copy link
Contributor

For reference, the bindgen command I used is here and everything after /* automatically generated by rust-bindgen */ is untouched.

I gave a quick look at portaudio-sys, I’d say it is less like the default output. Might be because of different parameters, a couple of manual edits, or they used an older version of bindgen. The biggest difference is the use of libc.

I don’t remember why I constified some enums exactly, because of the way error codes are handled maybe.

To answer a previous message, libc is still used by Buffer in stream.rs which needs malloc and free.

@joe-hauns
Copy link

I just found the other repo. @mitchmindtree I definitely agree that sharing the burden and preventing fragmentation is reasonable but i can't estimate whether it's a lot of work to integrate the other bindings. I think it might turn out to as much afford as #152. @cdghibaudo what do you think?

@joe-hauns
Copy link

joe-hauns commented May 26, 2017

Just thought about it again: Isn't it maybe better to just make our portaudio-sys an internal module (not a seperate crate)?
Basically I think the bindgen generated part shouldn't be edited at all, to make sure that no ffi-related bugs occur. Since the ffi shouldn't change noone would have to maintain the ffi.
Furthermore if we would do edit the ffi for some reason we would not have to watch out for compability with portaudio-rs.

@mitchmindtree
Copy link
Member Author

mitchmindtree commented May 26, 2017 via email

@cdghibaudo
Copy link
Contributor

I agree, it isn’t much of a burden.

@dash1291
Copy link

dash1291 commented Jun 7, 2020

I came here following from #128 because I'm facing the same issue I think. Wondering whether this discussion was concluded.

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

No branches or pull requests

4 participants