Re: [PATCH] mingw: include the Python parts in the build

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 29, 2022, 14:29 UTC
Message-ID: <0s3o2pr2-o0q3-q394-83n3-n117355o8o29@tzk.qr>
In-Reply-To: <xmqqczdpcfhx.fsf@gitster.g>

Hi Junio,

On Thu, 28 Jul 2022, Junio C Hamano wrote:

Show 26 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > While Git for Windows does not _ship_ Python (in order to save on
> > bandwidth), MSYS2 provides very fine Python interpreters that users can
> > easily take advantage of, by using Git for Windows within its SDK.
>
> It may be an accurate description of the world and there may not be
> anything incorrect in the above statement, but it took quite an
> effort to try matching that statement to what the patch does.
>
> I think
>
>     Builds on $uname_S==MINGW by default sets NO_PYTHON=YesPlease
>     and it benefits Git for Windows by allowing to omit Python.
>     However, when "Git for Windows" is used within MSYS2's SDK, we
>     can allow users to take advantage of Python interpreter that
>     comes with it.  Override NO_PYTHON when the presence of
>     ../THIS_IS_MSYSGIT indicates that we are in that situation.
>
> is how the logic in this patch can be explained, but I have to
> wonder if a more natural and easier-to-understand solution is to
> move NO_PYTHON=YesPlease into "if we do not have ../THIS_IS_MSYSGIT,
> do these things" ifneq() block, like the attached patch.

The outline is:

ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
	[...]
else
        ifneq ($(shell expr "$(uname_R)" : '1\.'),2)
                # MSys2
		[...]
	else
		[...]
	endif
endif

By moving it into the `THIS_IS_MSYSGIT` part, you changed the behavior of the MSys part (which is in the `else` block of that `uname_R` conditional).

Now, I vaguely remember that j6t said that they switched to MSYS2 some time ago, but all I found on the Git mailing list was https://lore.kernel.org/git/6c2bbca7-7a8f-d3d8-04b6-31494a3e1b43@kdbg.org/ which says that in 2017, MSys1 was still used by the person who apart from myself did the most crucial work to support Git on Windows (and that counts a lot in my book, so in this instance I am willing to bear a bit more maintenance burden than I otherwise would for a single person, even if the Windows-specific part of `config.mak.uname` is quite messy, I admit).

Hannes, do you still build with MSys1?

Show 5 quoted lines

> I didn't touch it but NO_GETTEXT does not appear in the common
> section above "do we have ../THIS_IS_MSYSGIT?", and gets set
> after "we do not have ../THIS_IS_MSYSGIT", so I do not think
> we need "NO_GETTEXT = " that clears it in the "we do have
> ../THIS_IS_MSYSGIT" part.

True. This is my mistake: in f9206ce2681 (mingw: let's use gettext with MSYS2, 2016-01-26), I should have looked more closely and realized that `NO_GETTEXT` is not defined in the MSYS2-specific part of `config.mak.uname`, and hence the line should not have changed to `NO_GETTEXT =` but it should have been removed instead.

I'll revamp the patch and send another iteration (but please do not expect any further work from me this coming week, I plan on staying off of work).

Ciao, Dscho

Show 27 quoted lines

> We may want to see if there are other things that needs cleaning up
> around this area.
>
>  config.mak.uname | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git i/config.mak.uname w/config.mak.uname
> index ce83cad47a..999a7ae270 100644
> --- i/config.mak.uname
> +++ w/config.mak.uname
> @@ -656,7 +656,6 @@ ifeq ($(uname_S),MINGW)
>  	UNRELIABLE_FSTAT = UnfortunatelyYes
>  	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
>  	NO_REGEX = YesPlease
> -	NO_PYTHON = YesPlease
>  	ETAGS_TARGET = ETAGS
>  	NO_POSIX_GOODIES = UnfortunatelyYes
>  	DEFAULT_HELP_FORMAT = html
> @@ -686,6 +685,7 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
>  	INTERNAL_QSORT = YesPlease
>  	HAVE_LIBCHARSET_H = YesPlease
>  	NO_GETTEXT = YesPlease
> +	NO_PYTHON = YesPlease
>  	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS
>  else
>  	ifneq ($(shell expr "$(uname_R)" : '1\.'),2)
>