Re: [PATCH] mingw: include the Python parts in the build
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
endifBy 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)
>
Johannes Schindelin via GitGitGadget
