git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] rust: generate bindings via cbindgen
@ 2025-10-23  7:17 Patrick Steinhardt
  2025-10-23  7:17 ` [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
                   ` (3 more replies)
  0 siblings, 4 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-23  7:17 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren

Hi,

this small patch series introduces support for cbindgen(1). This tool is
used to generate C headers from `extern "C"` blocks so that Rust code
can easily be called from C code.

For now, the only use case is to verify that our varint reimplementation
matches the C implementation. But later on this can and will be used to
call Rust-specific features from C, as well.

The topic is built on top of c54a18ef67 (The twenty-second batch,
2025-10-22) with ps/ci-rust at e509b5b8be (rust: support for Windows,
2025-10-15) merged into it.

Thanks!

Patrick

---
Patrick Steinhardt (3):
      ci: use Debian instead of deprecated i386/ubuntu
      meson: rename Rust library target
      rust: generate bindings via cbindgen

 .github/workflows/main.yml |  3 +--
 .gitignore                 |  1 +
 .gitlab-ci.yml             |  2 +-
 Makefile                   | 14 +++++++++++---
 cbindgen.toml              |  7 +++++++
 ci/install-dependencies.sh | 10 +++++-----
 ci/lib.sh                  |  2 +-
 meson.build                | 25 ++++++++++++++++++++-----
 shared.mak                 |  1 +
 src/meson.build            |  2 +-
 varint.c                   |  9 +++++++++
 11 files changed, 58 insertions(+), 18 deletions(-)


---
base-commit: 8654b230d9afe1326340989dd7082997c672472e
change-id: 20251009-b4-pks-rust-cbindgen-d80779ed2269


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu
  2025-10-23  7:17 [PATCH 0/3] rust: generate bindings via cbindgen Patrick Steinhardt
