diwic / alsa-sys

Rust raw FFI bindings for ALSA
MIT License
18 stars 12 forks source link

Add use-bindgen feature #7

Closed cyberia-ng closed 3 years ago

cyberia-ng commented 3 years ago

Addresses https://github.com/diwic/alsa-sys/issues/2

Points of discussion:

diwic commented 3 years ago

Hi and thanks for working on this feature!

On my system, the generated bindings in generated.rs are different from what is committed. I'm not sure whether this is because I have different header files to whoever ran the regenerate-bindings.sh script last, or because I have specified the bindgen options wrong, or something else.

Okay, so let's see if I can help you to sort these differences out.

1) I ran bindgen last with 0.53.2 and just upgraded to 0.56.0. This changes a few u32 to c_uint and str to str_, which seems to be reasonable changes.

2) I ran bindgen against git master of git://git.alsa-project.org/alsa-lib.git - not the installed system headers. This adds the doc comments, Tstamp_type constants and a few more things.

So, my regenerate_bindings.sh looks like this:

bindgen --size_t-is-usize --no-recursive-whitelist --no-prepend-enum-name --no-layout-tests \
--generate "functions,types" --whitelist-function="snd_.*" --whitelist-type="_?snd_.*" \
../../C/alsa-lib/include/alsa/asoundlib.h -o src/generated.rs -- -I../../C/alsa-lib/include/

I've updated src/generated.rs to bindgen 0.56.0. Hopefully you should be able to get the same output now, given that you run it against git master.

Edit: For reference: I'm running Ubuntu 20.04, my version of clang is 1:10.0-50~exp1, and with git master I mean git://git.alsa-project.org/alsa-lib.git whose master branch is currently at 55d59821ffb8.

diwic commented 3 years ago

I have deleted the old regenerate_bindings.sh script, because maintaining the bindgen arguments in two places seemed like it would be prone to failure. To regenerate the bindings now, you just run cargo build --features "use-bindgen" and the bindings are generated by the build script.

The argument makes sense, but I would not expect cargo build to change the source directory, this seems like bad practice. Instead it should generate it in OUT_DIR and include OUT_DIR's generated.rs instead of the ordinary one if the use-bindgen feature is set.

One could then imagine a regenerate_bindings script that would just call cargo build and then copy the resulting generated.rs into the src directory.

cyberia-ng commented 3 years ago

How's the latest commit?

cyberia-ng commented 3 years ago

You can use PKG_CONFIG_PATH=$(realpath ../alsa-sys/utils) (or similar) now, to point to the alsa.pc file in the checked-out master repo. Just make sure that you ran ./configure --prefix=$(realpath .) in there, otherwise pkg-config will still try to use your system include path.

diwic commented 3 years ago

You can use PKG_CONFIG_PATH=$(realpath ../alsa-sys/utils) (or similar) now, to point to the alsa.pc file in the checked-out master repo. Just make sure that you ran ./configure --prefix=$(realpath .) in there, otherwise pkg-config will still try to use your system include path.

Maybe this can be documented in the readme file?

cyberia-ng commented 3 years ago

Actually on reflection I think I want to rework this. The docs say

Build scripts may save any output files in the directory specified in the OUT_DIR environment variable. Scripts should not modify any files outside of that directory.

So it would probably be better to make regenerate-bindings.sh find the correct output directory and copy the file from there.

Something like

# Find most recently generated bindings file
generated_rs=$(find target -path '*/alsa-sys-*/generated.rs' -type f -printf '%T@ %P\n' | sort -n | awk '{print $2}' | head -n 1)
# Copy it to src/
cp "${generated_rs}" src/generated.rs

That find | sort | awk | head chain is a bit ugly, but better to stick to the docs behaviour for build.rs I think.

Edit: Plus, this way we wouldn't have all these special environment variables flying about.

diwic commented 3 years ago

That find | sort | awk | head chain is a bit ugly, but better to stick to the docs behaviour for build.rs I think.

Maybe we can, from the build script, output the full path of the generated file to the console? And somehow catch that in regenerate_bindings.sh, maybe by running run cargo with -vv?

cyberia-ng commented 3 years ago

How's this? We use a single-purpose binary which can access env!("OUT_DIR"), and thereby copy the generated bindings into src/

I also updated the Readme to say this.

cyberia-ng commented 3 years ago

I am also fine to do it with an output from the build.rs and cargo -vv if you think that is cleaner. I have no preference.

diwic commented 3 years ago

How's this? We use a single-purpose binary which can access env!("OUT_DIR"), and thereby copy the generated bindings into src/

I also updated the Readme to say this.

This is better, so I merged it as is. Thanks!

cyberia-ng commented 3 years ago

Great! Thanks for all the useful feedback and for the merge :)