Re: [PATCH 13/17] msvc: support building Git using MS Visual C++

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 19, 2019, 08:35 UTC
Message-ID: <CAPig+cTgmvwg_LV5zonQAR3c0et2p1Nm4YuAfAkYDF=2jY9YeQ@mail.gmail.com>
In-Reply-To: <587cbcf61903693139ad4a02b32727bddd6d116d.1560860634.git.gitgitgadget@gmail.com>

On Tue, Jun 18, 2019 at 8:24 AM Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 14 quoted lines

> With this patch, Git can be built using the Microsoft toolchain, via:
>
>         make MSVC=1 [DEBUG=1]
>
> Third party libraries are built from source using the open source
> "vcpkg" tool set. See https://github.com/Microsoft/vcpkg
> [...]
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/Makefile b/Makefile
> @@ -1240,7 +1240,7 @@ endif
> -BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(SANE_TOOL_PATH_SQ)|'
> +BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix "$(SANE_TOOL_PATH_SQ)"|'

This seems like a distinct bug fix which should live in its own patch.

Show 13 quoted lines

> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -2388,6 +2388,12 @@ static void maybe_redirect_std_handles(void)
> +#ifdef _MSC_VER
> +#ifdef _DEBUG
> +#include <crtdbg.h>
> +#endif
> +#endif
> @@ -2405,6 +2411,12 @@ int wmain(int argc, const wchar_t **wargv)
> +#ifdef _MSC_VER
> +#ifdef USE_MSVC_CRTDBG
> +       _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
> +#endif
> +#endif

Shouldn't these changes be squashed into 16/17 (with the commit message of 16/17 adjusted accordingly), rather than being included in this patch?

Show 5 quoted lines

> diff --git a/compat/vcbuild/README b/compat/vcbuild/README
> @@ -1,3 +1,54 @@
> +Alternatively, run `make MSVC=1 vcxproj` and then load the generated
> +git.sln in Visual Studio. The initial build will install the vcpkg
> +system and build the dependencies automatically. This will take a while.

Is this bit implemented yet, or will it be introduced by a subsequent patch series mentioned in the cover letter? If the latter, perhaps this README snippet belongs to that future patch series.

> +Note that this will automatically add and commit the generated
> +.sln and .vcxproj files to the repo.  You may want to drop this
> +commit before submitting a Pull Request....

Yuck. An automatic commit as part of the build process has high surprise-factor, and it seems like it's creating extra work (and possibility for error) if the user needs to remember to drop it before submitting.

> +Or maybe we should put the .sln/.vcxproj files in the .gitignore file
> +and not do this.  I'm not sure.

Seems like a better choice.

Show 6 quoted lines

> diff --git a/compat/vcbuild/find_vs_env.bat b/compat/vcbuild/find_vs_env.bat
> @@ -0,0 +1,169 @@
> +:not_2015
> +   REM TODO....
> +   echo TODO support older versions of VS. >&2
> +   EXIT /B 1

As this is a user-facing error message, perhaps it could be worded differently. Maybe:

    ERROR: unsupported VS version (older than VS2015)

or something.