git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Philip Oakley <philipoakley@iee.email>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Bagas Sanjaya <bagasdotme@gmail.com>
Cc: Matthew Rogers via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Sibi Siddharthan <sibisiddharthan.github@gmail.com>,
	Danh Doan <congdanhqx@gmail.com>,
	Matthew Rogers <mattr94@gmail.com>
Subject: Re: [PATCH 0/3] Make CMake work out of the box
Date: Fri, 18 Jun 2021 14:05:39 +0100	[thread overview]
Message-ID: <b52d7c45-06dd-a9eb-bc86-e04d2ee16c5e@iee.email> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2106101140590.57@tvgsbejvaqbjf.bet>

On 10/06/2021 10:43, Johannes Schindelin wrote:
> 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.
>> 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.

  reply	other threads:[~2021-06-18 13:05 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-04 17:43 [PATCH 0/3] Make CMake work out of the box Matthew Rogers via GitGitGadget
2021-06-04 17:43 ` [PATCH 1/3] cmake: add knob to disable vcpkg Matthew Rogers via GitGitGadget
2021-06-04 18:03   ` Eric Sunshine
2021-06-04 18:34     ` Matt Rogers
2021-06-04 20:55   ` Sibi Siddharthan
2021-06-05 22:30     ` Matt Rogers
2021-06-06  4:33       ` Sibi Siddharthan
2021-06-04 17:43 ` [PATCH 2/3] cmake: create compile_commands.json by default Matthew Rogers via GitGitGadget
2021-06-04 18:05   ` Eric Sunshine
2021-06-04 21:09   ` Sibi Siddharthan
2021-06-05 22:36     ` Matt Rogers
2021-06-06  4:39       ` Sibi Siddharthan
2021-06-04 17:43 ` [PATCH 3/3] cmake: add warning for ignored MSGFMT_EXE Matthew Rogers via GitGitGadget
2021-06-04 18:10   ` Eric Sunshine
2021-06-05  3:40 ` [PATCH 0/3] Make CMake work out of the box Bagas Sanjaya
2021-06-05 23:22   ` Matt Rogers
2021-06-10  9:43   ` Johannes Schindelin
2021-06-18 13:05     ` Philip Oakley [this message]
2021-06-18 13:42       ` Johannes Schindelin
2021-06-18 14:03         ` Philip Oakley
2021-06-22 22:32           ` Johannes Schindelin
2021-06-06 12:02 ` [PATCH v2 " Matthew Rogers via GitGitGadget
2021-06-06 12:02   ` [PATCH v2 1/3] cmake: add knob to disable vcpkg Matthew Rogers via GitGitGadget
2021-06-06 12:02   ` [PATCH v2 2/3] cmake: create compile_commands.json by default Matthew Rogers via GitGitGadget
2021-06-06 12:02   ` [PATCH v2 3/3] cmake: add warning for ignored MSGFMT_EXE Matthew Rogers via GitGitGadget
2021-06-07  0:54   ` [PATCH v2 0/3] Make CMake work out of the box Junio C Hamano
2021-06-10  9:45     ` Johannes Schindelin
2021-06-18 13:11       ` Philip Oakley
2021-06-18 13:09     ` Philip Oakley
2021-06-10  9:47   ` Johannes Schindelin
2021-06-11  6:22     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b52d7c45-06dd-a9eb-bc86-e04d2ee16c5e@iee.email \
    --to=philipoakley@iee.email \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bagasdotme@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=mattr94@gmail.com \
    --cc=sibisiddharthan.github@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).