From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Thu, 20 Nov 2025 19:45:34 GMT
Subject: Re: [PATCH v3 6/6] rust: support for Windows
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:
>
> 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.

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

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

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