Re: ab/cmake-nix-and-ci, was Re: [PATCH] test-lib.sh: discover "git" in subdirs of "contrib/buildsystems/out"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 8, 2022, 11:34 UTC
Message-ID: <221208.86wn726qcv.gmgdl@evledraar.gmail.com>
In-Reply-To: <oq7p2776-po8p-r9s0-82o2-o77so874n419@tzk.qr>

On Thu, Dec 08 2022, Johannes Schindelin wrote:

Show 70 quoted lines

> Hi,
>
> On Tue, 6 Dec 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Dec 06 2022, Phillip Wood wrote:
>>
>> > On 06/12/2022 03:52, Junio C Hamano wrote:
>> >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> >>
>> >>> Just to add my own digression: I asked in some past thread (which
>> >>> I'm too lazy to dig up) why it was the cmake file couldn't just
>> >>> dispatch to "make" for most things.
>> >
>> > Because make is not installed by default on Windows. Our CI job uses
>> > msbuild (whatever that is) and when I was playing with Visual Studio
>> > last week it was using ninja.
>> >
>> >>> I.e. it needs to at some level be aware of what it's building for the
>> >>> IDE integration, but for say making a "grep.o" there's no reason it
>> >>> couldn't be running:
>> >>>
>> >>> 	make grep.o
>> >>>
>> >>> Instead of:
>> >>>
>> >>>          cc <args> -o grep grep.c [...]
>> >>>
>> >>> which requires duplicating much of the Makefile logic (possibly with
>> >>> some Makefile shim to not consider any dependencies in that case).
>> >> That leads to a question at the other extreme.  Why does any logic
>> >> in CMakeLists.txt even have to exist at all?  Whenever it is asked
>> >> to make foo, it can be running "make foo" instead of having its own
>> >> logic at all.  ;-)
>> >
>> > Yes, if make was available then we wouldn't need to use CMake.
>>
>> I think Junio and I are talking about something slightly different. Yes
>> "make" isn't available by default. Getting it requires installing a
>> larger SDK.
>>
>> But if you look at the history of contrib/vscode/README.md in our tree
>> you'll see that we used to support this "Visual Studio Solution" for
>> years via GNU make, it probably still works.
>
> It probably doesn't. Last time I had to use it, during the embargoed
> v2.37.1 release process, it didn't. I had to add plenty of patches to make
> it work again:
> https://github.com/git-for-windows/git/compare/323a69709944%5E...323a69709944%5E2
>
>> The change in 4c2c38e800f (ci: modification of main.yml to use cmake for
>> vs-build job, 2020-06-26) shows when the CI was switched over to using
>> cmake instead.
>>
>> The code to support that is still in-tree as the "vcxproj" target in
>> "config.mak.uname", which calls out to the ~1k lines of Perl code in
>> contrib/buildsystems/Generators/*.
>
> At some stage we can probably get rid of the `vcxproj` code. Before that,
> we can even get rid of the `vcproj` code that is bit-rotting in
> `contrib/buildsystems/`. But there seems no harm, and less maintenance
> burden, in keeping the `vcxproj`/`vcproj` parts where they are, as they
> are.
>
> Taking a step back, I see that we got far away from the topic that started
> this thread.
>
> So here's my take on `ab/cmake-nix-and-ci`: While that patch series'
> intention is apparently to make it easier to diagnose and fix CI problems,
> I only see that it adds new problems. It won't make it possible to
> diagnose most win+VS problems because they don't reproduce on Linux.

That would also be my take if that was the goal of the series. I agree that would be pretty pointless. Why test win+VS-specific code on Linux? That makes no sense.

But that's not the goal.

It's to make it easier to test the majority of the platform-agnostic
code in the cmake recipe. E.g. here's some past commits from myself,
Jeff King and Han-Wen (who I'm pretty sure doesn't use Windows) where
we've had to patch the cmake recipe in addition to the Makefile:
	
	ef8a6c62687 (reftable: utility functions, 2021-10-07)
	cfe853e66be (hook-list.h: add a generated list of hooks, like config-list.h, 2021-09-26)
	d7a5649c82d (make git-bugreport a builtin, 2020-08-13)
	b5dd96b70ac (make credential helpers builtins, 2020-08-13)

Doing that currently requires bouncing things off the Windows CI. With ab/cmake-nix-and-ci you can not only build (which currently works) but run the full test against the cmake build on *nix in minutes. That's a big improvement.

I think this also misrepresents the nature of the cmake recipe, and how much of it is truly MSVC or VS-specific. I count less than 20 lines in a ~1.1k line recipe that are really "MSVC"-specific. I.e. guarded by "if" branches checking "MSVC" and 'CMAKE_C_COMPILER_ID STREQUAL "MSVC"'.

The rest are general in nature. E.g. you can run the tests with "ctest" from the VS GUI.

That's not because there's a VS-specific "hey Visual Studio, here's our tests" part of the recipe. Rather there's a generic cross-platform method of declaring how to run the tests, which cmake itself then knows to pick up and generate a VS-specific asset with.

Whereas you seem to be suggesting that the recipe is so
Windows+VS-specific that testing it on other platforms isn't going to
tell you much. I don't think that's true.

> But the patches already did introduce Windows-specific problems merely
> by trying to get the Linux side of CMake to work.

This seems like vague commentary on past bugs.

Do you have any specific concerns about things that are broken by the current v6 iteration?

> And trying to keep CMake
> working both on Linux and on Windows would cause many more problems in the
> future. And we do not even need CMake support for Linux, `make` works well
> there already. It would increase the maintenance burden unnecessarily.

If you're going to argue that "we do not even need CMake support for Linux" you're not making an argument against ab/cmake-nix-and-ci, but against the status quo on "master".

It's already the case that it mostly "works" on Linux, that's been the case since day 1. E.g. Yuyi's a561962479c (cmake: fix CMakeLists.txt on Linux, 2022-05-24) earlier this year.

> I am therefore suggesting to drop `cmake-nix-and-ci` entirely.

I wouldn't mind if we declare that it should never work on Linux, and I wouldn't mind if we drop ab/cmake-nix-and-ci entirely.

But only *if* the cmake recipe becomes purely a "lag-behind" alternative build system that the Windows folks are responsible for updating, or to submit follow-up patches for if it breaks.

That's not the status quo now, as e.g. Jeff King summarized nicely here:

	https://lore.kernel.org/git/Y4qF3iHW2s+I0yNe@coredump.intra.peff.net/

You don't seem to be suggesting a productive way forward with that.

I don't think dropping it, or even making the Windows cmake CI optional would be a productive way forward, I think it would ultimately waste more of your time, and that of other Windows developers.

But I don't see how that isn't the logical conclusion to not providing specific feedback on this topic & finding a way forward with it.