@ 2025-10-23  7:17 ` Patrick Steinhardt
  2025-10-23 17:56   ` Junio C Hamano
  2025-10-23  7:17 ` [PATCH 2/3] meson: rename Rust library target Patrick Steinhardt
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-23  7:17 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren

Ubuntu has ended support for 32 bit platforms and is not maintaining any
release anymore that has 32 bit support. But we still use i386/ubuntu in
our CI pipeline to test for compatibility with 32 bit systems, even
though that specific image does not receive updates anymore.

Besides being end-of-life, this image also doesn't have all packages
available to it anymore. This creates problems with a subsequent patch,
where we're about to pull in cbindgen for generating Rust to C bindings.

Drop the Ubuntu image and use Debian instead, which continues to
maintain its 32 bit port.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 3 +--
 .gitlab-ci.yml             | 2 +-
 ci/install-dependencies.sh | 6 +++---
 ci/lib.sh                  | 2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index cc54824c388..0b16970cd7e 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -395,9 +395,8 @@ jobs:
           cc: gcc
         - jobname: linux-musl-meson
           image: alpine:latest
-        # Supported until 2025-04-02.
         - jobname: linux32
-          image: i386/ubuntu:focal
+          image: i386/debian:latest
         # A RHEL 8 compatible distro.  Supported until 2029-05-31.
         - jobname: almalinux-8
           image: almalinux:8
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index b419a84e2cc..85feebf72d6 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -63,7 +63,7 @@ test:linux:
       - jobname: linux-musl-meson
         image: alpine:latest
       - jobname: linux32
-        image: i386/ubuntu:20.04
+        image: i386/debian:latest
       - jobname: linux-meson
         image: ubuntu:rolling
         CC: gcc
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 50628ee2dd6..b7b3cf35edf 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -39,7 +39,7 @@ fedora-*|almalinux-*)
 	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
 	;;
-ubuntu-*|i386/ubuntu-*|debian-*)
+ubuntu-*|i386/debian-*|debian-*)
 	# Required so that apt doesn't wait for user input on certain packages.
 	export DEBIAN_FRONTEND=noninteractive
 
@@ -48,9 +48,9 @@ ubuntu-*|i386/ubuntu-*|debian-*)
 		SVN='libsvn-perl subversion'
 		LANGUAGES='language-pack-is'
 		;;
-	i386/ubuntu-*)
+	i386/debian-*)
 		SVN=
-		LANGUAGES='language-pack-is'
+		LANGUAGES='locales-all'
 		;;
 	*)
 		SVN='libsvn-perl subversion'
diff --git a/ci/lib.sh b/ci/lib.sh
index f561884d401..865a0675371 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -250,7 +250,7 @@ then
 		CI_OS_NAME=osx
 		JOBS=$(nproc)
 		;;
-	*,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
+	*,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/debian:*)
 		CI_OS_NAME=linux
 		JOBS=$(nproc)
 		;;

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 2/3] meson: rename Rust library target
  2025-10-23  7:17 [PATCH 0/3] rust: generate bindings via cbindgen Patrick Steinhardt
  2025-10-23  7:17 ` [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
@ 2025-10-23  7:17 ` Patrick Steinhardt
  2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
  3 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-23  7:17 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren

Rename the Rust library target from `git_rs` to `rust`. The latter is
way easier to remember if one wants to compile only that target via
`meson compile rust`. Furthermore, this name matches the test target
that we have for Rust that can be invoked via `meson test rust`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 src/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/meson.build b/src/meson.build
index 25b9ad5a147..1c73549696c 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -17,7 +17,7 @@ if get_option('buildtype') == 'release'
   cargo_command += '--release'
 endif
 
-libgit_rs = custom_target('git_rs',
+libgit_rs = custom_target('rust',
   input: libgit_rs_sources + [
     meson.project_source_root() / 'Cargo.toml',
   ],

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23  7:17 [PATCH 0/3] rust: generate bindings via cbindgen Patrick Steinhardt
  2025-10-23  7:17 ` [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
  2025-10-23  7:17 ` [PATCH 2/3] meson: rename Rust library target Patrick Steinhardt
@ 2025-10-23  7:17 ` Patrick Steinhardt
  2025-10-23 18:00   ` Ezekiel Newren
                     ` (3 more replies)
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
  3 siblings, 4 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-23  7:17 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren

When compiling Git with Rust enabled we replace our C implementation of
the varint encoding with a Rust implementation. A prerequisite for doing
so is of course that the interfaces for both implementations are exactly
the same. If they aren't, then we risk subtle runtime errors.

We don't really have a way to detect such interface mismatches though:
the code will happily compile if we change either of the implementations
without adjusting the other implementation in the same spirit. The risk
of divergence is low right now as we only replace a single subsystem.
But it is expected that we'll grow more reimplementations over time, so
it is bound to increase.

A related issue is that we don't have an easy way to implement features
exclusively in Rust and make them available to our C library. Again, we
don't have such features yet, but there are work-in-progress patch
series that will eventually add them.

Both of these issues can be addressed by generating C bindings via the
cbindgen(1) tool: given a Rust crate, it extracts all functions marked
with `extern "C"` and creates a C declaration for them. These are then
written into a header file that we can include.

Set up this infrastructure in both our Makefile and in Meson. To
demonstrate its use, the generated "c-bindings.h" header is included in
"varint.c". If we now adapt "varint.rs" to have a different function
signature than the C code we'll now get a compiler error:

    In file included from ../varint.c:10:
    ./c-bindings.h:10:10: error: conflicting types for 'decode_varint'
       10 | uint32_t decode_varint(const uint8_t **bufp);
          |          ^
    ../varint.h:5:10: note: previous declaration is here
        5 | uint64_t decode_varint(const unsigned char **);

An initial version instead included the bindings in "varint.h". But that
would cause us to recompile all dependents of "varint.h" every time the
signatures of exported Rust functions change. So instead, we now include
it in "varint.c" and compile that file unconditionally again.

Adapt our CI to install cbindgen(1) accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitignore                 |  1 +
 Makefile                   | 14 +++++++++++---
 cbindgen.toml              |  7 +++++++
 ci/install-dependencies.sh |  4 ++--
 meson.build                | 25 ++++++++++++++++++++-----
 shared.mak                 |  1 +
 varint.c                   |  9 +++++++++
 7 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore
index 78a45cb5bec..20558c9dc8a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -197,6 +197,7 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
+/c-bindings.h
 /config-list.h
 /command-list.h
 /hook-list.h
diff --git a/Makefile b/Makefile
index 0bf5f17a90c..1213a0fc960 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,9 +1326,7 @@ LIB_OBJS += urlmatch.o
 LIB_OBJS += usage.o
 LIB_OBJS += userdiff.o
 LIB_OBJS += utf8.o
-ifndef WITH_RUST
 LIB_OBJS += varint.o
-endif
 LIB_OBJS += version.o
 LIB_OBJS += versioncmp.o
 LIB_OBJS += walker.o
@@ -1562,6 +1560,14 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 ifdef WITH_RUST
 BASIC_CFLAGS += -DWITH_RUST
 GITLIBS += $(RUST_LIB)
+
+C_BINDINGS = c-bindings.h
+
+GENERATED_H += $(C_BINDINGS)
+
+$(C_BINDINGS): cbindgen.toml $(RUST_SOURCES)
+	$(QUIET_CBINDGEN)cbindgen --output $@
+
 ifeq ($(uname_S),Windows)
 EXTLIBS += -luserenv
 endif
@@ -2619,6 +2625,8 @@ PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
 pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
+varint.sp varint.s varint.o: $(C_BINDINGS)
+
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
 	$(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@)
 
@@ -3806,7 +3814,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
 	$(RM) $(HCC)
-	$(RM) -r Cargo.lock target/
+	$(RM) -r Cargo.lock target/ $(C_BINDINGS)
 	$(RM) version-def.h
 	$(RM) -r $(dep_dirs) $(compdb_dir) compile_commands.json
 	$(RM) $(test_bindir_programs)
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"
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
 
 	# Starting with Ubuntu 25.10, sudo can now be provided via either
diff --git a/meson.build b/meson.build
index 308798e861b..b4acc417ad4 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())
 if rust_option.allowed()
   subdir('src')
   libgit_c_args += '-DWITH_RUST'
@@ -1712,10 +1715,22 @@ if rust_option.allowed()
   if host_machine.system() == 'windows'
     libgit_dependencies += compiler.find_library('userenv')
   endif
-else
-  libgit_sources += [
-    'varint.c',
-  ]
+
+  cbindgen_input = [ 'cbindgen.toml' ]
+  foreach source : libgit_rs_sources
+    cbindgen_input += 'src' / source
+  endforeach
+
+  libgit_sources += custom_target('c-bindings.h',
+    input: cbindgen_input,
+    output: 'c-bindings.h',
+    command: [
+      cbindgen,
+      '--output',
+      '@OUTPUT@',
+      meson.current_source_dir(),
+    ],
+  )
 endif
 
 libgit = declare_dependency(
diff --git a/shared.mak b/shared.mak
index 0e7492076eb..598e58e069c 100644
--- a/shared.mak
+++ b/shared.mak
@@ -57,6 +57,7 @@ ifndef V
 
 ## Used in "Makefile"
 	QUIET_CARGO    = @echo '   ' CARGO $@;
+	QUIET_CBINDGEN = @echo '   ' CBINDGEN $@;
 	QUIET_CC       = @echo '   ' CC $@;
 	QUIET_AR       = @echo '   ' AR $@;
 	QUIET_LINK     = @echo '   ' LINK $@;
diff --git a/varint.c b/varint.c
index 03cd54416b6..1ed738a756c 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"
+#else
 uint64_t decode_varint(const unsigned char **bufp)
 {
 	const unsigned char *buf = *bufp;
@@ -28,3 +36,4 @@ uint8_t encode_varint(uint64_t value, unsigned char *buf)
 		memcpy(buf, varint + pos, sizeof(varint) - pos);
 	return sizeof(varint) - pos;
 }
+#endif

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu
  2025-10-23  7:17 ` [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
@ 2025-10-23 17:56   ` Junio C Hamano
  2025-10-24  6:36     ` Patrick Steinhardt
  0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2025-10-23 17:56 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Ezekiel Newren

Patrick Steinhardt <ps@pks.im> writes:

> Ubuntu has ended support for 32 bit platforms and is not maintaining any
> release anymore that has 32 bit support. But we still use i386/ubuntu in
> our CI pipeline to test for compatibility with 32 bit systems, even
> though that specific image does not receive updates anymore.
>
> Besides being end-of-life, this image also doesn't have all packages
> available to it anymore. This creates problems with a subsequent patch,
> where we're about to pull in cbindgen for generating Rust to C bindings.
>
> Drop the Ubuntu image and use Debian instead, which continues to
> maintain its 32 bit port.

Thanks, this is long overdue.

Would this have nagative interactions with our recent tweak for
sudo-rust vs sudo-C, which I thought was only releavant for Ubuntu?

I guess as long as i386/debian does not have /etc/alternatives/sudo
we should be safe, and we also handle debian-* (presumably 64-bit)
in the same case arm, so this should not be a new problem.  Just
double checking.

> -        # Supported until 2025-04-02.
>          - jobname: linux32
> -          image: i386/ubuntu:focal
> +          image: i386/debian:latest

Will queue.  Thanks.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
@ 2025-10-23 18:00   ` Ezekiel Newren
  2025-10-24  6:37     ` Patrick Steinhardt
  2025-10-23 21:42   ` Junio C Hamano
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-23 18:00 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson

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. For platforms
that can't compile Rust how are they going to build C if they depend
on those generated headers?

> 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"]

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

language = "C"
no_includes = true
usize_is_size_t = true
style = "tag"
tab_width = 4

[parse]
parse_deps = false
```
tab_width is the number of spaces, there is no option to use a tab
character in cbindgen.

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
  2025-10-23 18:00   ` Ezekiel Newren
@ 2025-10-23 21:42   ` Junio C Hamano
  2025-10-23 22:01   ` Junio C Hamano
  2025-10-23 22:37   ` Junio C Hamano
  3 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2025-10-23 21:42 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Ezekiel Newren

Patrick Steinhardt <ps@pks.im> writes:

> Set up this infrastructure in both our Makefile and in Meson. To
> demonstrate its use, the generated "c-bindings.h" header is included in
> "varint.c". If we now adapt "varint.rs" to have a different function
> signature than the C code we'll now get a compiler error:
>
>     In file included from ../varint.c:10:
>     ./c-bindings.h:10:10: error: conflicting types for 'decode_varint'
>        10 | uint32_t decode_varint(const uint8_t **bufp);
>           |          ^
>     ../varint.h:5:10: note: previous declaration is here
>         5 | uint64_t decode_varint(const unsigned char **);
>
> An initial version instead included the bindings in "varint.h". But that
> would cause us to recompile all dependents of "varint.h" every time the
> signatures of exported Rust functions change. So instead, we now include
> it in "varint.c" and compile that file unconditionally again.
>
> Adapt our CI to install cbindgen(1) accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

OK.  I am getting this out of "make c-bindings.h", which looks
quite reasonable.

/**
 * Decode the variable-length integer stored in `bufp` and return the decoded value.
 *
 * Returns 0 in case the decoded integer would overflow u64::MAX.
 *
 * # Safety
 *
 * The buffer must be NUL-terminated to ensure safety.
 */
uint64_t decode_varint(const uint8_t **bufp);

/**
 * Encode `value` into `buf` as a variable-length integer unless `buf` is null.
 *
 * Returns the number of bytes written, or, if `buf` is null, the number of bytes that would be
 * written to encode the integer.
 *
 * # Safety
 *
 * `buf` must either be null or point to at least 16 bytes of memory.
 */
uint8_t encode_varint(uint64_t value, uint8_t *buf);

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
  2025-10-23 18:00   ` Ezekiel Newren
  2025-10-23 21:42   ` Junio C Hamano
@ 2025-10-23 22:01   ` Junio C Hamano
  2025-10-23 22:37   ` Junio C Hamano
  3 siblings, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2025-10-23 22:01 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Ezekiel Newren

Patrick Steinhardt <ps@pks.im> writes:

> An initial version instead included the bindings in "varint.h". But that
> would cause us to recompile all dependents of "varint.h" every time the
> signatures of exported Rust functions change. So instead, we now include
> it in "varint.c" and compile that file unconditionally again.
>
> Adapt our CI to install cbindgen(1) accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

I am debating myself if we want a patch like this.  I tend to prefer
"make clean" not to be too specific to the build options used to
leave crufts.


 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git i/Makefile w/Makefile
index 59e5a2c61c..9b673865e5 100644
--- i/Makefile
+++ w/Makefile
@@ -1558,12 +1558,13 @@ endif
 ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND)
 ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 
+# allow "make WITH_RUST=Yes && make clean" to discard it
+C_BINDINGS = c-bindings.h
+
 ifdef WITH_RUST
 BASIC_CFLAGS += -DWITH_RUST
 GITLIBS += $(RUST_LIB)
 
-C_BINDINGS = c-bindings.h
-
 GENERATED_H += $(C_BINDINGS)
 
 $(C_BINDINGS): cbindgen.toml $(RUST_SOURCES)

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
                     ` (2 preceding siblings ...)
  2025-10-23 22:01   ` Junio C Hamano
@ 2025-10-23 22:37   ` Junio C Hamano
  2025-10-24  6:36     ` Patrick Steinhardt
  3 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2025-10-23 22:37 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Ezekiel Newren

Patrick Steinhardt <ps@pks.im> writes:

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

A single ~120 columns wide should be line-wrapped.  This seems to
fail with almalinux-8 CI job due to lack of the package.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu
  2025-10-23 17:56   ` Junio C Hamano
@ 2025-10-24  6:36     ` Patrick Steinhardt
  0 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  6:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, brian m. carlson, Ezekiel Newren

On Thu, Oct 23, 2025 at 10:56:01AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Ubuntu has ended support for 32 bit platforms and is not maintaining any
> > release anymore that has 32 bit support. But we still use i386/ubuntu in
> > our CI pipeline to test for compatibility with 32 bit systems, even
> > though that specific image does not receive updates anymore.
> >
> > Besides being end-of-life, this image also doesn't have all packages
> > available to it anymore. This creates problems with a subsequent patch,
> > where we're about to pull in cbindgen for generating Rust to C bindings.
> >
> > Drop the Ubuntu image and use Debian instead, which continues to
> > maintain its 32 bit port.
> 
> Thanks, this is long overdue.
> 
> Would this have nagative interactions with our recent tweak for
> sudo-rust vs sudo-C, which I thought was only releavant for Ubuntu?
> 
> I guess as long as i386/debian does not have /etc/alternatives/sudo
> we should be safe, and we also handle debian-* (presumably 64-bit)
> in the same case arm, so this should not be a new problem.  Just
> double checking.

Yup, exactly. Debian continues to use the old sudo implementation, and
it doesn't have /etc/alternatives/sudo at the current point in time.

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23 22:37   ` Junio C Hamano
@ 2025-10-24  6:36     ` Patrick Steinhardt
  0 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  6:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, brian m. carlson, Ezekiel Newren

On Thu, Oct 23, 2025 at 03:37:58PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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
> >  	;;
> 
> A single ~120 columns wide should be line-wrapped.  This seems to
> fail with almalinux-8 CI job due to lack of the package.

Thanks, that's a test gap in GitLab CI then. Will fix the issue and the
gap.

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-23 18:00   ` Ezekiel Newren
@ 2025-10-24  6:37     ` Patrick Steinhardt
  2025-10-27 20:35       ` Ezekiel Newren
  0 siblings, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  6:37 UTC (permalink / raw)
  To: Ezekiel Newren; +Cc: git, brian m. carlson

On Thu, Oct 23, 2025 at 12:00:21PM -0600, Ezekiel Newren wrote:
> 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.

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

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 0/5] rust: generate bindings via cbindgen
  2025-10-23  7:17 [PATCH 0/3] rust: generate bindings via cbindgen Patrick Steinhardt
                   ` (2 preceding siblings ...)
  2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
@ 2025-10-24  9:51 ` Patrick Steinhardt
  2025-10-24  9:51   ` [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order Patrick Steinhardt
                     ` (5 more replies)
  3 siblings, 6 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  9:51 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

Hi,

this small patch series introduces support for cbindgen(1). This tool is
used to generate C headers from `extern "C"` blocks so that Rust code
can easily be called from C code.

For now, the only use case is to verify that our varint reimplementation
matches the C implementation. But later on this can and will be used to
call Rust-specific features from C, as well.

The topic is built on top of c54a18ef67 (The twenty-second batch,
2025-10-22) with ps/ci-rust at e509b5b8be (rust: support for Windows,
2025-10-15) merged into it.

Changes in v2:
  - Take some of the pieces from Ezekiel's "cbindgen.toml" file. I have
    not yet taken `usize_is_size_t`, as that option requires cbindgen
    v0.16.0, which is not available on Ubuntu 20.04.
  - Backfill missing jobs for GitLab CI.
  - Fix CI failures on Alma Linux 8 because cbindgen isn't available
    there.
  - Link to v1: https://lore.kernel.org/r/20251023-b4-pks-rust-cbindgen-v1-0-c19b61b03127@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (5):
      gitlab-ci: reorder Linux job matrix to match GitHub's order
      gitlab-ci: backfill missing Linux jobs
      ci: use Debian instead of deprecated i386/ubuntu
      meson: rename Rust library target
      rust: generate bindings via cbindgen

 .github/workflows/main.yml |  3 +--
 .gitignore                 |  1 +
 .gitlab-ci.yml             | 22 ++++++++++++++--------
 Makefile                   | 14 +++++++++++---
 cbindgen.toml              | 13 +++++++++++++
 ci/install-dependencies.sh | 16 +++++++++++-----
 ci/lib.sh                  |  2 +-
 meson.build                | 25 ++++++++++++++++++++-----
 shared.mak                 |  1 +
 src/meson.build            |  2 +-
 varint.c                   |  9 +++++++++
 11 files changed, 83 insertions(+), 25 deletions(-)

Range-diff versus v1:

-:  ---------- > 1:  81e7677f3a gitlab-ci: reorder Linux job matrix to match GitHub's order
-:  ---------- > 2:  c367154007 gitlab-ci: backfill missing Linux jobs
1:  740702529c ! 3:  a1d7260326 ci: use Debian instead of deprecated i386/ubuntu
    @@ .gitlab-ci.yml: test:linux:
            - jobname: linux32
     -        image: i386/ubuntu:20.04
     +        image: i386/debian:latest
    -       - jobname: linux-meson
    -         image: ubuntu:rolling
    -         CC: gcc
    +       # A RHEL 8 compatible distro.  Supported until 2029-05-31.
    +       - jobname: almalinux-8
    +         image: almalinux:8
     
      ## ci/install-dependencies.sh ##
     @@ ci/install-dependencies.sh: fedora-*|almalinux-*)
    @@ ci/lib.sh: then
      		CI_OS_NAME=osx
      		JOBS=$(nproc)
      		;;
    --	*,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
    -+	*,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/debian:*)
    +-	*,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
    ++	*,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/debian:*)
      		CI_OS_NAME=linux
      		JOBS=$(nproc)
      		;;
