Re: [PATCH 0/3] Make CMake work out of the box

On 10/06/2021 10:43, Johannes Schindelin wrote:

Hi,

On Sat, 5 Jun 2021, Bagas Sanjaya wrote:

quoted

On 05/06/21 00.43, Matthew Rogers via GitGitGadget wrote:

quoted

This pull request comes from our discussion here[1], and I think these
patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/ (local)

CCing the people involved in the original discussion.

Matt,
Thanks for picking this up and the approach to working around the
updated build approach of recent Visual Studio versions.
 
It looks good to me, but the CI should also be tweaked (see below) so
that it is tested.

quoted

This focused on improving CMake support, especially on Visual Studio, right?

Then so we have three ways to build Git:
1. plain Makefile
2. ./configure (really just wrapper on top of Makefile)
3. generate build file with CMake

If we want to support all of them, it may makes sense to have CI jobs that
perform build with each options above.

We already exercise the plain Makefile plenty, and the CMake-based build
using Windows (in the `vs-build` job in `.github/workflows/main.yml`).

There is one 'gotcha' in the yml (probably historical) in that it
doesn't actually test the approach/changes that Matt addresses regarding
my [1].

That is, I'm looking at the 'out of the box' view, while the yml test
_preloads_ the vcpkg artefacts.

There is also the (on Windows) issue that the ARM support has recently
been developed which also fudges the CmakeLists.txt file but forgot
about the assumption in the vcpkg install batch file that the default is
the x86 setup.

I do not see that it is worth spending many electrons exercising the
`./configure` way, seeing as the preferred way to build Git is by using
the `Makefile` directly.

And our CMake configuration only really works on Windows, the attempts to
get it to work on Linux were met with less enthusiasm, seeing as the
`Makefile` approach is the recommended (and supported) one.

tl;dr I don't think we need to augment our CI jobs as suggested.

I'd agree that there's no need to augment the CI job to expressly check
the other flags, but the existing test should reflect the intent of the
patches (i.e. no preloading of the vcpkg artefacts).

I haven't had much time to catch up on Git, and I'm off-line again from
Sat night for another week.