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

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

[Johannes Schindelin via GitGitGadget]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    mingw: include the Python parts in the build
    
    I've actually had this patch in Git for Windows ever since February
    2015.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1306%2Fdscho%2Fmsys2-python-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1306/dscho/msys2-python-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1306

 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index ce83cad47a2..7fc924d0364 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -722,6 +722,7 @@ else
 		USE_LIBPCRE = YesPlease
 		NO_CURL =
 		USE_NED_ALLOCATOR = YesPlease
+		NO_PYTHON =
 		ifeq (/mingw64,$(subst 32,64,$(prefix)))
 			# Move system config into top-level /etc/
 			ETC_GITCONFIG = ../etc/gitconfig

base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
-- 
gitgitgadget

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

[Junio C Hamano]

"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.

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.  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)

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

[Johannes Schindelin]

Hi Junio,

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

> "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?

> 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

> 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)
>

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

[Johannes Sixt]

Am 29.07.22 um 16:29 schrieb Johannes Schindelin:
> 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?

Thank you for keeping me in the loop. No, I have long since switched to
the Git for Windows tool set, i.e., MSYS2 + MinGW64. I don't know if
anybody is still using MSys1 + MinGW. There's likely no reason to keep
the MSys1 config section.

-- Hannes

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

[Johannes Schindelin]

Hi Hannes,

On Fri, 29 Jul 2022, Johannes Sixt wrote:

> Am 29.07.22 um 16:29 schrieb Johannes Schindelin:
> > 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?
>
> Thank you for keeping me in the loop. No, I have long since switched to
> the Git for Windows tool set, i.e., MSYS2 + MinGW64. I don't know if
> anybody is still using MSys1 + MinGW. There's likely no reason to keep
> the MSys1 config section.

Excellent. I've added a #leftoverbits ticket here:
https://github.com/gitgitgadget/git/issues/1319

Ciao,
Dscho

[PATCH v2 0/3] mingw: include the Python parts in the build

[Johannes Schindelin via GitGitGadget]

I've actually had a variation of the patch to include th Python bits in Git
for Windows's build ever since February 2015.

Changes since v1:

 * Instead of setting and then overriding NO_PYTHON, it is now only defined
   in the relevant parts of the Windows-specific section of
   config.mak.uname.
 * As Junio pointed out, there is an unneeded empty definition of
   NO_GETTEXT; Let's remove it.
 * The same holds for NO_CURL: No need to define it to the empty value.

Johannes Schindelin (3):
  windows: include the Python bits when building Git for Windows
  mingw: remove unneeded `NO_GETTEXT` directive
  mingw: remove unneeded `NO_CURL` directive

 config.mak.uname | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1306%2Fdscho%2Fmsys2-python-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1306/dscho/msys2-python-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1306

Range-diff vs v1:

 -:  ----------- > 1:  5d9b087625a windows: include the Python bits when building Git for Windows
 -:  ----------- > 2:  019fb837d68 mingw: remove unneeded `NO_GETTEXT` directive
 1:  a5739b9cce8 ! 3:  7dc0a1a9aa8 mingw: include the Python parts in the build
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    mingw: include the Python parts in the build
     +    mingw: remove unneeded `NO_CURL` directive
      
     -    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.
     +    In df5218b4c30 (config.mak.uname: support MSys2, 2016-01-13), we
     +    introduced support for building Git for Windows in the then-brand new
     +    Git for Windows v2.x build environment that was based off of MSYS2.
     +
     +    To do that, we split the non-msysGit part (that targeted MSys1) in two,
     +    and instead of sharing the `NO_CURL = YesPlease` setting with MSys1, we
     +    overrode it for MSYS2 with the empty value because we very much want to
     +    build Git for Windows with libcurl.
     +
     +    But that was unnecessary: we never set that variable beforehand,
     +    therefore there is no need to override it.
     +
     +    Let's just remove that unnecessary line.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## config.mak.uname ##
      @@ config.mak.uname: else
     + 		HAVE_LIBCHARSET_H = YesPlease
     + 		USE_GETTEXT_SCHEME = fallthrough
       		USE_LIBPCRE = YesPlease
     - 		NO_CURL =
     +-		NO_CURL =
       		USE_NED_ALLOCATOR = YesPlease
     -+		NO_PYTHON =
       		ifeq (/mingw64,$(subst 32,64,$(prefix)))
       			# Move system config into top-level /etc/
     - 			ETC_GITCONFIG = ../etc/gitconfig

