Subject: Re: [PATCH RFC v2 2/7] Makefile: introduce infrastructure to build internal Rust library

From: "brian m. carlson" <[email-masked]>
Date: Fri, 5 Sep 2025 20:21:54 +0000

On 2025-09-05 at 11:50:58, Patrick Steinhardt wrote:
> Introduce infrastructure to build the internal Rust library. This
> mirrors the infrastructure we have added to Meson in the preceding
> commit. Developers can enable the infrastructure by passing the new
> `WITH_RUST` build toggle.

The idea here seems great and I'm fully on board…

> diff --git a/Makefile b/Makefile
> index 555b7f4dc3..e7b3c8e57b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -483,6 +483,14 @@ include shared.mak
>  # Define LIBPCREDIR=/foo/bar if your PCRE header and library files are
>  # in /foo/bar/include and /foo/bar/lib directories.
>  #
> +# == Optional Rust support ==
> +#
> +# Define WITH_RUST if you want to include features and subsystems written in
> +# Rust into Git. For now, Rust is still an optional feature of the build
> +# process. With Git 3.0 though, Rust will always be enabled.
> +#
> +# Building Rust code requires Cargo.
> +#
>  # == SHA-1 and SHA-256 defines ==
>  #
>  # === SHA-1 backend ===
> @@ -918,6 +926,11 @@ TEST_SHELL_PATH = $(SHELL_PATH)
>  LIB_FILE = libgit.a
>  XDIFF_LIB = xdiff/lib.a
>  REFTABLE_LIB = reftable/libreftable.a
> +ifdef DEBUG
> +RUST_LIB = target/debug/libgit.a
> +else
> +RUST_LIB = target/release/libgit.a
> +endif
>  
>  GENERATED_H += command-list.h
>  GENERATED_H += config-list.h
> @@ -1387,8 +1400,12 @@ CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
>  
>  UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
>  
> -# xdiff and reftable libs may in turn depend on what is in libgit.a
> -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
> +GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
> +ifdef WITH_RUST
> +GITLIBS += $(RUST_LIB)
> +endif
> +# Other libs may in turn depend on what is in libgit.a.
> +GITLIBS += $(LIB_FILE)
>  EXTLIBS =
>  
>  GIT_USER_AGENT = git/$(GIT_VERSION)
> @@ -1411,6 +1428,19 @@ BASIC_LDFLAGS =
>  ARFLAGS = rcs
>  PTHREAD_CFLAGS =
>  
> +# Rust flags
> +CARGO_ARGS =
> +ifndef V
> +CARGO_ARGS += --quiet
> +endif
> +ifndef DEBUG
> +CARGO_ARGS += --release
> +endif
> +
> +ifdef WITH_RUST
> +BASIC_CFLAGS += -DWITH_RUST
> +endif

…but unfortunately, all of this code is above the `-include config.mak`
line, so if I set `WITH_RUST=1` in `config.mak`, it doesn't work: no
`target` directory is created and `git version --build-options` says
Rust isn't enabled.  (It does work if I specify `WITH_RUST=1` on the
command line, though.)

Might it be a better idea to place this with the conditional code
farther down so it's properly honoured when configured in `config.mak`
and friends?

I am very pleased by the fact that cargo is quiet by default, though,
and otherwise it seems to be well integrated into our build system.
This patch seems smaller than I was expecting, which is nice.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA