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

From: Philip Oakley <philipoakley@iee.email>
Date: Jun 18, 2021, 13:05 UTC
Message-ID: <b52d7c45-06dd-a9eb-bc86-e04d2ee16c5e@iee.email>
In-Reply-To: <nycvar.QRO.7.76.6.2106101140590.57@tvgsbejvaqbjf.bet>

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

Show 12 quoted lines

> Hi,
>
> On Sat, 5 Jun 2021, Bagas Sanjaya wrote:
>
>> On 05/06/21 00.43, Matthew Rogers via GitGitGadget wrote:
>>> 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/
>>>
>>> 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.

Show 11 quoted lines

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

Show 10 quoted lines

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