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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Eric,

On Wed, 19 Jun 2019, Eric Sunshine wrote:

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

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

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

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

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