* [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
* 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 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
* [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 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 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
* 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 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 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-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 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
* 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 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
* [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
* 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
* [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
* 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
* [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
* 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 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
* [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 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 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 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
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).