Re: [PATCH v2 5/5] rust: generate bindings via cbindgen

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> [snip]
>
> diff --git a/meson.build b/meson.build
> index 308798e861..b4acc417ad 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -523,6 +523,7 @@ libgit_sources = [
>    'usage.c',
>    'userdiff.c',
>    'utf8.c',
> +  'varint.c',
>    'version.c',
>    'versioncmp.c',
>    'walker.c',
> @@ -1704,7 +1705,9 @@ version_def_h = custom_target(
>  libgit_sources += version_def_h
>  
>  cargo = find_program('cargo', dirs: program_path, native: true, required: get_option('rust'))
> -rust_option = get_option('rust').disable_auto_if(not cargo.found())
> +cbindgen = find_program('cbindgen', dirs: program_path, native: true, required: get_option('rust'))
> +
> +rust_option = get_option('rust').disable_auto_if(not cargo.found() or not cbindgen.found())

This means to compile with Rust we not only need cargo, but also
cbindgen. As we ideally want to have a broad platform support, would
adding another dependency (i.e. `cbindgen`) narrow the platforms we'd
eventually support? Or can we consider platforms supporting Rust also
have cbindgen?

> [snip]
>
> diff --git a/varint.c b/varint.c
> index 03cd54416b..1ed738a756 100644
> --- a/varint.c
> +++ b/varint.c
> @@ -1,6 +1,14 @@
>  #include "git-compat-util.h"
>  #include "varint.h"
>  
> +/*
> + * When building with Rust we don't compile the C code, but we only verify
> + * whether the function signatures of our C bindings match the ones we have
> + * declared in "varint.h".
> + */
> +#ifdef WITH_RUST
> +# include "c-bindings.h"

So when we rewrite more subsystems into Rust, this will include
definitions from all those subsystems into this compilation unit.
If one subsystem with a Rust alternative implementation includes the
header from another subsystem with a Rust-alternative implementation,
the function signatures are checked twice, and errors are surfaced
twice. I guess this is a tradeoff we can accept for now, because:

* We only have one subsystem in Rust now.
* The approach in this patch simplifies the build setup.

We might revisit that at some point, but I can agree this is the most
sensical approach for now.

-- 
Cheers,
Toon