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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 19, 2019, 15:11 UTC
Message-ID: <nycvar.QRO.7.76.6.1906191500241.44@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPig+cTgmvwg_LV5zonQAR3c0et2p1Nm4YuAfAkYDF=2jY9YeQ@mail.gmail.com>

Hi Eric,

On Wed, 19 Jun 2019, Eric Sunshine wrote:

Show 18 quoted lines

> On Tue, Jun 18, 2019 at 8:24 AM Jeff Hostetler via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.

And so it did! And so it will do again, starting from the next iteration. Thanks.

Show 17 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?

Not really. This has little to do with assertions, and much more with memory usage debugging support in Visual Studio.

I guess that this (together with the other `USE_MSVC_CRTDBG`) should go into its own, separate commit.

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

It is actually implemented in a future patch series (I hinted at it in the cover letter).

Show 8 quoted lines

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

The thing to keep in mind is that the *primary* reason for this Makefile target is to publish a version of the source code that works in Visual Studio out of the box. Read: without getting a (pretty heavy) Git for Windows SDK on top.

The idea is to avoid having to download several hundred megabytes of Git for Windows SDK, and instead run the tests in the Git Bash of a regular Git for Windows.

Of course, this only works when certain "build products" (such as shell scripts that have been copy-edited to their final form, or certain generated files that the test suite wants to see) are included.

If you do not include them, tough luck, you cannot run the tests.

Of course, I *do* want contributors to run the tests, even if they choose the convenience of a full-fledged IDE, and I don't want to punish them for it by forcing them to pick all of the bits and pieces for themselves.

To achieve that, I force-add those generated files and commit the whole bunch, and publish the result as `vs/master` at https://github.com/git-for-windows/git. Actually, it is a trusty Azure Pipeline that does that.

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

Unless you struggled for yourself to find all the missing files you need in order to run the test suite. Or to cobble together a working Git from the build output of Visual Studio (which only contains the compiled C code, after all, and as you and me know fully well, Git insists on support files such as templates, too, and yes, they have to be "generated" and are of course excluded via `.gitignore`).

Once you went through those struggles, you don't want to do it again. I went a step further, and I don't want anybody but me to have to go through that again, either.

That is why I think that your statement might have been made under the false impression that this is an easy decision.

In any case, this is indeed a discussion for the next patch series, not this one.

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

Good idea! I made it so.

Thanks! Dscho