Subject: Re: cmake topics & js/ci-disable-cmake-by-default

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 27 Dec 2022 14:59:20 +0100

On Tue, Dec 27 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <[email-masked]> writes:
>
>> I split up the previously merged to "next" ab/cmake-nix-and-ci and
>> submitted the uncontroversial parts of it as:
>
> Not gathering any interest by folks who will be affected is
> different from being uncontroversial, though.  It may not have seen
> any controversy so far, but once it reappears in my tree and
> sufficiently advances to cause trouble to other people, it would.
>
> In other words, I am saving time and energy of people by waiting for
> positive support on these changes.

These changes had made it to "next" already on the basis of the feedback
the topic already got.

Now, that's not justification in itself that a re-roll & split-up
belongs there again, but it also strikes me as a bad use of time to just
throw them away at this point now that I've submitted a version that
addresses the trouble it caused others (which I think the combined v6
also fixed, but as the topic is fairly large I think this split-up was
prudent).

>> I think whatever happens with js/ci-disable-cmake-by-default that it
>> makes sense to pick up & integrate those.
>
> I do not think so at all, at least judging by what little has been
> said so far on the list.  Comments on two among these three are
> negative ones, and the other one had no traction.

I'm assuming you're counting your own
https://lore.kernel.org/git/[email-masked]/ as negative
here, I just replied at
https://lore.kernel.org/git/[email-masked]/,
but maybe you're still not convinced.

In any case, that 1/3 topic could be discarded without the other 2/3
being affected, even though it seems to me to be a trivially correct
fix.

If you mean
https://lore.kernel.org/git/[email-masked]/
I'm not sure what to do with that other than my reply at
https://lore.kernel.org/git/[email-masked]/.

I don't see how I, having spent quite a bit of time untangling the
platform-specific parts of the CMake recipe, would be inherently
unqualified to spot and clarify the CMake docs in such a way that they
make it clear what parts are the cross-platform parts, and which are the
platform-specific parts.

>>> * js/ci-disable-cmake-by-default (2022-12-20) 1 commit
>>>  - ci: only run win+VS build & tests in Git for Windows' fork
>>>
>>>  Stop running win+VS build by default.
>>>
>>>  Will merge to 'next'?
>>>  source: <[email-masked]>
>>
>> Per my feedback there, I still think it would make sense to at least
>> split up the "should we build with MSVC?" from the "do we use cmake, and
>> run the re-run tests we already ran with GCC with MSVC too?".
>
> Do you mean that in our primary CI jobs, you would, using Makefile,
> want to keep building the win+VS artifacts with MSVC and running the
> tests, even though Windows folks want to drive the same build
> process via CMake, and their release binaries will come from the
> latter?  

I don't think "running the tests" is a good use of time, although we
might want to do so if some "extended tests" ci-config option is set,
which e.g. "next" and/or "master" might want to turn on.