2:  ff696cfb2e = 4:  490c8a4d45 meson: rename Rust library target
3:  67a9f353df ! 5:  8b7a2469b2 rust: generate bindings via cbindgen
    @@ cbindgen.toml (new)
     @@
     +language = "C"
     +
    ++# Write a warning into the generated file.
    ++autogen_warning = "/* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */"
    ++
     +# 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"
    ++
    ++# Match our coding style more closely.
    ++tab_width = 8
     
      ## ci/install-dependencies.sh ##
    -@@ ci/install-dependencies.sh: fedora-*|almalinux-*)
    +@@ ci/install-dependencies.sh: alpine-*)
    + 		bash cvs gnupg perl-cgi perl-dbd-sqlite perl-io-tty >/dev/null
    + 	;;
    + fedora-*|almalinux-*)
    ++	RUST_DEPS="cargo cbindgen"
    + 	case "$jobname" in
    ++	almalinux-8)
    ++		# AlmaLinux 8 does not have cbindgen, it was only added in version 9.
    ++		RUST_DEPS=;;
    + 	*-meson)
      		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
    ++	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 $RUST_DEPS >/dev/null
      	;;
      ubuntu-*|i386/debian-*|debian-*)
      	# Required so that apt doesn't wait for user input on certain packages.

---
base-commit: 8654b230d9afe1326340989dd7082997c672472e
change-id: 20251009-b4-pks-rust-cbindgen-d80779ed2269


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
@ 2025-10-24  9:51   ` Patrick Steinhardt
  2025-10-28 19:14     ` Ezekiel Newren
  2025-10-24  9:51   ` [PATCH v2 2/5] gitlab-ci: backfill missing Linux jobs Patrick Steinhardt
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  9:51 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

We have mostly the exact same CI configuration as GitHub has for our
Linux jobs. It's harder than necessary though to compare them with one
another as the ordering is different between both.

Reorder the job matrix in GitLab CI to match GitHub's order.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index b419a84e2c..1dbf236b2c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -42,15 +42,15 @@ test:linux:
       - jobname: linux-reftable
         image: ubuntu:rolling
         CC: clang
+      - jobname: linux-TEST-vars
+        image: ubuntu:20.04
+        CC: gcc
+        CC_PACKAGE: gcc-8
       - jobname: linux-breaking-changes
         image: ubuntu:20.04
         CC: gcc
       - jobname: fedora-breaking-changes-meson
         image: fedora:latest
-      - jobname: linux-TEST-vars
-        image: ubuntu:20.04
-        CC: gcc
-        CC_PACKAGE: gcc-8
       - jobname: linux-leaks
         image: ubuntu:rolling
         CC: gcc
@@ -60,13 +60,13 @@ test:linux:
       - jobname: linux-asan-ubsan
         image: ubuntu:rolling
         CC: clang
+      - jobname: linux-meson
+        image: ubuntu:rolling
+        CC: gcc
       - jobname: linux-musl-meson
         image: alpine:latest
       - jobname: linux32
         image: i386/ubuntu:20.04
-      - jobname: linux-meson
-        image: ubuntu:rolling
-        CC: gcc
   artifacts:
     paths:
       - t/failed-test-artifacts

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 2/5] gitlab-ci: backfill missing Linux jobs
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
  2025-10-24  9:51   ` [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order Patrick Steinhardt
@ 2025-10-24  9:51   ` Patrick Steinhardt
  2025-10-28 19:15     ` Ezekiel Newren
  2025-10-24  9:51   ` [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  9:51 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

We're missing two Linux CI jobs in GitLab's CI that are present in
GitHub's CI. Backfill them to ensure that GitLab has the same test
coverage.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml | 6 ++++++
 ci/lib.sh      | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 1dbf236b2cd..f61ec2b6989 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -67,6 +67,12 @@ test:linux:
         image: alpine:latest
       - jobname: linux32
         image: i386/ubuntu:20.04
+      # A RHEL 8 compatible distro.  Supported until 2029-05-31.
+      - jobname: almalinux-8
+        image: almalinux:8
+      # Supported until 2026-08-31.
+      - jobname: debian-11
+        image: debian:11
   artifacts:
     paths:
       - t/failed-test-artifacts
diff --git a/ci/lib.sh b/ci/lib.sh
index f561884d401..a5c4eb40bea 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -250,7 +250,7 @@ then
 		CI_OS_NAME=osx
 		JOBS=$(nproc)
 		;;
-	*,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
+	*,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
 		CI_OS_NAME=linux
 		JOBS=$(nproc)
 		;;

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
  2025-10-24  9:51   ` [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order Patrick Steinhardt
  2025-10-24  9:51   ` [PATCH v2 2/5] gitlab-ci: backfill missing Linux jobs Patrick Steinhardt
@ 2025-10-24  9:51   ` Patrick Steinhardt
  2025-10-28 19:17     ` Ezekiel Newren
  2025-10-24  9:51   ` [PATCH v2 4/5] meson: rename Rust library target Patrick Steinhardt
                     ` (2 subsequent siblings)
  5 siblings, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  9:51 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

Ubuntu has ended support for 32 bit platforms and is not maintaining any
release anymore that has 32 bit support. But we still use i386/ubuntu in
our CI pipeline to test for compatibility with 32 bit systems, even
though that specific image does not receive updates anymore.

Besides being end-of-life, this image also doesn't have all packages
available to it anymore. This creates problems with a subsequent patch,
where we're about to pull in cbindgen for generating Rust to C bindings.

Drop the Ubuntu image and use Debian instead, which continues to
maintain its 32 bit port.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 3 +--
 .gitlab-ci.yml             | 2 +-
 ci/install-dependencies.sh | 6 +++---
 ci/lib.sh                  | 2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index cc54824c388..0b16970cd7e 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -395,9 +395,8 @@ jobs:
           cc: gcc
         - jobname: linux-musl-meson
           image: alpine:latest
-        # Supported until 2025-04-02.
         - jobname: linux32
-          image: i386/ubuntu:focal
+          image: i386/debian:latest
         # A RHEL 8 compatible distro.  Supported until 2029-05-31.
         - jobname: almalinux-8
           image: almalinux:8
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index f61ec2b6989..31d5e5a3e0d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -66,7 +66,7 @@ test:linux:
       - jobname: linux-musl-meson
         image: alpine:latest
       - jobname: linux32
-        image: i386/ubuntu:20.04
+        image: i386/debian:latest
       # A RHEL 8 compatible distro.  Supported until 2029-05-31.
       - jobname: almalinux-8
         image: almalinux:8
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 50628ee2dd6..b7b3cf35edf 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -39,7 +39,7 @@ fedora-*|almalinux-*)
 	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
 	;;
-ubuntu-*|i386/ubuntu-*|debian-*)
+ubuntu-*|i386/debian-*|debian-*)
 	# Required so that apt doesn't wait for user input on certain packages.
 	export DEBIAN_FRONTEND=noninteractive
 
@@ -48,9 +48,9 @@ ubuntu-*|i386/ubuntu-*|debian-*)
 		SVN='libsvn-perl subversion'
 		LANGUAGES='language-pack-is'
 		;;
-	i386/ubuntu-*)
+	i386/debian-*)
 		SVN=
-		LANGUAGES='language-pack-is'
+		LANGUAGES='locales-all'
 		;;
 	*)
 		SVN='libsvn-perl subversion'
diff --git a/ci/lib.sh b/ci/lib.sh
index a5c4eb40bea..fdfde612339 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -250,7 +250,7 @@ then
 		CI_OS_NAME=osx
 		JOBS=$(nproc)
 		;;
-	*,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
+	*,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/debian:*)
 		CI_OS_NAME=linux
 		JOBS=$(nproc)
 		;;

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 4/5] meson: rename Rust library target
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
                     ` (2 preceding siblings ...)
  2025-10-24  9:51   ` [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
@ 2025-10-24  9:51   ` Patrick Steinhardt
  2025-10-24  9:51   ` [PATCH v2 5/5] rust: generate bindings via cbindgen Patrick Steinhardt
  2025-10-28 19:37   ` [PATCH v2 0/5] " Ezekiel Newren
  5 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  9:51 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

Rename the Rust library target from `git_rs` to `rust`. The latter is
way easier to remember if one wants to compile only that target via
`meson compile rust`. Furthermore, this name matches the test target
that we have for Rust that can be invoked via `meson test rust`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 src/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/meson.build b/src/meson.build
index 25b9ad5a14..1c73549696 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -17,7 +17,7 @@ if get_option('buildtype') == 'release'
   cargo_command += '--release'
 endif
 
-libgit_rs = custom_target('git_rs',
+libgit_rs = custom_target('rust',
   input: libgit_rs_sources + [
     meson.project_source_root() / 'Cargo.toml',
   ],

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 5/5] rust: generate bindings via cbindgen
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
                     ` (3 preceding siblings ...)
  2025-10-24  9:51   ` [PATCH v2 4/5] meson: rename Rust library target Patrick Steinhardt
@ 2025-10-24  9:51   ` Patrick Steinhardt
  2025-10-24 14:01     ` Toon Claes
  2025-10-28 19:37   ` [PATCH v2 0/5] " Ezekiel Newren
  5 siblings, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-24  9:51 UTC (permalink / raw)
  To: git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

When compiling Git with Rust enabled we replace our C implementation of
the varint encoding with a Rust implementation. A prerequisite for doing
so is of course that the interfaces for both implementations are exactly
the same. If they aren't, then we risk subtle runtime errors.

We don't really have a way to detect such interface mismatches though:
the code will happily compile if we change either of the implementations
without adjusting the other implementation in the same spirit. The risk
of divergence is low right now as we only replace a single subsystem.
But it is expected that we'll grow more reimplementations over time, so
it is bound to increase.

A related issue is that we don't have an easy way to implement features
exclusively in Rust and make them available to our C library. Again, we
don't have such features yet, but there are work-in-progress patch
series that will eventually add them.

Both of these issues can be addressed by generating C bindings via the
cbindgen(1) tool: given a Rust crate, it extracts all functions marked
with `extern "C"` and creates a C declaration for them. These are then
written into a header file that we can include.

Set up this infrastructure in both our Makefile and in Meson. To
demonstrate its use, the generated "c-bindings.h" header is included in
"varint.c". If we now adapt "varint.rs" to have a different function
signature than the C code we'll now get a compiler error:

    In file included from ../varint.c:10:
    ./c-bindings.h:10:10: error: conflicting types for 'decode_varint'
       10 | uint32_t decode_varint(const uint8_t **bufp);
          |          ^
    ../varint.h:5:10: note: previous declaration is here
        5 | uint64_t decode_varint(const unsigned char **);

An initial version instead included the bindings in "varint.h". But that
would cause us to recompile all dependents of "varint.h" every time the
signatures of exported Rust functions change. So instead, we now include
it in "varint.c" and compile that file unconditionally again.

Adapt our CI to install cbindgen(1) accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitignore                 |  1 +
 Makefile                   | 14 +++++++++++---
 cbindgen.toml              | 13 +++++++++++++
 ci/install-dependencies.sh | 10 ++++++++--
 meson.build                | 25 ++++++++++++++++++++-----
 shared.mak                 |  1 +
 varint.c                   |  9 +++++++++
 7 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore
index 78a45cb5be..20558c9dc8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -197,6 +197,7 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
+/c-bindings.h
 /config-list.h
 /command-list.h
 /hook-list.h
diff --git a/Makefile b/Makefile
index 0bf5f17a90..1213a0fc96 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,9 +1326,7 @@ LIB_OBJS += urlmatch.o
 LIB_OBJS += usage.o
 LIB_OBJS += userdiff.o
 LIB_OBJS += utf8.o
-ifndef WITH_RUST
 LIB_OBJS += varint.o
-endif
 LIB_OBJS += version.o
 LIB_OBJS += versioncmp.o
 LIB_OBJS += walker.o
@@ -1562,6 +1560,14 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
 ifdef WITH_RUST
 BASIC_CFLAGS += -DWITH_RUST
 GITLIBS += $(RUST_LIB)
+
+C_BINDINGS = c-bindings.h
+
+GENERATED_H += $(C_BINDINGS)
+
+$(C_BINDINGS): cbindgen.toml $(RUST_SOURCES)
+	$(QUIET_CBINDGEN)cbindgen --output $@
+
 ifeq ($(uname_S),Windows)
 EXTLIBS += -luserenv
 endif
@@ -2619,6 +2625,8 @@ PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
 pager.sp pager.s pager.o: EXTRA_CPPFLAGS = \
 	-DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
 
+varint.sp varint.s varint.o: $(C_BINDINGS)
+
 version-def.h: version-def.h.in GIT-VERSION-GEN GIT-VERSION-FILE GIT-USER-AGENT
 	$(QUIET_GEN)$(call version_gen,"$(shell pwd)",$<,$@)
 
@@ -3806,7 +3814,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
 	$(RM) $(HCC)
-	$(RM) -r Cargo.lock target/
+	$(RM) -r Cargo.lock target/ $(C_BINDINGS)
 	$(RM) version-def.h
 	$(RM) -r $(dep_dirs) $(compdb_dir) compile_commands.json
 	$(RM) $(test_bindir_programs)
diff --git a/cbindgen.toml b/cbindgen.toml
new file mode 100644
index 0000000000..7f8cdfa4b2
--- /dev/null
+++ b/cbindgen.toml
@@ -0,0 +1,13 @@
+language = "C"
+
+# Write a warning into the generated file.
+autogen_warning = "/* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */"
+
+# 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"
+
+# Match our coding style more closely.
+tab_width = 8
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index b7b3cf35ed..d42c705391 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -32,12 +32,18 @@ alpine-*)
 		bash cvs gnupg perl-cgi perl-dbd-sqlite perl-io-tty >/dev/null
 	;;
 fedora-*|almalinux-*)
+	RUST_DEPS="cargo cbindgen"
 	case "$jobname" in
+	almalinux-8)
+		# AlmaLinux 8 does not have cbindgen, it was only added in version 9.
+		RUST_DEPS=;;
 	*-meson)
 		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 $RUST_DEPS >/dev/null
 	;;
 ubuntu-*|i386/debian-*|debian-*)
 	# Required so that apt doesn't wait for user input on certain packages.
@@ -64,7 +70,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
 
 	# Starting with Ubuntu 25.10, sudo can now be provided via either
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())
 if rust_option.allowed()
   subdir('src')
   libgit_c_args += '-DWITH_RUST'
@@ -1712,10 +1715,22 @@ if rust_option.allowed()
   if host_machine.system() == 'windows'
     libgit_dependencies += compiler.find_library('userenv')
   endif
-else
-  libgit_sources += [
-    'varint.c',
-  ]
+
+  cbindgen_input = [ 'cbindgen.toml' ]
+  foreach source : libgit_rs_sources
+    cbindgen_input += 'src' / source
+  endforeach
+
+  libgit_sources += custom_target('c-bindings.h',
+    input: cbindgen_input,
+    output: 'c-bindings.h',
+    command: [
+      cbindgen,
+      '--output',
+      '@OUTPUT@',
+      meson.current_source_dir(),
+    ],
+  )
 endif
 
 libgit = declare_dependency(
diff --git a/shared.mak b/shared.mak
index 0e7492076e..598e58e069 100644
--- a/shared.mak
+++ b/shared.mak
@@ -57,6 +57,7 @@ ifndef V
 
 ## Used in "Makefile"
 	QUIET_CARGO    = @echo '   ' CARGO $@;
+	QUIET_CBINDGEN = @echo '   ' CBINDGEN $@;
 	QUIET_CC       = @echo '   ' CC $@;
 	QUIET_AR       = @echo '   ' AR $@;
 	QUIET_LINK     = @echo '   ' LINK $@;
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"
+#else
 uint64_t decode_varint(const unsigned char **bufp)
 {
 	const unsigned char *buf = *bufp;
@@ -28,3 +36,4 @@ uint8_t encode_varint(uint64_t value, unsigned char *buf)
 		memcpy(buf, varint + pos, sizeof(varint) - pos);
 	return sizeof(varint) - pos;
 }
+#endif

-- 
2.51.1.930.gacf6e81ea2.dirty


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 5/5] rust: generate bindings via cbindgen
  2025-10-24  9:51   ` [PATCH v2 5/5] rust: generate bindings via cbindgen Patrick Steinhardt
@ 2025-10-24 14:01     ` Toon Claes
  2025-10-30  9:51       ` Patrick Steinhardt
  0 siblings, 1 reply; 38+ messages in thread
From: Toon Claes @ 2025-10-24 14:01 UTC (permalink / raw)
  To: Patrick Steinhardt, git; +Cc: brian m. carlson, Ezekiel Newren, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-24  6:37     ` Patrick Steinhardt
@ 2025-10-27 20:35       ` Ezekiel Newren
  2025-10-27 21:14         ` brian m. carlson
  0 siblings, 1 reply; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-27 20:35 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson

On Fri, Oct 24, 2025 at 12:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > 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.

You're targeting a very old version of Rust (1.49). I'm not even sure
that cbindgen will work with a version that old, but if it does then
we should use it in build.rs to make sure we're not using any features
of cbindgen that aren't available until later versions. If we use
cbindgen that is packaged with the platform then we can't precisely
control which version of cbindgen is being used. This is a matter of
reproducibility. There may be platforms that can compile Rust, but
can't generate C header files via cbindgen because cbindgen hard codes
that a certain minimum Rust version is required in its own Cargo.toml
file.

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

I'd like to avoid creating a gordian knot in Rust like how the
existing C code is a gordian knot. Creating multiple crates will
encourage better api design so that our code can be more modular and
easier to maintain. The reason why I don't want to put this off is
because the more we add to Rust the harder it'll be to refactor the
code to use Cargo workspaces. Let's do it now while it's still really
easy. As there is only 1 crate conceptually the code changes required
to convert it to a workspace will be very minimal.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-27 20:35       ` Ezekiel Newren
@ 2025-10-27 21:14         ` brian m. carlson
  2025-10-28  4:15           ` Junio C Hamano
  2025-10-30  9:50           ` Patrick Steinhardt
  0 siblings, 2 replies; 38+ messages in thread
From: brian m. carlson @ 2025-10-27 21:14 UTC (permalink / raw)
  To: Ezekiel Newren; +Cc: Patrick Steinhardt, git

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

On 2025-10-27 at 20:35:59, Ezekiel Newren wrote:
> On Fri, Oct 24, 2025 at 12:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > 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.
> 
> You're targeting a very old version of Rust (1.49). I'm not even sure
> that cbindgen will work with a version that old, but if it does then
> we should use it in build.rs to make sure we're not using any features
> of cbindgen that aren't available until later versions. If we use
> cbindgen that is packaged with the platform then we can't precisely
> control which version of cbindgen is being used. This is a matter of
> reproducibility. There may be platforms that can compile Rust, but
> can't generate C header files via cbindgen because cbindgen hard codes
> that a certain minimum Rust version is required in its own Cargo.toml
> file.

Yes, I agree with this.  Not all systems have cbindgen and it's not
guaranteed that the system's cbindgen will work with the version of Rust
that you want to target or that's being used to compile.

For instance, I'm using Debian unstable with a system cbindgen 0.27.0.
This requires Rust 1.70 or newer. If I use rustup to test my code on
Rust 1.49, then the code won't compile for me.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-27 21:14         ` brian m. carlson
@ 2025-10-28  4:15           ` Junio C Hamano
  2025-10-28 19:11             ` Ezekiel Newren
  2025-10-30  9:50             ` Patrick Steinhardt
  2025-10-30  9:50           ` Patrick Steinhardt
  1 sibling, 2 replies; 38+ messages in thread
From: Junio C Hamano @ 2025-10-28  4:15 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Ezekiel Newren, Patrick Steinhardt, git

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2025-10-27 at 20:35:59, Ezekiel Newren wrote:
>> On Fri, Oct 24, 2025 at 12:37 AM Patrick Steinhardt <ps@pks.im> wrote:
>> > > 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.
>> 
>> You're targeting a very old version of Rust (1.49). I'm not even sure
>> that cbindgen will work with a version that old, but if it does then
>> we should use it in build.rs to make sure we're not using any features
>> of cbindgen that aren't available until later versions.
> ...
> For instance, I'm using Debian unstable with a system cbindgen 0.27.0.
> This requires Rust 1.70 or newer. If I use rustup to test my code on
> Rust 1.49, then the code won't compile for me.

Have we even agreed on which Rust version we would aim for?  With
BreakingChanges.adoc We have agreed to make some version of Rust
mandatory by the time we hit Git 3.0 but IIRC, there isn't anything
written down except for an old message from you

  https://lore.kernel.org/git/ZZ9K1CVBKdij4tG0@tapette.crustytoothpaste.net/

that expressed your preference to support the version of Rust in the
latest Debian stable plus the version in Debian's oldstable until
the latest stable has been out for a year, which nobody responded
to, so we cannot quite say that is the consensus of the community,
yet.

Given that the stable/trixie was released on August 9th, 2025, we
still need to go by oldstable/bookworm, which has Rust 1.63, if
people agree that your rule to decide the floor version is sensible
(which I would say is OK).




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-28  4:15           ` Junio C Hamano
@ 2025-10-28 19:11             ` Ezekiel Newren
  2025-10-30  9:50               ` Patrick Steinhardt
  2025-10-30  9:50             ` Patrick Steinhardt
  1 sibling, 1 reply; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-28 19:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: brian m. carlson, Patrick Steinhardt, git

On Mon, Oct 27, 2025 at 10:15 PM Junio C Hamano <gitster@pobox.com> wrote:
> Given that the stable/trixie was released on August 9th, 2025, we
> still need to go by oldstable/bookworm, which has Rust 1.63, if
> people agree that your rule to decide the floor version is sensible
> (which I would say is OK).

I think that 1.63 should be the minimum that Git supports. I think
1.49 is way too old. It was a bit of a struggle to get cbindgen to
work with 1.63, and I don't know if it will work at all with 1.49.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order
  2025-10-24  9:51   ` [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order Patrick Steinhardt
@ 2025-10-28 19:14     ` Ezekiel Newren
  0 siblings, 0 replies; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-28 19:14 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Junio C Hamano

On Fri, Oct 24, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> We have mostly the exact same CI configuration as GitHub has for our
> Linux jobs. It's harder than necessary though to compare them with one
> another as the ordering is different between both.
>
> Reorder the job matrix in GitLab CI to match GitHub's order.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  .gitlab-ci.yml | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index b419a84e2c..1dbf236b2c 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -42,15 +42,15 @@ test:linux:
>        - jobname: linux-reftable
>          image: ubuntu:rolling
>          CC: clang
> +      - jobname: linux-TEST-vars
> +        image: ubuntu:20.04
> +        CC: gcc
> +        CC_PACKAGE: gcc-8
>        - jobname: linux-breaking-changes
>          image: ubuntu:20.04
>          CC: gcc
>        - jobname: fedora-breaking-changes-meson
>          image: fedora:latest
> -      - jobname: linux-TEST-vars
> -        image: ubuntu:20.04
> -        CC: gcc
> -        CC_PACKAGE: gcc-8
>        - jobname: linux-leaks
>          image: ubuntu:rolling
>          CC: gcc
> @@ -60,13 +60,13 @@ test:linux:
>        - jobname: linux-asan-ubsan
>          image: ubuntu:rolling
>          CC: clang
> +      - jobname: linux-meson
> +        image: ubuntu:rolling
> +        CC: gcc
>        - jobname: linux-musl-meson
>          image: alpine:latest
>        - jobname: linux32
>          image: i386/ubuntu:20.04
> -      - jobname: linux-meson
> -        image: ubuntu:rolling
> -        CC: gcc
>    artifacts:
>      paths:
>        - t/failed-test-artifacts
>
> --
> 2.51.1.930.gacf6e81ea2.dirty
>

Looks good.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/5] gitlab-ci: backfill missing Linux jobs
  2025-10-24  9:51   ` [PATCH v2 2/5] gitlab-ci: backfill missing Linux jobs Patrick Steinhardt
@ 2025-10-28 19:15     ` Ezekiel Newren
  0 siblings, 0 replies; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-28 19:15 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Junio C Hamano

On Fri, Oct 24, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> We're missing two Linux CI jobs in GitLab's CI that are present in
> GitHub's CI. Backfill them to ensure that GitLab has the same test
> coverage.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  .gitlab-ci.yml | 6 ++++++
>  ci/lib.sh      | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 1dbf236b2cd..f61ec2b6989 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -67,6 +67,12 @@ test:linux:
>          image: alpine:latest
>        - jobname: linux32
>          image: i386/ubuntu:20.04
> +      # A RHEL 8 compatible distro.  Supported until 2029-05-31.
> +      - jobname: almalinux-8
> +        image: almalinux:8
> +      # Supported until 2026-08-31.
> +      - jobname: debian-11
> +        image: debian:11
>    artifacts:
>      paths:
>        - t/failed-test-artifacts
> diff --git a/ci/lib.sh b/ci/lib.sh
> index f561884d401..a5c4eb40bea 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -250,7 +250,7 @@ then
>                 CI_OS_NAME=osx
>                 JOBS=$(nproc)
>                 ;;
> -       *,alpine:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
> +       *,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
>                 CI_OS_NAME=linux
>                 JOBS=$(nproc)
>                 ;;
>
> --
> 2.51.1.930.gacf6e81ea2.dirty
>

Looks good.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu
  2025-10-24  9:51   ` [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
@ 2025-10-28 19:17     ` Ezekiel Newren
  2025-10-30  9:50       ` Patrick Steinhardt
  0 siblings, 1 reply; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-28 19:17 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Junio C Hamano

On Fri, Oct 24, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Ubuntu has ended support for 32 bit platforms and is not maintaining any
> release anymore that has 32 bit support. But we still use i386/ubuntu in
> our CI pipeline to test for compatibility with 32 bit systems, even
> though that specific image does not receive updates anymore.
>
> Besides being end-of-life, this image also doesn't have all packages
> available to it anymore. This creates problems with a subsequent patch,
> where we're about to pull in cbindgen for generating Rust to C bindings.
>
> Drop the Ubuntu image and use Debian instead, which continues to
> maintain its 32 bit port.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  .github/workflows/main.yml | 3 +--
>  .gitlab-ci.yml             | 2 +-
>  ci/install-dependencies.sh | 6 +++---
>  ci/lib.sh                  | 2 +-
>  4 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index cc54824c388..0b16970cd7e 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -395,9 +395,8 @@ jobs:
>            cc: gcc
>          - jobname: linux-musl-meson
>            image: alpine:latest
> -        # Supported until 2025-04-02.
>          - jobname: linux32
> -          image: i386/ubuntu:focal
> +          image: i386/debian:latest
>          # A RHEL 8 compatible distro.  Supported until 2029-05-31.
>          - jobname: almalinux-8
>            image: almalinux:8
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index f61ec2b6989..31d5e5a3e0d 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -66,7 +66,7 @@ test:linux:
>        - jobname: linux-musl-meson
>          image: alpine:latest
>        - jobname: linux32
> -        image: i386/ubuntu:20.04
> +        image: i386/debian:latest
>        # A RHEL 8 compatible distro.  Supported until 2029-05-31.
>        - jobname: almalinux-8
>          image: almalinux:8
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 50628ee2dd6..b7b3cf35edf 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -39,7 +39,7 @@ fedora-*|almalinux-*)
>         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
>         ;;
> -ubuntu-*|i386/ubuntu-*|debian-*)
> +ubuntu-*|i386/debian-*|debian-*)
>         # Required so that apt doesn't wait for user input on certain packages.
>         export DEBIAN_FRONTEND=noninteractive
>
> @@ -48,9 +48,9 @@ ubuntu-*|i386/ubuntu-*|debian-*)
>                 SVN='libsvn-perl subversion'
>                 LANGUAGES='language-pack-is'
>                 ;;
> -       i386/ubuntu-*)
> +       i386/debian-*)
>                 SVN=
> -               LANGUAGES='language-pack-is'
> +               LANGUAGES='locales-all'
>                 ;;
>         *)
>                 SVN='libsvn-perl subversion'
> diff --git a/ci/lib.sh b/ci/lib.sh
> index a5c4eb40bea..fdfde612339 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -250,7 +250,7 @@ then
>                 CI_OS_NAME=osx
>                 JOBS=$(nproc)
>                 ;;
> -       *,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
> +       *,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/debian:*)
>                 CI_OS_NAME=linux
>                 JOBS=$(nproc)
>                 ;;
>
> --
> 2.51.1.930.gacf6e81ea2.dirty
>

Makes sense to me, but I wonder if we should test with other 32 bit
linux distros. This isn't a critique or a suggestion, I'm just
wondering out loud.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/5] rust: generate bindings via cbindgen
  2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
                     ` (4 preceding siblings ...)
  2025-10-24  9:51   ` [PATCH v2 5/5] rust: generate bindings via cbindgen Patrick Steinhardt
@ 2025-10-28 19:37   ` Ezekiel Newren
  2025-10-30  9:50     ` Patrick Steinhardt
  5 siblings, 1 reply; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-28 19:37 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, brian m. carlson, Junio C Hamano

On Fri, Oct 24, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Hi,
>
> this small patch series introduces support for cbindgen(1). This tool is
> used to generate C headers from `extern "C"` blocks so that Rust code
> can easily be called from C code.
>
> For now, the only use case is to verify that our varint reimplementation
> matches the C implementation. But later on this can and will be used to
> call Rust-specific features from C, as well.
>
> The topic is built on top of c54a18ef67 (The twenty-second batch,
> 2025-10-22) with ps/ci-rust at e509b5b8be (rust: support for Windows,
> 2025-10-15) merged into it.
>
> Changes in v2:
>   - Take some of the pieces from Ezekiel's "cbindgen.toml" file. I have
>     not yet taken `usize_is_size_t`, as that option requires cbindgen
>     v0.16.0, which is not available on Ubuntu 20.04.
>   - Backfill missing jobs for GitLab CI.
>   - Fix CI failures on Alma Linux 8 because cbindgen isn't available
>     there.
>   - Link to v1: https://lore.kernel.org/r/20251023-b4-pks-rust-cbindgen-v1-0-c19b61b03127@pks.im

I really think that this patch series should include migrating to a
Cargo Workspace. That'll mean moving /Cargo.toml and /src into
gitcore/ and creating a new top-level /Cargo.toml with the following
content:
[workspace]
members = [
    "gitcore",
]
resolver = "2"

Along with the other cascading refactor changes needed to make this
work. Let's do this now while it's still easy.

If we don't do this now then we'll be locked into a single crate
project forever because there'll be too much momentum later. Having
the ability to define multiple crates allows us to:
* Easier to understand the big picture of Git as each logical
component will be its own crate
* Make Git more modular
* Design better api interfaces between logically separate code
* define a header file per crate via cbindgen
* avoid gordian knot problems from making Rust a monolithic component
* easier to reduce dependency sprawl as each crate only defines what it needs
* Improves compile times through incremental rebuilds. Cargo caches
build artifacts per crate. When only one crate changes, others don’t
need to recompile. This becomes a major quality-of-life improvement as
the Rust footprint grows.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-28 19:11             ` Ezekiel Newren
@ 2025-10-30  9:50               ` Patrick Steinhardt
  0 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-30  9:50 UTC (permalink / raw)
  To: Ezekiel Newren; +Cc: Junio C Hamano, brian m. carlson, git

On Tue, Oct 28, 2025 at 01:11:53PM -0600, Ezekiel Newren wrote:
> On Mon, Oct 27, 2025 at 10:15 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Given that the stable/trixie was released on August 9th, 2025, we
> > still need to go by oldstable/bookworm, which has Rust 1.63, if
> > people agree that your rule to decide the floor version is sensible
> > (which I would say is OK).
> 
> I think that 1.63 should be the minimum that Git supports. I think
> 1.49 is way too old. It was a bit of a struggle to get cbindgen to
> work with 1.63, and I don't know if it will work at all with 1.49.

I think cbindgen 0.20 should support Rust 1.49, but I'm honestly not
sure about this. They simply didn't specify a MSRV before 0.21, and in
0.21 they bumped to require Rust 1.57.

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-28  4:15           ` Junio C Hamano
  2025-10-28 19:11             ` Ezekiel Newren
@ 2025-10-30  9:50             ` Patrick Steinhardt
  2025-10-30 21:40               ` brian m. carlson
  1 sibling, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-30  9:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: brian m. carlson, Ezekiel Newren, git

On Mon, Oct 27, 2025 at 09:15:51PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On 2025-10-27 at 20:35:59, Ezekiel Newren wrote:
> >> On Fri, Oct 24, 2025 at 12:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> >> > > 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.
> >> 
> >> You're targeting a very old version of Rust (1.49). I'm not even sure
> >> that cbindgen will work with a version that old, but if it does then
> >> we should use it in build.rs to make sure we're not using any features
> >> of cbindgen that aren't available until later versions.
> > ...
> > For instance, I'm using Debian unstable with a system cbindgen 0.27.0.
> > This requires Rust 1.70 or newer. If I use rustup to test my code on
> > Rust 1.49, then the code won't compile for me.
> 
> Have we even agreed on which Rust version we would aim for?  With
> BreakingChanges.adoc We have agreed to make some version of Rust
> mandatory by the time we hit Git 3.0 but IIRC, there isn't anything
> written down except for an old message from you
> 
>   https://lore.kernel.org/git/ZZ9K1CVBKdij4tG0@tapette.crustytoothpaste.net/
> 
> that expressed your preference to support the version of Rust in the
> latest Debian stable plus the version in Debian's oldstable until
> the latest stable has been out for a year, which nobody responded
> to, so we cannot quite say that is the consensus of the community,
> yet.

For now that Rust version is 1.49, and that's enforced by our CI. The
reason for this specific version is that it's the target version for the
gcc-rs folks, so it may help currently-unsupported platforms to get
support earlier.

But I made clear in past patch series that if we have strong reasons to
use a more recent version of Rust, then we should update. I mostly
wanted us to do this intentionally than picking any random Rust version
and saying that "this is it now".

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu
  2025-10-28 19:17     ` Ezekiel Newren
@ 2025-10-30  9:50       ` Patrick Steinhardt
  0 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-30  9:50 UTC (permalink / raw)
  To: Ezekiel Newren; +Cc: git, brian m. carlson, Junio C Hamano

On Tue, Oct 28, 2025 at 01:17:53PM -0600, Ezekiel Newren wrote:
> On Fri, Oct 24, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index a5c4eb40bea..fdfde612339 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -250,7 +250,7 @@ then
> >                 CI_OS_NAME=osx
> >                 JOBS=$(nproc)
> >                 ;;
> > -       *,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/ubuntu:*)
> > +       *,almalinux:*|*,alpine:*|*,debian:*|*,fedora:*|*,ubuntu:*|*,i386/debian:*)
> >                 CI_OS_NAME=linux
> >                 JOBS=$(nproc)
> >                 ;;
> >
> > --
> > 2.51.1.930.gacf6e81ea2.dirty
> >
> 
> Makes sense to me, but I wonder if we should test with other 32 bit
> linux distros. This isn't a critique or a suggestion, I'm just
> wondering out loud.

Potentially, I guess. I basically picked Debian because it's close to
Ubuntu and because we already support it in our CI. So it required the
least amount of churn.

We could eventually add additional 32 bit distros. The question of
course is whether that's really worth it. The importance of 32 bit is
overall dwindling, so it may be good enough to only test on a single
distro.

That being said, I don't really have a strong opinion on this.

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/5] rust: generate bindings via cbindgen
  2025-10-28 19:37   ` [PATCH v2 0/5] " Ezekiel Newren
@ 2025-10-30  9:50     ` Patrick Steinhardt
  0 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-30  9:50 UTC (permalink / raw)
  To: Ezekiel Newren; +Cc: git, brian m. carlson, Junio C Hamano

On Tue, Oct 28, 2025 at 01:37:00PM -0600, Ezekiel Newren wrote:
> On Fri, Oct 24, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > Hi,
> >
> > this small patch series introduces support for cbindgen(1). This tool is
> > used to generate C headers from `extern "C"` blocks so that Rust code
> > can easily be called from C code.
> >
> > For now, the only use case is to verify that our varint reimplementation
> > matches the C implementation. But later on this can and will be used to
> > call Rust-specific features from C, as well.
> >
> > The topic is built on top of c54a18ef67 (The twenty-second batch,
> > 2025-10-22) with ps/ci-rust at e509b5b8be (rust: support for Windows,
> > 2025-10-15) merged into it.
> >
> > Changes in v2:
> >   - Take some of the pieces from Ezekiel's "cbindgen.toml" file. I have
> >     not yet taken `usize_is_size_t`, as that option requires cbindgen
> >     v0.16.0, which is not available on Ubuntu 20.04.
> >   - Backfill missing jobs for GitLab CI.
> >   - Fix CI failures on Alma Linux 8 because cbindgen isn't available
> >     there.
> >   - Link to v1: https://lore.kernel.org/r/20251023-b4-pks-rust-cbindgen-v1-0-c19b61b03127@pks.im
> 
> I really think that this patch series should include migrating to a
> Cargo Workspace. That'll mean moving /Cargo.toml and /src into
> gitcore/ and creating a new top-level /Cargo.toml with the following
> content:
> [workspace]
> members = [
>     "gitcore",
> ]
> resolver = "2"
> 
> Along with the other cascading refactor changes needed to make this
> work. Let's do this now while it's still easy.

I simply think that cbindgen and workspaces are quite unrelated to one
another for now. So an alternate suggestion: once this patch series here
lands we could introduce workspaces in the subsequent patch series. I
think that's still early enough, and this patch series here shouldn't
cause significant additional churn.

In any case, I'd prefer if you wrote that patch series to introduce
workspaces. I don't feel like I have enough experience with them to be
able to argue properly why we want to have them.

Does that work for you?

Thanks!

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-27 21:14         ` brian m. carlson
  2025-10-28  4:15           ` Junio C Hamano
@ 2025-10-30  9:50           ` Patrick Steinhardt
  2025-10-31 23:36             ` Ezekiel Newren
  1 sibling, 1 reply; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-30  9:50 UTC (permalink / raw)
  To: brian m. carlson, Ezekiel Newren, git

On Mon, Oct 27, 2025 at 09:14:51PM +0000, brian m. carlson wrote:
> On 2025-10-27 at 20:35:59, Ezekiel Newren wrote:
> > On Fri, Oct 24, 2025 at 12:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > > 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.
> > 
> > You're targeting a very old version of Rust (1.49). I'm not even sure
> > that cbindgen will work with a version that old, but if it does then
> > we should use it in build.rs to make sure we're not using any features
> > of cbindgen that aren't available until later versions. If we use
> > cbindgen that is packaged with the platform then we can't precisely
> > control which version of cbindgen is being used. This is a matter of
> > reproducibility. There may be platforms that can compile Rust, but
> > can't generate C header files via cbindgen because cbindgen hard codes
> > that a certain minimum Rust version is required in its own Cargo.toml
> > file.
> 
> Yes, I agree with this.  Not all systems have cbindgen and it's not
> guaranteed that the system's cbindgen will work with the version of Rust
> that you want to target or that's being used to compile.

Okay. In that case the question to me is how to drive cbindgen from our
Makefile and from Meson if it's going to be invoked via "build.rs". It
doesn't make much sense from my PoV to make generation of the C headers
depend on building the complete Rust library. Doubly so because we'd now
have a chicken-and-egg problem:

  1. To build libgit.a we need to have the C interop header.

  2. To build the C interop header we need to build the Rust library.

  3. The Rust library depends on libgit.a.

So am I missing anything obvious here for how to declare cbindgen in our
"Cargo.toml" file and invoke it directly from our other build systems?

Thanks!

Patrick

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 5/5] rust: generate bindings via cbindgen
  2025-10-24 14:01     ` Toon Claes
@ 2025-10-30  9:51       ` Patrick Steinhardt
  0 siblings, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-30  9:51 UTC (permalink / raw)
  To: Toon Claes; +Cc: git, brian m. carlson, Ezekiel Newren, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-30  9:50             ` Patrick Steinhardt
@ 2025-10-30 21:40               ` brian m. carlson
  2025-10-30 21:50                 ` Junio C Hamano
  2025-10-31  6:05                 ` Patrick Steinhardt
  0 siblings, 2 replies; 38+ messages in thread
From: brian m. carlson @ 2025-10-30 21:40 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Junio C Hamano, Ezekiel Newren, git

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

On 2025-10-30 at 09:50:36, Patrick Steinhardt wrote:
> For now that Rust version is 1.49, and that's enforced by our CI. The
> reason for this specific version is that it's the target version for the
> gcc-rs folks, so it may help currently-unsupported platforms to get
> support earlier.

As I mentioned a couple of times, gcc-rs uses the standard library of
Rust 1.49 since that's what it's targeting, and as a result it will not
support any platforms that Rust 1.49 didn't support since there isn't
standard library support for those platforms in that version.  It's like
trying to use a 2009 version of glibc and expecting it to work on
RISC-V, which was released in 2010—it simply won't.

That's why I was very clear at the Contributor's Summit that the message
we must send to platforms that do not have Rust is that they need to
port LLVM and target Rust that way, since that is the surest path to
success and to being able to get the necessary standard library changes
for things to work properly.  gcc-rs may be a viable solution in the
future, but it is not now, and absent substantial advances and an
order-of-magnitude faster development, it is unlikely to meet that
standard in time for Git 3.0.

So given that, I would propose that we target Rust 1.63 in conjunction
with my proposal.  I can send a patch to that effect later on.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-30 21:40               ` brian m. carlson
@ 2025-10-30 21:50                 ` Junio C Hamano
  2025-10-30 23:38                   ` brian m. carlson
  2025-10-31  6:05                 ` Patrick Steinhardt
  1 sibling, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2025-10-30 21:50 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Patrick Steinhardt, Ezekiel Newren, git

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> with my proposal.  I can send a patch to that effect later on.

Presumably 1.63 is older than 1.77 so we would need the single-colon
syntax in the output from build.rs in your series?


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-30 21:50                 ` Junio C Hamano
@ 2025-10-30 23:38                   ` brian m. carlson
  0 siblings, 0 replies; 38+ messages in thread
From: brian m. carlson @ 2025-10-30 23:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Patrick Steinhardt, Ezekiel Newren, git

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On 2025-10-30 at 21:50:05, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > with my proposal.  I can send a patch to that effect later on.
> 
> Presumably 1.63 is older than 1.77 so we would need the single-colon
> syntax in the output from build.rs in your series?

Yes, I will include that in v2.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-30 21:40               ` brian m. carlson
  2025-10-30 21:50                 ` Junio C Hamano
@ 2025-10-31  6:05                 ` Patrick Steinhardt
  1 sibling, 0 replies; 38+ messages in thread
From: Patrick Steinhardt @ 2025-10-31  6:05 UTC (permalink / raw)
  To: brian m. carlson, Junio C Hamano, Ezekiel Newren, git

On Thu, Oct 30, 2025 at 09:40:01PM +0000, brian m. carlson wrote:
> On 2025-10-30 at 09:50:36, Patrick Steinhardt wrote:
> > For now that Rust version is 1.49, and that's enforced by our CI. The
> > reason for this specific version is that it's the target version for the
> > gcc-rs folks, so it may help currently-unsupported platforms to get
> > support earlier.
> 
> As I mentioned a couple of times, gcc-rs uses the standard library of
> Rust 1.49 since that's what it's targeting, and as a result it will not
> support any platforms that Rust 1.49 didn't support since there isn't
> standard library support for those platforms in that version.  It's like
> trying to use a 2009 version of glibc and expecting it to work on
> RISC-V, which was released in 2010—it simply won't.
> 
> That's why I was very clear at the Contributor's Summit that the message
> we must send to platforms that do not have Rust is that they need to
> port LLVM and target Rust that way, since that is the surest path to
> success and to being able to get the necessary standard library changes
> for things to work properly.  gcc-rs may be a viable solution in the
> future, but it is not now, and absent substantial advances and an
> order-of-magnitude faster development, it is unlikely to meet that
> standard in time for Git 3.0.

It seems like there is good progress in gccrs, and it seems like the
speed is picking up a bit. They also recently said that it shouldn't be
that complicated to move to 1.80 once the 1.49 baseline is implemented,
so that makes me more amenable towards picking a more recent Rust version
[1].

> So given that, I would propose that we target Rust 1.63 in conjunction
> with my proposal.  I can send a patch to that effect later on.

I might've missed it, but why 1.63 in particular? Happy to defer the
discussion until you post the patch though. I mostly want to make sure
that we pick the version with intent.

Thanks!

Patrick

[1]: https://lwn.net/Articles/1040197/

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/3] rust: generate bindings via cbindgen
  2025-10-30  9:50           ` Patrick Steinhardt
@ 2025-10-31 23:36             ` Ezekiel Newren
  0 siblings, 0 replies; 38+ messages in thread
From: Ezekiel Newren @ 2025-10-31 23:36 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: brian m. carlson, git

On Thu, Oct 30, 2025 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 27, 2025 at 09:14:51PM +0000, brian m. carlson wrote:
> > On 2025-10-27 at 20:35:59, Ezekiel Newren wrote:
> > > On Fri, Oct 24, 2025 at 12:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > > > 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.
> > >
> > > You're targeting a very old version of Rust (1.49). I'm not even sure
> > > that cbindgen will work with a version that old, but if it does then
> > > we should use it in build.rs to make sure we're not using any features
> > > of cbindgen that aren't available until later versions. If we use
> > > cbindgen that is packaged with the platform then we can't precisely
> > > control which version of cbindgen is being used. This is a matter of
> > > reproducibility. There may be platforms that can compile Rust, but
> > > can't generate C header files via cbindgen because cbindgen hard codes
> > > that a certain minimum Rust version is required in its own Cargo.toml
> > > file.
> >
> > Yes, I agree with this.  Not all systems have cbindgen and it's not
> > guaranteed that the system's cbindgen will work with the version of Rust
> > that you want to target or that's being used to compile.
>
> Okay. In that case the question to me is how to drive cbindgen from our
> Makefile and from Meson if it's going to be invoked via "build.rs". It
> doesn't make much sense from my PoV to make generation of the C headers
> depend on building the complete Rust library. Doubly so because we'd now
> have a chicken-and-egg problem:
>
>   1. To build libgit.a we need to have the C interop header.
>
>   2. To build the C interop header we need to build the Rust library.
>
>   3. The Rust library depends on libgit.a.
>
> So am I missing anything obvious here for how to declare cbindgen in our
> "Cargo.toml" file and invoke it directly from our other build systems?

You would need a separate crate whose only job is to generate the
header files and then Makefile or Meson can be invoked. But since we
only have a single crate we can't do that.

As to why version 1.63.0 it's because of Debian [1]. "Our discussed
plan is to support the version in Debian stable, plus a
year.  So we'd be supporting 1.63.0 for a year after trixie's
release." - Brian M. Carlson

Rust version 1.49 can't use cbindgen newer than 0.1.0 (the latest is
0.29.2). Any cbindgen version newer than 0.1.0 requires a minimum of
Rust 1.68.0. The way that I got cbindgen to work with version 1.63.0
is that I overrode cbindgen's dependencies in Cargo.toml. That hack
only works so far because the older the Rust version the less likely
older and older sub dependencies will work.

