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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Sep 5, 2025, 20:21 UTC
Message-ID: <aLtGYlTXktuzxD0q@fruit.crustytoothpaste.net>
In-Reply-To: <20250905-b4-pks-rust-breaking-change-v2-2-6939cbf4a0b8@pks.im>

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…

Show 62 quoted lines

> 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