Re: [PATCH v3 6/6] rust: support for Windows

From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Nov 20, 2025, 19:45 UTC
Message-ID: <CAH=ZcbB8cRgCTp-Q_CxJ4VFNY1+w+C20zgx9bMre4-hNmPrD7g@mail.gmail.com>
In-Reply-To: <20251015-b4-pks-ci-rust-v3-6-13810af33bd5@pks.im>

This is a retrospective review. I completely missed this patch series, and only noticed its existence after it was merged into master. The core problem is that these changes assume that windows builds only ever use the MSVC compiler, but that's not true.

On Wed, Oct 15, 2025 at 12:04 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 10 quoted lines

>
> The initial patch series that introduced Rust into the core of Git only
> cared about macOS and Linux. This specifically leaves out Windows, which
> indeed fails to build right now due to two issues:
>
>   - The Rust runtime requires `GetUserProfileDirectoryW()`, but we don't
>     link against "userenv.dll".
>
>   - The path of the Rust library built on Windows is different than on
>     most other systems systems.

That is true, but the build systems also need to check if the C compiler is gnu or msvc. Also you used the word "systems" twice.

Show 33 quoted lines

> diff --git a/Makefile b/Makefile
> index 7ea149598d..366fd173e7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -929,10 +929,17 @@ 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/libgitcore.a
> +RUST_TARGET_DIR = target/debug
>  else
> -RUST_LIB = target/release/libgitcore.a
> +RUST_TARGET_DIR = target/release
> +endif
> +
> +ifeq ($(uname_S),Windows)
> +RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
> +else
> +RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
>  endif
>
>  # xdiff and reftable libs may in turn depend on what is in libgit.a
> @@ -1538,6 +1545,9 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
>  ifdef WITH_RUST
>  BASIC_CFLAGS += -DWITH_RUST
>  GITLIBS += $(RUST_LIB)
> +ifeq ($(uname_S),Windows)
> +EXTLIBS += -luserenv
> +endif
>  endif
>  ifdef SANITIZE

This is not fully correct for Makefile. If Windows AND using MSVC -> gitcore.lib. However this bug doesn't show up because github ci doesn't test the windows+msvc+makefile combo.

Show 15 quoted lines

> diff --git a/meson.build b/meson.build
> index ec55d6a5fd..a9c865b2af 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1707,6 +1707,10 @@ rust_option = get_option('rust').disable_auto_if(not cargo.found())
>  if rust_option.allowed()
>    subdir('src')
>    libgit_c_args += '-DWITH_RUST'
> +
> +  if host_machine.system() == 'windows'
> +    libgit_dependencies += compiler.find_library('userenv')
> +  endif
>  else
>    libgit_sources += [
>      'varint.c',

Same issue as above, but it doesn't show up because the github ci doesn't test the windows+gnu+meson combo.

Show 21 quoted lines

> diff --git a/src/cargo-meson.sh b/src/cargo-meson.sh
> index 99400986d9..3998db0435 100755
> --- a/src/cargo-meson.sh
> +++ b/src/cargo-meson.sh
> @@ -26,7 +26,14 @@ then
>         exit $RET
>  fi
>
> -if ! cmp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
> +case "$(cargo -vV | sed -s 's/^host: \(.*\)$/\1/')" in
> +       *-windows-*)
> +               LIBNAME=gitcore.lib;;
> +       *)
> +               LIBNAME=libgitcore.a;;
> +esac
> +
> +if ! cmp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a" >/dev/null 2>&1
>  then
> -       cp "$BUILD_DIR/$BUILD_TYPE/libgitcore.a" "$BUILD_DIR/libgitcore.a"
> +       cp "$BUILD_DIR/$BUILD_TYPE/$LIBNAME" "$BUILD_DIR/libgitcore.a"
>  fi

Same issue again. This needs to test for windows AND msvc.