mingw: include the Python parts in the build

From: Johannes Schindelin <[email protected]>

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.

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

index ce83cad47a2..7fc924d0364 100644
@@ -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

"Johannes Schindelin via GitGitGadget" <[email protected]>

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

index ce83cad47a..999a7ae270 100644
@@ -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)

Hi Junio,

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/[email protected]/
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?

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

Am 29.07.22 um 16:29 schrieb Johannes Schindelin:

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

Hi Hannes,

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

Ciao,
Dscho

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 <[email protected]>

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

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

From: Johannes Schindelin <[email protected]>

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.

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

index ce83cad47a2..fd7b6a90429 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 config.mak.uname | 1 -
 1 file changed, 1 deletion(-)

index fd7b6a90429..e897b80b3a7 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 config.mak.uname | 1 -
 1 file changed, 1 deletion(-)

index e897b80b3a7..d63629fe807 100644
@@ -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

Hi,

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

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?

Hi Junio,

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