Re: [PATCH 3/3] rust: generate bindings via cbindgen

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 24, 2025, 06:37 UTC
Message-ID: <aPsepOtUf92fqDL-@pks.im>
In-Reply-To: <CAH=ZcbADTLvTioBf+LYQej1G0biZM8s3-iJG+BZjnpxj+8NjsA@mail.gmail.com>

On Thu, Oct 23, 2025 at 12:00:21PM -0600, Ezekiel Newren wrote:

Show 13 quoted lines

> On Thu, Oct 23, 2025 at 1:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > +C_BINDINGS = c-bindings.h
> > +
> > +GENERATED_H += $(C_BINDINGS)
> > +
> > +$(C_BINDINGS): cbindgen.toml $(RUST_SOURCES)
> > +       $(QUIET_CBINDGEN)cbindgen --output $@
> > +
> 
> My preference is to store all generated header files from cbindgen
> under interop/ that way we don't pollute the root of Git. It also
> makes ignoring generated header files easier (if we choose to) since
> we'd only need to specify `/interop/` in .gitignore.

It's (unfortunately) common practice to generate headers all over the place in Git. So I'd propose that until we grow a second interop header we keep it in the root hierarchy.

The proper fix for this in my opinion is out-of-tree builds. In Meson for example we don't have to worry about this issue at all. If we wanted to fix this more broadly for our Makefile, as well, I'd propose to instead introduce a "generated/" directory and move all of our generated headers into it.

> For platforms that can't compile Rust how are they going to build C if
> they depend on those generated headers?

For now they don't have to care about this, as the interop header is only included in case we build with Rust support enabled.

Show 18 quoted lines

> > diff --git a/cbindgen.toml b/cbindgen.toml
> > new file mode 100644
> > index 00000000000..ba4b2d63672
> > --- /dev/null
> > +++ b/cbindgen.toml
> > @@ -0,0 +1,7 @@
> > +language = "C"
> > +
> > +# Don't include standard C headers. These are managed by "git-compat-util.h".
> > +no_includes = true
> > +
> > +# Use plain structs instead of using typedefs.
> > +style = "tag"
> 
> This is what my cbindgen.toml file looked like.
>
> ```
> sys_includes = ["git-compat-util.h"]

Our headers generally don't include "git-compat-util.h". The rule is that it should always be included first by our code files. So I think we should stick to that rule for this generated file, as well.

> autogen_warning = "/* Warning, this file is autogenerated by cbindgen.
> Don't modify this manually. */"

Makes sense.

> language = "C"
> no_includes = true
> style = "tag"

Yup, also got these.

> usize_is_size_t = true

Ah, this feels like the right thing to do, agreed.

> tab_width = 4
> 
> tab_width is the number of spaces, there is no option to use a tab
> character in cbindgen.

Shouldn't this rather use 8 then to match our coding style better, even if it's not a perfect match?

> [parse]
> parse_deps = false

This is the default according to cbindgen's documentation, so I'll not include this line.

Show 23 quoted lines

> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index b7b3cf35edf..3bce6f47f87 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -37,7 +37,7 @@ fedora-*|almalinux-*)
> >                 MESON_DEPS="meson ninja";;
> >         esac
> >         dnf -yq update >/dev/null &&
> > -       dnf -yq install shadow-utils sudo make pkg-config gcc findutils diffutils perl python3 gawk gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel $MESON_DEPS cargo >/dev/null
> > +       dnf -yq install shadow-utils sudo make pkg-config gcc findutils diffutils perl python3 gawk gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel $MESON_DEPS cargo cbindgen >/dev/null
> >         ;;
> >  ubuntu-*|i386/debian-*|debian-*)
> >         # Required so that apt doesn't wait for user input on certain packages.
> > @@ -64,7 +64,7 @@ ubuntu-*|i386/debian-*|debian-*)
> >                 make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
> >                 tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
> >                 libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
> > -               libsecret-1-dev libpcre2-dev meson ninja-build pkg-config cargo \
> > +               libsecret-1-dev libpcre2-dev meson ninja-build pkg-config cargo cbindgen \
> >                 ${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
> 
> cbindgen is a Rust crate and it should be specified in the Cargo.toml
> under [build-dependencies] block.

What is the benefit for us? The generated code is not a dependency of the Rust code, and neither do we use it via "build.rs". And if we use cbindgen via "Cargo.toml" we'd be forced to build it first, which slows down our CI jobs.

Please let me know in case I miss any reasons to have it in our build dependencies instead.

> Also I think that we should convert from using a Cargo crate to using
> a Cargo workspace, so that we can generate multiple header files (1
> per crate) rather than a monolithic generated header. This is because
> cbindgen operates at the granularity of a crate.

I'm basically still punting this into the future. There is no good reason yet to have workspaces, so I'd rather introduce them once we have a better way to sell them and demonstrate their immediate benefits.

Patrick