Re: [PATCH v2 07/18] build: introduce rust

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 17, 2025, 08:26 UTC
Message-ID: <CAPig+cS_=YTBaCjn8-Th3yn3_k+a8_pMOmdv_Dq4S0tfp8BRCg@mail.gmail.com>
In-Reply-To: <6032a8740c0ba72420f42c3d8d801e1bdeec12d0.1758071798.git.gitgitgadget@gmail.com>

On Tue, Sep 16, 2025 at 9:17 PM Ezekiel Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 21 quoted lines

> Upcoming patches will simplify xdiff, while also porting parts of it to
> Rust. In preparation, add some stubs and setup the Rust build. For now,
> it is easier to let cargo build rust and have make or meson merely link
> against the static library that cargo builds. In line with ongoing
> libification efforts, use multiple crates to allow more modularity on
> the Rust side. The xdiff and ivec crates are the focal point for this
> series.
>
> cbindgen will be used to generate header files for each Rust crate for
> C interoperability.
>
> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
> diff --git a/.gitignore b/.gitignore
> @@ -254,3 +254,6 @@ Release/
>  /contrib/buildsystems/out
>  /contrib/libgit-rs/target
>  /contrib/libgit-sys/target
> +.idea/
> +/rust/target/
> +/rust/Cargo.lock

Is ".idea" directory detritus from your chosen editor? If so, it probably ought not be added to this list since we don't otherwise ignore detritus from foreign tools such as that.

> diff --git a/Makefile b/Makefile
> @@ -916,6 +924,51 @@ TEST_SHELL_PATH = $(SHELL_PATH)
> +RUST_CRATES :=
> +.PHONY: compile_rust rustclean

This is an extremely tiny nit, but multi-word target names in the Git Makefile are almost universally spelled with a hyphen rather than an underscore, so "compile-rust" would be more consistent and appropriate.

> +$(GIT_BUILD_DIR)/lib%.a:
> +       ./build_rust.sh $(GIT_BUILD_DIR) $(RUST_BUILD_MODE) $*

I wonder if this should follow the convention of most (but not quite all) other local shell script invocations in the Makefile of referencing SHELL_PATH; for instance:

    $(QUIET_GEN)$(SHELL_PATH) ./build_rust.sh ...etc...

with or without the $(QUIET_GEN) bit.

Show 6 quoted lines

> diff --git a/build_rust.sh b/build_rust.sh
> @@ -0,0 +1,60 @@
> +if [ "$dir_git_root" = "" ]; then
> +  echo "did not specify the directory for the root of git"
> +  exit 1
> +fi

Let's follow project style conventions for shell scripts:

    if test "$dir_git_root" = ""
    then
        ...
    fi

or even:

    if test -z "$dir_git_root"
    then
        ...
    fi

Same comment applies to the rest of the script.

Also, should the above error message be sent to stderr rather than stdout? (Genuine question since I'm not sure if other scripts are necessarily consistent in that regard.)

Show 10 quoted lines

> +if [ "$rust_build_profile" = "release" ]; then
> +  rust_args="--release"
> +  export RUSTFLAGS=''
> +elif [ "$rust_build_profile" = "debug" ]; then
> +  rust_args=""
> +  export RUSTFLAGS='-C debuginfo=2 -C opt-level=1 -C force-frame-pointers=yes'
> +else
> +  echo "illegal rust_build_profile value $rust_build_profile"
> +  exit 1
> +fi

For maximum portability, we don't use the:

    export FOO=bar

idiom in this project. Instead, do this:

    FOO=bar
    export FOO

The same comment applies to the remainder of the patch, as well.

Also, the empty-string assignments to RUSTFLAGS and rust_args above inconsistently use a mix of single and double quotes. However, we also usually just assign nothingness in such cases rather than an empty string:

    RUSTFLAGS=
    export RUSTFLAGS
    ...
    rust_args=

> +if rustup show active-toolchain | grep windows-msvc; then
> +  libfile="${crate}.lib"
> +  PATH="$(echo $PATH | tr ':' '\n' | grep -Ev "^(/mingw64/bin|/usr/bin)$" | paste -sd: -):/mingw64/bin:/usr/bin"
> +fi

Please add either an in-code comment or a sentence/paragraph to the commit message explaining why this PATH munging is needed.

> +cd $dir_rust && cargo clean && pwd && USE_LINKING="false" cargo build -p $crate $rust_args; cd $dir_git_root

When formatted like this, it is very easy to overlook the fact that the final `cd` is (correctly) not &&-chained with the preceding commands. Reformatting it like this would help:

    cd $dir_rust &&
        cargo clean &&
        pwd &&
        USE_LINKING="false" cargo build -p $crate $rust_args
    cd $dir_git_root

Alternatively, use a subshell and drop the final `cd`:

    (
        cd $dir_rust &&
        cargo clean &&
        pwd &&
        USE_LINKING="false" cargo build -p $crate $rust_args
    )

By the way, is the `pwd` command needed? What is its purpose?

> +rm $dst 2>/dev/null

I think we typically use `rm -f` without redirect to /dev/null to do the "right thing".

> +echo mv $src $dst
> +mv $src $dst