From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 30 Oct 2025 09:51:01 GMT
Subject: Re: [PATCH v2 5/5] rust: generate bindings via cbindgen
Message-ID: <aQM1Bdwrf0tEluEM@pks.im>
In-Reply-To: <87v7k4pffg.fsf@iotcl.com>

On Fri, Oct 24, 2025 at 04:01:07PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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?

cbindgen is written in Rust, so as long as you have Rust you should also
be able to have cbindgen. The obvious catch though is that cbindgen also
has a minimum supported Rust version, so we need to ensure that the
version of cbindgen that we pick is supported by our MSRV.

> > 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.

Yeah. I guess this is also something that may be helped by introducing
Rust workspaces as Ezekiel proposes. But for now it's going to be good
enough, I think.

Patrick