Since you're worried about build times I think that cbindgen should be
in its own crate whose only job is to generate the header files and
then have the subsequent CI jobs copy in those generated header files.
For local development cargo will need to be invoked first to generate
the header files and then Makefile or Meson can be invoked. If we put
cbindgen into its own crate then we won't have a chicken-and-egg
problem. That's why we need to convert to a cargo workspace in this
patch series or before. It cannot come after.

[1]  https://lore.kernel.org/git/aHlp1joMwexLZAAb@fruit.crustytoothpaste.net/

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2025-10-31 23:37 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-23  7:17 [PATCH 0/3] rust: generate bindings via cbindgen Patrick Steinhardt
2025-10-23  7:17 ` [PATCH 1/3] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
2025-10-23 17:56   ` Junio C Hamano
2025-10-24  6:36     ` Patrick Steinhardt
2025-10-23  7:17 ` [PATCH 2/3] meson: rename Rust library target Patrick Steinhardt
2025-10-23  7:17 ` [PATCH 3/3] rust: generate bindings via cbindgen Patrick Steinhardt
2025-10-23 18:00   ` Ezekiel Newren
2025-10-24  6:37     ` Patrick Steinhardt
2025-10-27 20:35       ` Ezekiel Newren
2025-10-27 21:14         ` brian m. carlson
2025-10-28  4:15           ` Junio C Hamano
2025-10-28 19:11             ` Ezekiel Newren
2025-10-30  9:50               ` Patrick Steinhardt
2025-10-30  9:50             ` Patrick Steinhardt
2025-10-30 21:40               ` brian m. carlson
2025-10-30 21:50                 ` Junio C Hamano
2025-10-30 23:38                   ` brian m. carlson
2025-10-31  6:05                 ` Patrick Steinhardt
2025-10-30  9:50           ` Patrick Steinhardt
2025-10-31 23:36             ` Ezekiel Newren
2025-10-23 21:42   ` Junio C Hamano
2025-10-23 22:01   ` Junio C Hamano
2025-10-23 22:37   ` Junio C Hamano
2025-10-24  6:36     ` Patrick Steinhardt
2025-10-24  9:51 ` [PATCH v2 0/5] " Patrick Steinhardt
2025-10-24  9:51   ` [PATCH v2 1/5] gitlab-ci: reorder Linux job matrix to match GitHub's order Patrick Steinhardt
2025-10-28 19:14     ` Ezekiel Newren
2025-10-24  9:51   ` [PATCH v2 2/5] gitlab-ci: backfill missing Linux jobs Patrick Steinhardt
2025-10-28 19:15     ` Ezekiel Newren
2025-10-24  9:51   ` [PATCH v2 3/5] ci: use Debian instead of deprecated i386/ubuntu Patrick Steinhardt
2025-10-28 19:17     ` Ezekiel Newren
2025-10-30  9:50       ` Patrick Steinhardt
2025-10-24  9:51   ` [PATCH v2 4/5] meson: rename Rust library target Patrick Steinhardt
2025-10-24  9:51   ` [PATCH v2 5/5] rust: generate bindings via cbindgen Patrick Steinhardt
2025-10-24 14:01     ` Toon Claes
2025-10-30  9:51       ` Patrick Steinhardt
2025-10-28 19:37   ` [PATCH v2 0/5] " Ezekiel Newren
2025-10-30  9:50     ` Patrick Steinhardt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).