So just like we run tests on OSX for both the git built with gcc and
clang (and yes, that isn't the only difference, we sneak in the "no
sha1dc" there too) perhaps we'd like to run the tests for the MSVC-built
git too.

> I am not sure which extra corners, which matter to us, are covered
> by doing so.  What's the upside?

Doing the *build* is relatively cheap when it comes to CI time, and
would catch some portability issues.

In any case, I'm not asserting here that js/ci-disable-cmake-by-default
must do what I'm suggesting in the last few paragraphs.

But rather that its commit message doesn't really cover what it's
actually doing, and that at least adjusting that would be
worthwhile. It's saying that it's disabling the "build and test with
cmake + MSVC" entirely because the tests are slow, and that cmake isn't
wanted in git/git.

Whereas the more obvious conclusion IMO is to then undo the later change
to that job to use "cmake" to build with "MSVC" (which was a later
development) and/or don't run the test part.

>> But now we won't even run that in CI, and "git-for-windows" will have
>> ownership of it.
>>
>> Does that mean that for such Makefile changes we should simply leave out
>> the cmake changes, and rely on git-for-windows to "catch up" with its
>> cmake contrib component?
>
> That is the natural conclusion of what has been said on the list so
> far.  We do not even "rely on"---it is up to them who chose to use
> CMake to keep it up to date or lag behind.  Among those who have
> made significant contributions and works outside Windows, we found
> nobody whowants to touch CMake.

Thanks, that's pretty much the clarification I was looking for. I do
think it's going to result in more disruption than is strictly
necessary, but *shrug*.

I do think it would be good to prominently note this at the top of
CMakeLists.txt, i.e. that the version in git/git can be expected to be
broken or out of date, I could re-roll the "cmake doc" topic I have
outstanding to add that, except...

>> Ultimately I don't mind such an arrangement, but I think that
>> js/ci-disable-cmake-by-default brings us to a weird in-between
>> state. Just removing it from the tree and having git-for-windows carry
>> it would make sense.
>
> That's fine by me personally, but somebody has to help coordinating
> such a move between two projects.

...(continuing from above). I'd be happy to do that, even though I'm
still surprised that this is the path Johannes seems to prefer going
forward.

As it's surely more work for git-for-windows to deal with whatever we
throw over the wall at them, rather than have us take them into account
(which would have been easy once we CI'd it on Ubuntu:
https://lore.kernel.org/git/[email-masked]/).

But *shrug*.

I think if we're going down this path it would make sense to first start
by removing the support for building with MSVC and generating the VS
project files from the Makefile.

There's more to it than this, but it's basically this & adjacent code
(e.g. in config.mak.uname):
	
	$ wc -l $(git ls-files 'contrib/buildsystems/Generators*' 'compat/vcbuild/scripts*' compat/vcbuild/find_vs_env.bat)
	 168 compat/vcbuild/find_vs_env.bat
	 133 compat/vcbuild/scripts/clink.pl
	  26 compat/vcbuild/scripts/lib.pl
	  42 contrib/buildsystems/Generators.pm
	 189 contrib/buildsystems/Generators/QMake.pm
	 579 contrib/buildsystems/Generators/Vcproj.pm
	 402 contrib/buildsystems/Generators/Vcxproj.pm
	1539 total

This has all been bitrotting since 4c2c38e800f (ci: modification of
main.yml to use cmake for vs-build job, 2020-06-26), and it's been
broken since sometime in 2020 for DEVELOPER=1 builds at least (started
failing with new warnings, and then the C99 change in
git-compat-util.h).

One reason I thought keeping the CMake recipe up-to-date & in-tree is
that it's much easier to maintain it than e.g. the hack that is
compat/vcbuild/scripts/clink.pl, which we'd need (or something like it)
if we're going to support building with MSVC with the Makefile.

But I don't see how it would make sense to have CMakeLists.txt
bitrotting in-tree, and for git/git to pick up the maintenance of this
"build using MSVC with the Makefile", which git-for-windows itself seems
to have abandoned.

(The actual release versions of GfW are, as I understand it, built with
GCC, so the only "cmake with MSVC" users are those using some
lightweight version of MS development tools)

Then, as a follow-up to that, if improvements to
contrib/buildsystems/CMakeLists.txt aren't going to be accepted to
git/git.

Or rather, if they would in principle, but are going to stall because
they might impact git-for-windows. As seems to be the case with my
proposed, which have had no timely feedback from the git-for-windows
folks on "upstreaming" those (or rather, fixing "upstream").

Then shouldn't we just:

	git rm contrib/buildsystems/CMakeLists.txt

Along with the other bits in-tree which at that point would only be used
for CMake, e.g. compat/vcbuild/vcpkg_install.bat?

I could submit all of that if you agree. This would not be my first
choice, but I don't think it's good to leave this in limbo in git/git
either.

The "git-for-windows" project would then just revert the part that
removes contrib/buildsystems/CMakeLists.txt once it lands there, and
proceed to maintain contrib/buildsystems/CMakeLists.txt only in that
project, along with the diverging part of main.yml etc.

What do you think?