From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 17 Sep 2025 08:26:16 GMT
Subject: Re: [PATCH v2 07/18] build: introduce rust
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:
> 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.

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

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