mingw: include the Python parts in the build #1306
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1306.git.1659016906707.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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) |
|
This branch is now known as |
|
This patch series was integrated into seen via git@fc4cddd. |
|
This patch series was integrated into seen via git@2ab40de. |
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>
In f9206ce (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>
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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)
> |
In df5218b (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>
|
/submit |
|
Submitted as pull.1306.v2.git.1659109272.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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?
|
|
On the Git mailing list, Johannes Sixt wrote (reply to this): 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 |
|
User |
|
This patch series was integrated into seen via git@59cf9b3. |
|
There was a status update in the "New Topics" section about the branch Conditionally allow building Python interpreter on Windows Will merge to 'next'. source: <pull.1306.v2.git.1659109272.gitgitgadget@gmail.com> |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@NELLIETHA Please use your own fork for practicing how to use GitHub. |
|
This patch series was integrated into seen via git@50a98ad. |
|
This patch series was integrated into next via git@73b8f06. |
|
There was a status update in the "Cooking" section about the branch Conditionally allow building Python interpreter on Windows Will merge to 'master'. source: <pull.1306.v2.git.1659109272.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@5a3a405. |
|
This patch series was integrated into seen via git@0f06e7a. |
|
There was a status update in the "Cooking" section about the branch Conditionally allow building Python interpreter on Windows Will merge to 'master'. source: <pull.1306.v2.git.1659109272.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@fbecabf. |
|
This patch series was integrated into seen via git@8dfa09f. |
|
This patch series was integrated into master via git@8dfa09f. |
|
This patch series was integrated into next via git@8dfa09f. |
|
Closed via 8dfa09f. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
NO_PYTHON, it is now only defined in the relevant parts of the Windows-specific section ofconfig.mak.uname.NO_GETTEXT; Let's remove it.NO_CURL: No need to define it to the empty value.cc: Johannes Sixt j6t@kdbg.org