-- 
gitgitgadget

[PATCH v2 1/3] windows: include the Python bits when building Git for Windows

[Johannes Schindelin via GitGitGadget]

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.

Previously, we excluded the Python bits, mostly due to historical
reasons: In the Git for Windows v1.x days, we built Git using
MSys/MinGW, without support for any Python scripts.

Therefore, let's move out the `NO_PYTHON` definition from the generic
part of the MINGW section (which includes special handling for MSYS2/Git
for Windows, for the long-superseded msysGit environment, as well as for
the setup of probably just one developer remaining with their MSys1)
into the two sections that cover different environments than Git for
Windows' SDK.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index ce83cad47a2..fd7b6a90429 100644
--- a/config.mak.uname
+++ b/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)
@@ -730,6 +730,7 @@ else
 	else
 		COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO
 		NO_CURL = YesPlease
+		NO_PYTHON = YesPlease
 	endif
 endif
 endif
-- 
gitgitgadget

[PATCH v2 2/3] mingw: remove unneeded `NO_GETTEXT` directive

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In f9206ce2681 (mingw: let's use gettext with MSYS2, 2016-01-26), we
flipped the switch to build Git for Windows with support for gettext.

However, the way we flipped the switch was by changing the value of the
`NO_GETTEXT` variable from a non-empty string to the empty string, as if
there was any `NO_GETTEXT` definition we needed to override.

But that was a mistake: while there _is_ a definition, it is in the
`THIS_IS_MSYSGIT` section, i.e. it does not affect the Git for Windows
part at all.

Let's just remove that unnecessary line.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 1 -
 1 file changed, 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index fd7b6a90429..e897b80b3a7 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -717,7 +717,6 @@ else
 		INSTALL = /bin/install
 		INTERNAL_QSORT = YesPlease
 		HAVE_LIBCHARSET_H = YesPlease
-		NO_GETTEXT =
 		USE_GETTEXT_SCHEME = fallthrough
 		USE_LIBPCRE = YesPlease
 		NO_CURL =
-- 
gitgitgadget

[PATCH v2 3/3] mingw: remove unneeded `NO_CURL` directive

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In df5218b4c30 (config.mak.uname: support MSys2, 2016-01-13), we
introduced support for building Git for Windows in the then-brand new
Git for Windows v2.x build environment that was based off of MSYS2.

To do that, we split the non-msysGit part (that targeted MSys1) in two,
and instead of sharing the `NO_CURL = YesPlease` setting with MSys1, we
overrode it for MSYS2 with the empty value because we very much want to
build Git for Windows with libcurl.

But that was unnecessary: we never set that variable beforehand,
therefore there is no need to override it.

Let's just remove that unnecessary line.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.mak.uname | 1 -
 1 file changed, 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index e897b80b3a7..d63629fe807 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -719,7 +719,6 @@ else
 		HAVE_LIBCHARSET_H = YesPlease
 		USE_GETTEXT_SCHEME = fallthrough
 		USE_LIBPCRE = YesPlease
-		NO_CURL =
 		USE_NED_ALLOCATOR = YesPlease
 		ifeq (/mingw64,$(subst 32,64,$(prefix)))
 			# Move system config into top-level /etc/
-- 
gitgitgadget

Re: [PATCH v2 0/3] mingw: include the Python parts in the build

[Johannes Schindelin]

Hi,

On Fri, 29 Jul 2022, Johannes Schindelin via GitGitGadget wrote:

> Range-diff vs v1:
>
>  -:  ----------- > 1:  5d9b087625a windows: include the Python bits when building Git for Windows
>  -:  ----------- > 2:  019fb837d68 mingw: remove unneeded `NO_GETTEXT` directive
>  1:  a5739b9cce8 ! 3:  7dc0a1a9aa8 mingw: include the Python parts in the build
>      @@ Metadata
>       Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
>        ## Commit message ##
>      -    mingw: include the Python parts in the build
>      +    mingw: remove unneeded `NO_CURL` directive
>
>      -    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.
>      +    In df5218b4c30 (config.mak.uname: support MSys2, 2016-01-13), we
>      +    introduced support for building Git for Windows in the then-brand new
>      +    Git for Windows v2.x build environment that was based off of MSYS2.
>      +
>      +    To do that, we split the non-msysGit part (that targeted MSys1) in two,
>      +    and instead of sharing the `NO_CURL = YesPlease` setting with MSys1, we
>      +    overrode it for MSYS2 with the empty value because we very much want to
>      +    build Git for Windows with libcurl.
>      +
>      +    But that was unnecessary: we never set that variable beforehand,
>      +    therefore there is no need to override it.
>      +
>      +    Let's just remove that unnecessary line.
>
>           Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
>        ## config.mak.uname ##
>       @@ config.mak.uname: else
>      + 		HAVE_LIBCHARSET_H = YesPlease
>      + 		USE_GETTEXT_SCHEME = fallthrough
>        		USE_LIBPCRE = YesPlease
>      - 		NO_CURL =
>      +-		NO_CURL =
>        		USE_NED_ALLOCATOR = YesPlease
>      -+		NO_PYTHON =
>        		ifeq (/mingw64,$(subst 32,64,$(prefix)))
>        			# Move system config into top-level /etc/
>      - 			ETC_GITCONFIG = ../etc/gitconfig

Oh, that's funny. This is actually the first time I personally see
`range-diff` matching up a wrong patch pair (because it really looks for
the minimal diff between the diffs). It is of course nonsense to match up
the original patch with the `NO_CURL` patch.

Ciao,
Dscho

Re: [PATCH v2 0/3] mingw: include the Python parts in the build

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 29 Jul 2022, Johannes Schindelin via GitGitGadget wrote:
>
>> Range-diff vs v1:
>>
>>  -:  ----------- > 1:  5d9b087625a windows: include the Python bits when building Git for Windows
>>  -:  ----------- > 2:  019fb837d68 mingw: remove unneeded `NO_GETTEXT` directive
>>  1:  a5739b9cce8 ! 3:  7dc0a1a9aa8 mingw: include the Python parts in the build
> ...
> Oh, that's funny. This is actually the first time I personally see
> `range-diff` matching up a wrong patch pair (because it really looks for
> the minimal diff between the diffs). It is of course nonsense to match up
> the original patch with the `NO_CURL` patch.

It would depend on the creation-factor number, I suspect.  To me, it
does not seem to match anything at all, but with an unreasonably
high number like 9999, I see 1 corresponds to the old one, with the
other two follow-up patch as new.

As the maintainer, I mostly use range-diff to compare two iterations
of a single topic, and not "compare 'seen' from 24 hours ago with
'seen' I just rebuilt, so that I can match up everything in an
uncontrolled mess", so the optimum factor number would be different
for my usecase from the one used for general use (which is
documented to be 60).

The "maintainer" use case compares two iterations that are known and
expected to have corresponding patches (and no corresponding one
means either dropped or added), and come to think of it, the use
case for submitter to run "format-patch --range-diff" shares exactly
the same expectation.  It is very different from "pick corresponding
patches from two piles of many unrelated topics" use case, in which
"range-diff" proper can be used.

Perhaps the default used for "format-patch" should become different
and set a lot higher than the default for "range-diff" proper?

Overriding creation factor in format-patch, was Re: [PATCH v2 0/3] mingw: include the Python parts in the build

[Johannes Schindelin]

Hi Junio,

On Fri, 29 Jul 2022, Junio C Hamano wrote:

> Perhaps the default used for "format-patch" should become different
> and set a lot higher than the default for "range-diff" proper?

Good idea.

However, this will require careful research about the best value to use.

An idea would be to use the lore archive to extract patch series
iterations that have been sent to the Git mailing list, then use a
variation of https://github.com/dscho/git/tree/range-diff-from-mbox to
compare them using `range-diff` with multiple creation factors to
determine the bounds within which the optimal value lies.

Sadly a bit too involved a project for me to take on right now.

Ciao,
Dscho