Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable MINGW32-specific compile options #5470

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

dscho
Copy link
Member

@dscho dscho commented Mar 6, 2025

This reverts a mistake of mine: My change to introduce an "else ifeq" in #5439 chain missed that there is an else clause that should be used for MINGW32 and MSys (to add large-address-awareness and use 32-bit time_t) but not for CLANGARM64 nor for MINGW64.

This reverts a mistake of mine: My change to introduce an "else ifeq"
chain missed that there is an `else` clause that should be used for
MINGW32 (to add large-address-awareness and use 32-bit `time_t`) but not
for CLANGARM64 nor for MINGW64.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho added this to the Next release milestone Mar 6, 2025
@dscho dscho self-assigned this Mar 6, 2025
@dscho dscho mentioned this pull request Mar 6, 2025
@jeremyd2019
Copy link

Why would you still want 32-bit time_t? And why would you want those flags for msys?

@dscho
Copy link
Member Author

dscho commented Mar 6, 2025

Why would you still want 32-bit time_t? And why would you want those flags for msys?

Good question. I instinctively wanted to reinstate what I had mistakenly removed. Now, there's this:

#if defined (_WIN32) && !defined (_WIN64) && !defined (__MINGW_USE_VC2005_COMPAT) && !defined (_UCRT)
#ifndef _USE_32BIT_TIME_T
#define _USE_32BIT_TIME_T
#endif
#endif

But that's mingw-w64's headers, and historically I did not mean to rely on those for the MINGW builds.

I guess the more important idea was to maintain backwards-compatibility because I thought that Git's on-disk representation might be careless enough to use time_t. But now that I looked, it seems as if only the packed_git structure uses this type, and I don't think that this is read back verbatim from a cached version on disk (which is the scenario I want to be careful about: When Git reads data that is potentially written by an older Git version).

So I guess for the 32-bit time_t, it is basically just tradition. And since Git for Windows, as per #5394 and related tickets, will abandon 32-bit builds in 2029 (i.e. well in advance of 2038), I am not worried about the 32-bit time_t, either.

A more important issue here, then, is the large address aware stuff. I cannot find the documentation rn but I vaguely remember that without this option, you cannot allocate regions larger than 2GB on i686, and we very much want to be able to do that.

@jeremyd2019
Copy link

ok. yes, the large address aware flag does opt a process into addresses with the most significant bit set. It is not necessary to set it for msys though, it seems to be set by default already.

@dscho
Copy link
Member Author

dscho commented Mar 7, 2025

ok. yes, the large address aware flag does opt a process into addresses with the most significant bit set. It is not necessary to set it for msys though, it seems to be set by default already.

It also does not do any harm and my thinking was the this else clause would catch whatever obscure setup some random (MINGW-based?) developer might have.

@dscho dscho merged commit 6ac2e9e into git-for-windows:main Mar 10, 2025
59 checks passed
@dscho dscho deleted the fix-mingw32-builds branch March 10, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants