[RFC/PATCH v1] Towards MinGW(-W64) cross-compilation

[RFC/PATCH v1] Towards MinGW(-W64) cross-compilation

[Marat Radchenko]

This patch series fixes building on modern MinGW and (32bit only yet) MinGW-W64.

*Compilation* tested on:
 - MSVC (via WinGit environment)
 - msysGit environment
 - Linux cross-toolchain i686-pc-mingw32 (4.8.2) with mingw-runtime-3.20.2
 - Linux cross-toolchain i686-w64-mingw32 (4.8.2) with mingw64-runtime-3.1.0

Stuff still required to make Git build with x86_64 MinGW-W64 toolchain:

1. Drop -D_USE_32BIT_TIME_T that was added in fa93bb to config.mak.uname
because time_t cannot be 32bit on x86_64. I haven't yet figured out what
should break if this define is removed (pointers are welcome) and why it was
added in the first place.

2. Stop passing --large-address-aware to linker. I wonder if it does anything
for 32bit MinGW builds.

3. Fix several places with mismatched pointer size casts.

Building it from Gentoo Linux:

MinGW:

  crossdev -t i686-pc-mingw32
  ARCH=x86 emerge-i686-pc-mingw32 -u dev-libs/libiconv sys-libs/zlib net-misc/curl sys-devel/gettext expat
  cd <git>
  make CROSS_COMPILE=i686-pc-mingw32- CC=i686-pc-mingw32-gcc NO_OPENSSL=1 MINGW=1 CURLDIR=/usr/i686-pc-mingw32/usr

MinGW-W64 (32 bit):

  crossdev -t i686-w64-mingw32
  ARCH=x86 emerge-i686-w64-mingw32 -u dev-libs/libiconv sys-libs/zlib net-misc/curl sys-devel/gettext expat
  cd <git>
  make CROSS_COMPILE=i686-w64-mingw32- CC=i686-w64-mingw32-gcc NO_OPENSSL=1 MINGW=1 CURLDIR=/usr/i686-w64-mingw32/usr

Debian/Ubuntu build instructions are WIP (xdeb is non-trivial at all).

[PATCH 01/12] MINGW: config.mak.uname: add explicit way to request MinGW-build

[Marat Radchenko]

When crosscompiling, one cannot rely on `uname` from host system.
Thus, add an option to use `make MINGW=1` for building MinGW build
from non-MinGW host (Linux, for example).

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index 23a8803..5d301da 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -13,6 +13,11 @@ ifdef MSVC
 	uname_O := Windows
 endif
 
+ifdef MINGW
+	uname_S := MINGW
+	uname_O := MINGW
+endif
+
 # We choose to avoid "if .. else if .. else .. endif endif"
 # because maintaining the nesting to match is a pain.  If
 # we had "elif" things would have been much nicer...
-- 
1.9.1

[PATCH 02/12] MINGW: compat/bswap.h: include stdint.h

[Marat Radchenko]

bswap.h uses uint32_t type which might not be defined.
This patch adds direct include so bswap.h can be safely included.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 compat/bswap.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/compat/bswap.h b/compat/bswap.h
index 120c6c1..d170447 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -5,6 +5,8 @@
  * operation.
  */
 
+#include <stdint.h>
+
 /*
  * Default version that the compiler ought to optimize properly with
  * constant values.
-- 
1.9.1

[PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Marat Radchenko]

mingw-w64 has lseek defined in io.h.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 compat/mingw.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/compat/mingw.h b/compat/mingw.h
index e033e72..262b300 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -265,7 +265,9 @@ static inline int getrlimit(int resource, struct rlimit *rlp)
  * Use mingw specific stat()/lstat()/fstat() implementations on Windows.
  */
 #define off_t off64_t
+#ifndef lseek
 #define lseek _lseeki64
+#endif
 
 /* use struct stat with 64 bit st_size */
 #ifdef stat
-- 
1.9.1

[PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Marat Radchenko]

To ease cross-compilation process, introduce a single variable
with the prefix to all compiler-related executables.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 Makefile         | 13 +++++++++----
 config.mak.uname |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 74a929b..24befc2 100644
--- a/Makefile
+++ b/Makefile
@@ -350,6 +350,10 @@ all::
 #
 # Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not
 # return NULL when it receives a bogus time_t.
+#
+# Define CROSS_COMPILE to specify the prefix used for all executables used
+# during compilation. Only gcc and related bin-utils executables
+# are prefixed with $(CROSS_COMPILE).
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -361,7 +365,6 @@ CFLAGS = -g -O2 -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
-STRIP ?= strip
 
 # Among the variables below, these:
 #   gitexecdir
@@ -401,8 +404,11 @@ htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 
 export prefix bindir sharedir sysconfdir gitwebdir localedir
 
-CC = cc
-AR = ar
+AR = $(CROSS_COMPILE)ar
+CC = $(CROSS_COMPILE)cc
+GCOV = $(CROSS_COMPILE)gcov
+STRIP = $(CROSS_COMPILE)strip
+
 RM = rm -f
 DIFF = diff
 TAR = tar
@@ -415,7 +421,6 @@ XGETTEXT = xgettext
 MSGFMT = msgfmt
 PTHREAD_LIBS = -lpthread
 PTHREAD_CFLAGS =
-GCOV = gcov
 
 export TCL_PATH TCLTK_PATH
 
diff --git a/config.mak.uname b/config.mak.uname
index 5d301da..6c2e6df 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -511,7 +511,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	EXTLIBS += -lws2_32
 	GITLIBS += git.res
 	PTHREAD_LIBS =
-	RC = windres -O coff
+	RC = $(CROSS_COMPILE)windres -O coff
 	NATIVE_CRLF = YesPlease
 	X = .exe
 	SPARSE_FLAGS = -Wno-one-bit-signed-bitfield
-- 
1.9.1

[PATCH 05/12] MINGW: git-compat-util.h: use inttypes.h for printf macros.

[Marat Radchenko]

Also, pass -D__USE_MINGW_ANSI_STDIO=0 to select MSVCRT-compatible
macro definitions.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 compat/mingw.h    |  2 --
 compat/msvc.h     |  3 +++
 config.mak.uname  |  3 ++-
 git-compat-util.h | 11 ++++++-----
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 262b300..c502a22 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -342,8 +342,6 @@ static inline char *mingw_find_last_dir_sep(const char *path)
 }
 #define find_last_dir_sep mingw_find_last_dir_sep
 #define PATH_SEP ';'
-#define PRIuMAX "I64u"
-#define PRId64 "I64d"
 
 void mingw_open_html(const char *path);
 #define open_html mingw_open_html
diff --git a/compat/msvc.h b/compat/msvc.h
index 580bb55..cb41ce3 100644
--- a/compat/msvc.h
+++ b/compat/msvc.h
@@ -15,6 +15,9 @@
 #define strtoull     _strtoui64
 #define strtoll      _strtoi64
 
+#define PRIuMAX "I64u"
+#define PRId64 "I64d"
+
 static __inline int strcasecmp (const char *s1, const char *s2)
 {
 	int size1 = strlen(s1);
diff --git a/config.mak.uname b/config.mak.uname
index 6c2e6df..e5edae6 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -321,6 +321,7 @@ ifeq ($(uname_S),Windows)
 	NO_PREAD = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
+	NO_INTTYPES_H = UnfortunatelyYes
 	NO_POLL = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
@@ -502,7 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_INET_NTOP = YesPlease
 	NO_POSIX_GOODIES = UnfortunatelyYes
 	DEFAULT_HELP_FORMAT = html
-	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
+	COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..aa57a04 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,6 +85,12 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
+#ifndef NO_INTTYPES_H
+#include <inttypes.h>
+#else
+#include <stdint.h>
+#endif
+
 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */
 # if defined (_MSC_VER) && !defined(_WIN32_WINNT)
 #  define _WIN32_WINNT 0x0502
@@ -146,11 +152,6 @@
 #include <netdb.h>
 #include <pwd.h>
 #include <sys/un.h>
-#ifndef NO_INTTYPES_H
-#include <inttypes.h>
-#else
-#include <stdint.h>
-#endif
 #ifdef NO_INTPTR_T
 /*
  * On I16LP32, ILP32 and LP64 "long" is the save bet, however
-- 
1.9.1

[PATCH 06/12] MSVC: config.mak.uname: drop -D__USE_MINGW_ACCESS from compile definitions

[Marat Radchenko]

-D__USE_MINGW_ACCESS only affects MinGW and does nothing when
MSVC is used.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index e5edae6..dc87e21 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -363,7 +363,7 @@ ifeq ($(uname_S),Windows)
 	COMPAT_OBJS = compat/msvc.o compat/winansi.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS = -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
 	PTHREAD_LIBS =
-- 
1.9.1

[PATCH 07/12] MINGW: config.mak.uname: reorganize MINGW settings

[Marat Radchenko]

HAVE_LIBCHARSET_H and NO_R_TO_GCC_LINKER are not specific to
msysGit, they're general MinGW settings.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index dc87e21..2f1939e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -521,13 +521,13 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 	prefix =
 	INSTALL = /bin/install
 	EXTLIBS += /mingw/lib/libz.a
-	NO_R_TO_GCC_LINKER = YesPlease
 	INTERNAL_QSORT = YesPlease
-	HAVE_LIBCHARSET_H = YesPlease
 	NO_GETTEXT = YesPlease
 else
 	NO_CURL = YesPlease
 endif
+	HAVE_LIBCHARSET_H = YesPlease
+	NO_R_TO_GCC_LINKER = YesPlease
 endif
 ifeq ($(uname_S),QNX)
 	COMPAT_CFLAGS += -DSA_RESTART=0
-- 
1.9.1

[PATCH 08/12] MINGW: config.mak.uname allow using CURL for non-msysGit builds

[Marat Radchenko]

Also, fix `warning: passing argument 2 of 'mingw_main' from
incompatible pointer type` in http-fetch.c and remote-curl.c.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname | 2 --
 http-fetch.c     | 5 +++--
 remote-curl.c    | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index 2f1939e..a376b32 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -523,8 +523,6 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 	EXTLIBS += /mingw/lib/libz.a
 	INTERNAL_QSORT = YesPlease
 	NO_GETTEXT = YesPlease
-else
-	NO_CURL = YesPlease
 endif
 	HAVE_LIBCHARSET_H = YesPlease
 	NO_R_TO_GCC_LINKER = YesPlease
diff --git a/http-fetch.c b/http-fetch.c
index ba3ea10..a6a9a2f 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -6,7 +6,7 @@
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url";
 
-int main(int argc, const char **argv)
+int main(int argc, char **argv)
 {
 	struct walker *walker;
 	int commits_on_stdin = 0;
@@ -38,7 +38,8 @@ int main(int argc, const char **argv)
 		} else if (argv[arg][1] == 'v') {
 			get_verbosely = 1;
 		} else if (argv[arg][1] == 'w') {
-			write_ref = &argv[arg + 1];
+			const char *ref = argv[arg + 1];
+			write_ref = &ref;
 			arg++;
 		} else if (argv[arg][1] == 'h') {
 			usage(http_fetch_usage);
diff --git a/remote-curl.c b/remote-curl.c
index 52c2d96..565b6c9 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -938,7 +938,7 @@ static void parse_push(struct strbuf *buf)
 	free(specs);
 }
 
-int main(int argc, const char **argv)
+int main(int argc, char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
-- 
1.9.1

[PATCH 09/12] MINGW: config.mak.uname: drop -DNOGDI

[Marat Radchenko]

On MinGW-W64, MsgWaitForMultipleObjects is guarded with #ifndef NOGDI.

Unfortunately, including wingdi.h brings in #define ERROR 0 which
conflicts with several Git internal enums. So, #undef ERROR.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname  | 4 ++--
 git-compat-util.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index a376b32..4883fd5 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -363,7 +363,7 @@ ifeq ($(uname_S),Windows)
 	COMPAT_OBJS = compat/msvc.o compat/winansi.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
-	COMPAT_CFLAGS = -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
+	COMPAT_CFLAGS = -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
 	PTHREAD_LIBS =
@@ -503,7 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_INET_NTOP = YesPlease
 	NO_POSIX_GOODIES = UnfortunatelyYes
 	DEFAULT_HELP_FORMAT = html
-	COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
+	COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -Icompat -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
diff --git a/git-compat-util.h b/git-compat-util.h
index aa57a04..48bf0f7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -98,6 +98,8 @@
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
 #include <winsock2.h>
 #include <windows.h>
+/* wingdi.h defines ERROR=0, undef it to avoid conflicts */
+#undef ERROR
 #define GIT_WINDOWS_NATIVE
 #endif
 
-- 
1.9.1

[PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Marat Radchenko]

nedalloc was initially added in f0ed82 to fix slowness of standard WinXP
memory allocator. Since WinXP is EOLed, this point is no longer valid.

The actual reason behind this commit is incompatibility of malloc.c.h
with MinGW-W64 headers. Alternative solution implies updating nedalloc
to something newer.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname | 2 --
 1 file changed, 2 deletions(-)

diff --git a/config.mak.uname b/config.mak.uname
index 4883fd5..3fea7a8 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -342,7 +342,6 @@ ifeq ($(uname_S),Windows)
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
-	# USE_NED_ALLOCATOR = YesPlease
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
@@ -492,7 +491,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
-	USE_NED_ALLOCATOR = YesPlease
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
-- 
1.9.1

[PATCH 11/12] MINGW: do not fail at redefining pid_t on MinGW-W64

[Marat Radchenko]

pid_t is available in sys/types.h on both MinGW and MinGW-W64

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 compat/mingw.h | 1 -
 compat/msvc.h  | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index c502a22..8850109 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -5,7 +5,6 @@
  * things that are not available in header files
  */
 
-typedef int pid_t;
 typedef int uid_t;
 typedef int socklen_t;
 #define hstrerror strerror
diff --git a/compat/msvc.h b/compat/msvc.h
index cb41ce3..e2fda48 100644
--- a/compat/msvc.h
+++ b/compat/msvc.h
@@ -18,6 +18,8 @@
 #define PRIuMAX "I64u"
 #define PRId64 "I64d"
 
+typedef int pid_t;
+
 static __inline int strcasecmp (const char *s1, const char *s2)
 {
 	int size1 = strlen(s1);
-- 
1.9.1

[PATCH 12/12] MINGW: compat/mingw.h: drop fork() definition

[Marat Radchenko]

fork() is not used in MinGW builds but causes a compiler warning
on x86_64 MinGW-W64: conflicting types for built-in function 'fork'

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 compat/mingw.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 8850109..2fbc8ea 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -89,8 +89,6 @@ static inline int symlink(const char *oldpath, const char *newpath)
 { errno = ENOSYS; return -1; }
 static inline int fchmod(int fildes, mode_t mode)
 { errno = ENOSYS; return -1; }
-static inline pid_t fork(void)
-{ errno = ENOSYS; return -1; }
 static inline unsigned int alarm(unsigned int seconds)
 { return 0; }
 static inline int fsync(int fd)
-- 
1.9.1

Re: [PATCH 02/12] MINGW: compat/bswap.h: include stdint.h

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> bswap.h uses uint32_t type which might not be defined.
> This patch adds direct include so bswap.h can be safely included.
>
> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---
>  compat/bswap.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/compat/bswap.h b/compat/bswap.h
> index 120c6c1..d170447 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -5,6 +5,8 @@
>   * operation.
>   */
>
> +#include <stdint.h>
> +
>  /*
>   * Default version that the compiler ought to optimize properly with
>   * constant values.

Hmm, what's the symptom this fixes? From what I can tell, bswap.h is
included after stdint.h from git-compat-util.h anyway...

Re: [PATCH 02/12] MINGW: compat/bswap.h: include stdint.h

[Marat Radchenko]

On Mon, Apr 28, 2014 at 04:45:43PM +0200, Erik Faye-Lund wrote:
> bswap.h is included after stdint.h from git-compat-util.h anyway...

That only becomes true after PATCH 05 when talking about MinGW.

Will drop this one.

Re: [PATCH 05/12] MINGW: git-compat-util.h: use inttypes.h for printf macros.

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> Also, pass -D__USE_MINGW_ANSI_STDIO=0 to select MSVCRT-compatible
> macro definitions.
>
> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---
>  compat/mingw.h    |  2 --
>  compat/msvc.h     |  3 +++
>  config.mak.uname  |  3 ++-
>  git-compat-util.h | 11 ++++++-----
>  4 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 262b300..c502a22 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -342,8 +342,6 @@ static inline char *mingw_find_last_dir_sep(const char *path)
>  }
>  #define find_last_dir_sep mingw_find_last_dir_sep
>  #define PATH_SEP ';'
> -#define PRIuMAX "I64u"
> -#define PRId64 "I64d"
>
>  void mingw_open_html(const char *path);
>  #define open_html mingw_open_html
> diff --git a/compat/msvc.h b/compat/msvc.h
> index 580bb55..cb41ce3 100644
> --- a/compat/msvc.h
> +++ b/compat/msvc.h
> @@ -15,6 +15,9 @@
>  #define strtoull     _strtoui64
>  #define strtoll      _strtoi64
>
> +#define PRIuMAX "I64u"
> +#define PRId64 "I64d"
> +
>  static __inline int strcasecmp (const char *s1, const char *s2)
>  {
>         int size1 = strlen(s1);
> diff --git a/config.mak.uname b/config.mak.uname
> index 6c2e6df..e5edae6 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -321,6 +321,7 @@ ifeq ($(uname_S),Windows)
>         NO_PREAD = YesPlease
>         NEEDS_CRYPTO_WITH_SSL = YesPlease
>         NO_LIBGEN_H = YesPlease
> +       NO_INTTYPES_H = UnfortunatelyYes
>         NO_POLL = YesPlease
>         NO_SYMLINK_HEAD = YesPlease
>         NO_IPV6 = YesPlease
> @@ -502,7 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>         NO_INET_NTOP = YesPlease
>         NO_POSIX_GOODIES = UnfortunatelyYes
>         DEFAULT_HELP_FORMAT = html
> -       COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
> +       COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
>         COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
>         COMPAT_OBJS += compat/mingw.o compat/winansi.o \
>                 compat/win32/pthread.o compat/win32/syslog.o \
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f6d3a46..aa57a04 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -85,6 +85,12 @@
>  #define _NETBSD_SOURCE 1
>  #define _SGI_SOURCE 1
>
> +#ifndef NO_INTTYPES_H
> +#include <inttypes.h>
> +#else
> +#include <stdint.h>
> +#endif
> +

Just checking that I understand: Does this mean that we now require an
MSVC-version that has stdint.h? If so, I'm not against such a case.
IMO, the biggest benefit of using MSVC is not building on legacy
systems, but being able to use it's debugger. And for that purpose
it's probably OK to increase the required version.

Re: [PATCH 02/12] MINGW: compat/bswap.h: include stdint.h

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 4:52 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> On Mon, Apr 28, 2014 at 04:45:43PM +0200, Erik Faye-Lund wrote:
>> bswap.h is included after stdint.h from git-compat-util.h anyway...
>
> That only becomes true after PATCH 05 when talking about MinGW.
>
> Will drop this one.

Right, thanks.

Re: [PATCH 09/12] MINGW: config.mak.uname: drop -DNOGDI

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> On MinGW-W64, MsgWaitForMultipleObjects is guarded with #ifndef NOGDI.
>
> Unfortunately, including wingdi.h brings in #define ERROR 0 which
> conflicts with several Git internal enums. So, #undef ERROR.
>
> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---
>  config.mak.uname  | 4 ++--
>  git-compat-util.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index a376b32..4883fd5 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -363,7 +363,7 @@ ifeq ($(uname_S),Windows)
>         COMPAT_OBJS = compat/msvc.o compat/winansi.o \
>                 compat/win32/pthread.o compat/win32/syslog.o \
>                 compat/win32/dirent.o
> -       COMPAT_CFLAGS = -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
> +       COMPAT_CFLAGS = -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
>         BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
>         EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
>         PTHREAD_LIBS =
> @@ -503,7 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>         NO_INET_NTOP = YesPlease
>         NO_POSIX_GOODIES = UnfortunatelyYes
>         DEFAULT_HELP_FORMAT = html
> -       COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
> +       COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -Icompat -Icompat/win32
>         COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
>         COMPAT_OBJS += compat/mingw.o compat/winansi.o \
>                 compat/win32/pthread.o compat/win32/syslog.o \
> diff --git a/git-compat-util.h b/git-compat-util.h
> index aa57a04..48bf0f7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -98,6 +98,8 @@
>  #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
>  #include <winsock2.h>
>  #include <windows.h>
> +/* wingdi.h defines ERROR=0, undef it to avoid conflicts */
> +#undef ERROR
>  #define GIT_WINDOWS_NATIVE
>  #endif

Perhaps it would be cleaner to just undef NOGDI in compat/poll/poll.c?
That way we don't end up pulling in all kinds of unrelated GUI-stuff
into places where they're not needed...?

Re: [PATCH 07/12] MINGW: config.mak.uname: reorganize MINGW settings

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> HAVE_LIBCHARSET_H and NO_R_TO_GCC_LINKER are not specific to
> msysGit, they're general MinGW settings.

Actually, HAVE_LIBCHARSET_H is. It's only present because we have
libiconv installed.

Re: [PATCH 05/12] MINGW: git-compat-util.h: use inttypes.h for printf macros.

[Marat Radchenko]

On Mon, Apr 28, 2014 at 04:53:52PM +0200, Erik Faye-Lund wrote:
> Just checking that I understand: Does this mean that we now require an
> MSVC-version that has stdint.h? If so, I'm not against such a case.
> IMO, the biggest benefit of using MSVC is not building on legacy
> systems, but being able to use it's debugger. And for that purpose
> it's probably OK to increase the required version.

Ouch, that was not intentional. What minimal MSVC version is currently
supported and who decides if it is OK to increase required one?

Re: [PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> mingw-w64 has lseek defined in io.h.

msysGit has a declaration of it in io.h as well. But it's not a
preprocessor-definition... Are you saying that it's a
preprocessor-define in mingw-w64, that points to a 64-bit version? If
so, looks good.

Re: [PATCH 07/12] MINGW: config.mak.uname: reorganize MINGW settings

[Marat Radchenko]

On Mon, Apr 28, 2014 at 04:58:11PM +0200, Erik Faye-Lund wrote:
> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> > HAVE_LIBCHARSET_H and NO_R_TO_GCC_LINKER are not specific to
> > msysGit, they're general MinGW settings.
> 
> Actually, HAVE_LIBCHARSET_H is. It's only present because we have
> libiconv installed.

1. What are other ways to provide iconv on MinGW?
2. One can still completely disable iconv with NO_ICONV=1

Re: [PATCH 05/12] MINGW: git-compat-util.h: use inttypes.h for printf macros.

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 5:00 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> On Mon, Apr 28, 2014 at 04:53:52PM +0200, Erik Faye-Lund wrote:
>> Just checking that I understand: Does this mean that we now require an
>> MSVC-version that has stdint.h? If so, I'm not against such a case.
>> IMO, the biggest benefit of using MSVC is not building on legacy
>> systems, but being able to use it's debugger. And for that purpose
>> it's probably OK to increase the required version.
>
> Ouch, that was not intentional. What minimal MSVC version is currently
> supported and who decides if it is OK to increase required one?

I don't know what the oldest one anyone have ever used, but I don't
think that matters. IMO, just noting it in the commit-message is good
enough. Others might feel differently, though.

stdint.h is available in VS10 and onwards.
contrib/buildsystems/Generators/Vcproj.pm generates project files for
VS9.

Re: [PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Marat Radchenko]

On Mon, Apr 28, 2014 at 05:02:09PM +0200, Erik Faye-Lund wrote:
> msysGit has a declaration of it in io.h as well. But it's not a
> preprocessor-definition... Are you saying that it's a
> preprocessor-define in mingw-w64, that points to a 64-bit version? If
> so, looks good.

MinGW is x86 only.

MinGW-W64, a separate project, provides both x86 and x86_64.

And here's relevant part of io.h from MingW-W64:
http://sourceforge.net/apps/trac/mingw-w64/browser/trunk/mingw-w64-headers/crt/io.h?rev=5437#L321

Re: [PATCH 07/12] MINGW: config.mak.uname: reorganize MINGW settings

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 5:04 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> On Mon, Apr 28, 2014 at 04:58:11PM +0200, Erik Faye-Lund wrote:
>> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
>> > HAVE_LIBCHARSET_H and NO_R_TO_GCC_LINKER are not specific to
>> > msysGit, they're general MinGW settings.
>>
>> Actually, HAVE_LIBCHARSET_H is. It's only present because we have
>> libiconv installed.
>
> 1. What are other ways to provide iconv on MinGW?

I'm not sure I understand. To set HAVE_LIBCHARSET_H, we need to have
libcharset.h. MinGW doesn't supply by default to my knowledge, so we
get it from iconv. The THIS_IS_MSYSGIT file is there for us to be able
to pick the right defaults for msysGit, and us having libcharset is
indeed a msysGit-detail. Not all iconv-flavors supply libcharset.h, so
this tells a particularity about the one we have in msysGit.

> 2. One can still completely disable iconv with NO_ICONV=1

Sure. And it does seem like the current setup assumes that anyone
building for MinGW has iconv. But perhaps that's a mistake?

All in all, I think maybe the line between MinGW and msysGit is a bit
blurry at the moment. On the other hand, I don't know of anyone else
than Sebastian that builds outside of msysGit.

To be honest, I think the whole THIS_IS_MSYSGIT-block should have
stayed downstream.

Re: [PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 5:09 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> On Mon, Apr 28, 2014 at 05:02:09PM +0200, Erik Faye-Lund wrote:
>> msysGit has a declaration of it in io.h as well. But it's not a
>> preprocessor-definition... Are you saying that it's a
>> preprocessor-define in mingw-w64, that points to a 64-bit version? If
>> so, looks good.
>
> MinGW is x86 only.
>
> MinGW-W64, a separate project, provides both x86 and x86_64.
>
> And here's relevant part of io.h from MingW-W64:
> http://sourceforge.net/apps/trac/mingw-w64/browser/trunk/mingw-w64-headers/crt/io.h?rev=5437#L321

It looks like Line 335 is the real goodie:
https://sourceforge.net/apps/trac/mingw-w64/browser/trunk/mingw-w64-headers/crt/io.h?rev=5437#L335

#define lseek lseek64

So yeah, looks good to me.

Re: [PATCH 12/12] MINGW: compat/mingw.h: drop fork() definition

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> fork() is not used in MinGW builds but causes a compiler warning
> on x86_64 MinGW-W64: conflicting types for built-in function 'fork'
>
> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---
>  compat/mingw.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 8850109..2fbc8ea 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -89,8 +89,6 @@ static inline int symlink(const char *oldpath, const char *newpath)
>  { errno = ENOSYS; return -1; }
>  static inline int fchmod(int fildes, mode_t mode)
>  { errno = ENOSYS; return -1; }
> -static inline pid_t fork(void)
> -{ errno = ENOSYS; return -1; }
>  static inline unsigned int alarm(unsigned int seconds)
>  { return 0; }
>  static inline int fsync(int fd)

I've been using a similar patch for a while, so:

Acked-by: Erik Faye-Lund <kusmabite@gmail.com>

Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> nedalloc was initially added in f0ed82 to fix slowness of standard WinXP
> memory allocator. Since WinXP is EOLed, this point is no longer valid.
>
> The actual reason behind this commit is incompatibility of malloc.c.h
> with MinGW-W64 headers. Alternative solution implies updating nedalloc
> to something newer.

Did you measure that malloc on newer Windows-versions are actually
faster? AFAIK, malloc does a lot more inside the CRT than in the
kernel...

Re: [PATCH 11/12] MINGW: do not fail at redefining pid_t on MinGW-W64

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> pid_t is available in sys/types.h on both MinGW and MinGW-W64
>
> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---
>  compat/mingw.h | 1 -
>  compat/msvc.h  | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index c502a22..8850109 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -5,7 +5,6 @@
>   * things that are not available in header files
>   */
>
> -typedef int pid_t;
>  typedef int uid_t;
>  typedef int socklen_t;
>  #define hstrerror strerror
> diff --git a/compat/msvc.h b/compat/msvc.h
> index cb41ce3..e2fda48 100644
> --- a/compat/msvc.h
> +++ b/compat/msvc.h
> @@ -18,6 +18,8 @@
>  #define PRIuMAX "I64u"
>  #define PRId64 "I64d"
>
> +typedef int pid_t;
> +
>  static __inline int strcasecmp (const char *s1, const char *s2)
>  {
>         int size1 = strlen(s1);


Looks good!

Acked-by: Erik Faye-Lund <kusmabite@gmail.com>

Re: [PATCH 08/12] MINGW: config.mak.uname allow using CURL for non-msysGit builds

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> Also, fix `warning: passing argument 2 of 'mingw_main' from
> incompatible pointer type` in http-fetch.c and remote-curl.c.

These seems completely unrelated, perhaps it should be split in two?

Re: [PATCH 06/12] MSVC: config.mak.uname: drop -D__USE_MINGW_ACCESS from compile definitions

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> -D__USE_MINGW_ACCESS only affects MinGW and does nothing when
> MSVC is used.

Seems reasonable in itself.

But, doesn't this mean that our access are currently broken on MSVC?
The comment about __USE_MINGW_ACCESS is:

/*  Old versions of MSVCRT access() just ignored X_OK, while the version
    shipped with Vista, returns an error code.  This will restore the
    old behaviour  */

This sounds like we should lift the access-fix up one abstraction, into Git.

But wait a minute. In Git for Windows, we already wrapped up access
for unicode-support (03a102ff - "Win32: Unicode file name support
(except dirent)"), doing the exact same thing already. This patch
isn't upstreamed yet, though.

Sounds like there's some cleaning up left to do on our behalf :)

This clean-up makes sense regardless, though.

Re: [RFC/PATCH v1] Towards MinGW(-W64) cross-compilation

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> This patch series fixes building on modern MinGW and (32bit only yet) MinGW-W64.
>
> *Compilation* tested on:
>  - MSVC (via WinGit environment)
>  - msysGit environment
>  - Linux cross-toolchain i686-pc-mingw32 (4.8.2) with mingw-runtime-3.20.2
>  - Linux cross-toolchain i686-w64-mingw32 (4.8.2) with mingw64-runtime-3.1.0
>
> Stuff still required to make Git build with x86_64 MinGW-W64 toolchain:
>
> 1. Drop -D_USE_32BIT_TIME_T that was added in fa93bb to config.mak.uname
> because time_t cannot be 32bit on x86_64. I haven't yet figured out what
> should break if this define is removed (pointers are welcome) and why it was
> added in the first place.
>
> 2. Stop passing --large-address-aware to linker. I wonder if it does anything
> for 32bit MinGW builds.
>
> 3. Fix several places with mismatched pointer size casts.
>
> Building it from Gentoo Linux:
>
> MinGW:
>
>   crossdev -t i686-pc-mingw32
>   ARCH=x86 emerge-i686-pc-mingw32 -u dev-libs/libiconv sys-libs/zlib net-misc/curl sys-devel/gettext expat
>   cd <git>
>   make CROSS_COMPILE=i686-pc-mingw32- CC=i686-pc-mingw32-gcc NO_OPENSSL=1 MINGW=1 CURLDIR=/usr/i686-pc-mingw32/usr
>
> MinGW-W64 (32 bit):
>
>   crossdev -t i686-w64-mingw32
>   ARCH=x86 emerge-i686-w64-mingw32 -u dev-libs/libiconv sys-libs/zlib net-misc/curl sys-devel/gettext expat
>   cd <git>
>   make CROSS_COMPILE=i686-w64-mingw32- CC=i686-w64-mingw32-gcc NO_OPENSSL=1 MINGW=1 CURLDIR=/usr/i686-w64-mingw32/usr
>
> Debian/Ubuntu build instructions are WIP (xdeb is non-trivial at all).

Apart from some minor nits, this looks good to me. Thanks a lot for
spending the time, and I look forward to a second iteration!

Re: [PATCH 01/12] MINGW: config.mak.uname: add explicit way to request MinGW-build

[Jonathan Nieder]

Hi,

Marat Radchenko wrote:

> When crosscompiling, one cannot rely on `uname` from host system.
> Thus, add an option to use `make MINGW=1` for building MinGW build
> from non-MinGW host (Linux, for example).

The same also applies when cross-compiling for any other platform, no?
The consistent thing to do would be to add an ifdef block like this
for every other platform,version,cpuarch triplet we care about, which
doesn't seem like a great change.

Can't the caller pass in uname_S=MINGW uname_M=x86_64 uname_O=MINGW
uname_R= uname_P= uname_V= (or a single uname_A variable that the
makefile could decompose if that is more convenient)?

Of course that is fussy.  It would be better to infer everything from
CC, since the caller has to specify CC anyway.  Does the output from
"$(CC) -dumpmachine" have the information you need?

Curious,
Jonathan

Re: [PATCH 08/12] MINGW: config.mak.uname allow using CURL for non-msysGit builds

[Marat Radchenko]

On Mon, Apr 28, 2014 at 05:26:38PM +0200, Erik Faye-Lund wrote:
> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> > Also, fix `warning: passing argument 2 of 'mingw_main' from
> > incompatible pointer type` in http-fetch.c and remote-curl.c.
> 
> These seems completely unrelated, perhaps it should be split in two?

Okay, will split. Though there is a connection - until this patch,
http-fetch.c and remote-curl.c never built for MinGW.

Re: [PATCH 08/12] MINGW: config.mak.uname allow using CURL for non-msysGit builds

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 6:23 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> On Mon, Apr 28, 2014 at 05:26:38PM +0200, Erik Faye-Lund wrote:
>> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
>> > Also, fix `warning: passing argument 2 of 'mingw_main' from
>> > incompatible pointer type` in http-fetch.c and remote-curl.c.
>>
>> These seems completely unrelated, perhaps it should be split in two?
>
> Okay, will split. Though there is a connection - until this patch,
> http-fetch.c and remote-curl.c never built for MinGW.

Ahh, thanks for pointing that out. But yeah, I think splitting is
still the right option.

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Jonathan Nieder]

Hi,

Marat Radchenko wrote:

> +# Define CROSS_COMPILE to specify the prefix used for all executables used
> +# during compilation. Only gcc and related bin-utils executables
> +# are prefixed with $(CROSS_COMPILE).

Please include an example.

	# Define CROSS_COMPILE=foo- if your compiler and binary utilities
	# are foo-cc, foo-ar, foo-strip, etc.  More specific variables
	# override this, so if you set CC=gcc CROSS_COMPILE=ia64-linux-gnu-
	# then the compiler will be 'gcc', not 'ia64-linux-gnu-gcc'.

Otherwise unless I happen to know the convention from other packages I
would not know whether to include a trailing '-' in CROSS_COMPILE,
etc.

Does the effect of this setting depend on whether CC=gcc (i.e., is the
Makefile checking the value of CC and ignoring CROSS_COMPILE when it
is e.g. the Intel compiler)?

[...]
> -STRIP ?= strip
> +STRIP = $(CROSS_COMPILE)strip

Before, STRIP from the environment took precedence over STRIP from the
makefile.  Switching to the more usual 'environment can't be trusted'
convention is a good change, but please mention it in the commit
message.

The rest looks good from a quick look.

Thanks,
Jonathan

Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Jonathan Nieder]

Erik Faye-Lund wrote:
> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:

>> nedalloc was initially added in f0ed82 to fix slowness of standard WinXP
>> memory allocator. Since WinXP is EOLed, this point is no longer valid.
>>
>> The actual reason behind this commit is incompatibility of malloc.c.h
>> with MinGW-W64 headers. Alternative solution implies updating nedalloc
>> to something newer.
>
> Did you measure that malloc on newer Windows-versions are actually
> faster? AFAIK, malloc does a lot more inside the CRT than in the
> kernel...

If it does turn out that nedalloc is not needed any more, please
remove it from compat/, too. ;-)

Thanks for cleaning up.

Jonathan

Re: [PATCH 07/12] MINGW: config.mak.uname: reorganize MINGW settings

[Marat Radchenko]

On Mon, Apr 28, 2014 at 05:17:25PM +0200, Erik Faye-Lund wrote:
> > 1. What are other ways to provide iconv on MinGW?
> 
> I'm not sure I understand. To set HAVE_LIBCHARSET_H, we need to have
> libcharset.h. MinGW doesn't supply by default to my knowledge, so we
> get it from iconv. The THIS_IS_MSYSGIT file is there for us to be able
> to pick the right defaults for msysGit, and us having libcharset is
> indeed a msysGit-detail. Not all iconv-flavors supply libcharset.h, so
> this tells a particularity about the one we have in msysGit.

> > 2. One can still completely disable iconv with NO_ICONV=1
> 
> Sure. And it does seem like the current setup assumes that anyone
> building for MinGW has iconv. But perhaps that's a mistake?

This patch assumes that "if user has iconv under MinGW, he has
libcharset.h". Without it, we assume "if user has iconv under MinGW,
he has langinfo.h". If user doesn't have iconv, he needs to say this
via NO_ICONV=1 in both cases. Anyway, if it is a questionable change,
I'll drop it and only keep NO_R_TO_GCC_LINKER change.

> To be honest, I think the whole THIS_IS_MSYSGIT-block should have
> stayed downstream.

That's a completely different story.

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Felipe Contreras]

Jonathan Nieder wrote:
> Marat Radchenko wrote:

> Does the effect of this setting depend on whether CC=gcc (i.e., is the
> Makefile checking the value of CC and ignoring CROSS_COMPILE when it
> is e.g. the Intel compiler)?

If the user is doing 'make CC=gcc', the CROSS_COMPILE setting shouldn't be
appended.

> [...]
> > -STRIP ?= strip
> > +STRIP = $(CROSS_COMPILE)strip
> 
> Before, STRIP from the environment took precedence over STRIP from the
> makefile.  Switching to the more usual 'environment can't be trusted'
> convention is a good change, but please mention it in the commit
> message.

That's only if you run make without -e. If somebody wants the environment to be
trusted, they should run 'make -e' in my opinion.

-- 
Felipe Contreras

RE: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Felipe Contreras]

Marat Radchenko wrote:

> diff --git a/Makefile b/Makefile
> index 74a929b..24befc2 100644
> --- a/Makefile
> +++ b/Makefile

> @@ -401,8 +404,11 @@ htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
>  
>  export prefix bindir sharedir sysconfdir gitwebdir localedir
>  
> -CC = cc
> -AR = ar
> +AR = $(CROSS_COMPILE)ar
> +CC = $(CROSS_COMPILE)cc
> +GCOV = $(CROSS_COMPILE)gcov
> +STRIP = $(CROSS_COMPILE)strip

Nice.

> diff --git a/config.mak.uname b/config.mak.uname
> index 5d301da..6c2e6df 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -511,7 +511,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>  	EXTLIBS += -lws2_32
>  	GITLIBS += git.res
>  	PTHREAD_LIBS =
> -	RC = windres -O coff
> +	RC = $(CROSS_COMPILE)windres -O coff

I don't think this is the best.

We should probably have this in the Makefile:

  RC = $(CROSS_COMPILE)windres

And then config.mak.uname should have

  RCFLAGS += -O coff

>  	NATIVE_CRLF = YesPlease
>  	X = .exe
>  	SPARSE_FLAGS = -Wno-one-bit-signed-bitfield

-- 
Felipe Contreras

RE: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Felipe Contreras]

Marat Radchenko wrote:
> nedalloc was initially added in f0ed82 to fix slowness of standard WinXP
> memory allocator. Since WinXP is EOLed, this point is no longer valid.

The fact that WinXP is EOLed doesn't mean people are not using it any more =/

I think it should be supported by Git at least for a while.

-- 
Felipe Contreras

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Felipe Contreras]

Marat Radchenko wrote:
> On Mon, Apr 28, 2014 at 12:37:42PM -0500, Felipe Contreras wrote:
> > > +CC = $(CROSS_COMPILE)cc
> > 
> > Nice.
> 
> Actually, not. You still have to override CC because it is
> $(CROSS_COMPILE)*g*cc. Any thoughts how to handle this?

Tell mingw to fix this? My distribution (Arch Linux) provides a link
x86_64-w64-mingw32-cc -> x86_64-w64-mingw32-gcc.

-- 
Felipe Contreras

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Marat Radchenko]

On Mon, Apr 28, 2014 at 12:37:42PM -0500, Felipe Contreras wrote:
> > +CC = $(CROSS_COMPILE)cc
> 
> Nice.

Actually, not. You still have to override CC because it is
$(CROSS_COMPILE)*g*cc. Any thoughts how to handle this?

> > -	RC = windres -O coff
> > +	RC = $(CROSS_COMPILE)windres -O coff
> 
> I don't think this is the best.
> 
> We should probably have this in the Makefile:
> 
>   RC = $(CROSS_COMPILE)windres
> 
> And then config.mak.uname should have
> 
>   RCFLAGS += -O coff

Okay, will do in v2.

Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Philip Oakley]

From: "Felipe Contreras" <felipe.contreras@gmail.com>
> Marat Radchenko wrote:
>> nedalloc was initially added in f0ed82 to fix slowness of standard 
>> WinXP
>> memory allocator. Since WinXP is EOLed, this point is no longer 
>> valid.
>
> The fact that WinXP is EOLed doesn't mean people are not using it any 
> more =/
>
> I think it should be supported by Git at least for a while.
>
> -- 
> Felipe Contreras
> --
My family and I have a number of XP machines [chooses frying pan vs 
fire..]

I'd expect a lot of basic users who are more difficult to support to 
still be on XP for a while.
At least until the next big zero day weakness openss up ;-)

Philip 

Re: [PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Felipe Contreras]

Torsten Bögershausen wrote:
> On 2014-04-28 15.51, Marat Radchenko wrote:
> > mingw-w64 has lseek defined in io.h.
> []
> >  #define off_t off64_t
> > +#ifndef lseek
> >  #define lseek _lseeki64
> > +#endif
> Is the commit message in line with the code?
> 
> I would have expected something in this style:
> 
> #if defined(__x86_64__) && ! defined(lseek))
> #include <io.h>
> #endif 

mingw-w64 provides a 32-bit compiler as well, and it needs this fix as well
IIRC.

-- 
Felipe Contreras

Re: [PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Torsten Bögershausen]

On 2014-04-28 15.51, Marat Radchenko wrote:
> mingw-w64 has lseek defined in io.h.
[]
>  #define off_t off64_t
> +#ifndef lseek
>  #define lseek _lseeki64
> +#endif
Is the commit message in line with the code?

I would have expected something in this style:

#if defined(__x86_64__) && ! defined(lseek))
#include <io.h>
#endif 

Re: [PATCH 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64

[Erik Faye-Lund]

On Mon, Apr 28, 2014 at 9:59 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 2014-04-28 15.51, Marat Radchenko wrote:
>> mingw-w64 has lseek defined in io.h.
> []
>>  #define off_t off64_t
>> +#ifndef lseek
>>  #define lseek _lseeki64
>> +#endif
> Is the commit message in line with the code?
>
> I would have expected something in this style:
>
> #if defined(__x86_64__) && ! defined(lseek))
> #include <io.h>
> #endif

No, we want 64-bit off_t either way, to support large files. So we
need the version that takes a 64-bit argument.

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Marat Radchenko]

On Mon, Apr 28, 2014 at 09:25:36AM -0700, Jonathan Nieder wrote:
> > -STRIP ?= strip
> > +STRIP = $(CROSS_COMPILE)strip
> 
> Before, STRIP from the environment took precedence over STRIP from the
> makefile.  Switching to the more usual 'environment can't be trusted'
> convention is a good change, but please mention it in the commit
> message.

Taken from [1]:

> Simply expanded variables are defined by lines using ‘:=’ or ‘::=’ (see Setting Variables).
> Both forms are equivalent in GNU make; however only the ‘::=’ form is described by the POSIX
> standard (support for ‘::=’ was added to the POSIX standard in 2012, so older versions of make
> won't accept this form either).
>
> The value of a simply expanded variable is scanned once and for all, expanding any references
> to other variables and functions, when the variable is defined. The actual value of the simply
> expanded variable is the result of expanding the text that you write. It does not contain any
> references to other variables; it contains their values as of the time this variable was defined.
> Therefore,
>
>    x := foo
>    y := $(x) bar
>    x := later
> is equivalent to
>
>    y := foo bar
>    x := later
>
> When a simply expanded variable is referenced, its value is substituted verbatim.

I don't see how it relates to environment precedence. Could you please provide me an example of
a situation that changed due to my commit?

[1]: http://www.gnu.org/software/make/manual/make.html#Flavors

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Jonathan Nieder]

Hi,

Marat Radchenko wrote:
> On Mon, Apr 28, 2014 at 09:25:36AM -0700, Jonathan Nieder wrote:

>>> -STRIP ?= strip
>>> +STRIP = $(CROSS_COMPILE)strip
>>
>> Before, STRIP from the environment took precedence over STRIP from the
>> makefile.  Switching to the more usual 'environment can't be trusted'
>> convention is a good change, but please mention it in the commit
>> message.
>
> Taken from [1]:
>
>> Simply expanded variables are defined by

I'm not really sure what in particular you're pointing to in that
page.  If you have a more specific question about what '?=' means,
could you say it?

Hope that helps,
Jonathan

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Marat Radchenko]

On Mon, Apr 28, 2014 at 01:45:04PM -0700, Jonathan Nieder wrote:
> I'm not really sure what in particular you're pointing to in that
> page.  If you have a more specific question about what '?=' means,
> could you say it?

Woops. I guess I need some sleep. Confused '?=' with ':='.

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Jonathan Nieder]

Marat Radchenko wrote:
> On Mon, Apr 28, 2014 at 12:37:42PM -0500, Felipe Contreras wrote:

>>> +CC = $(CROSS_COMPILE)cc
>>
>> Nice.
>
> Actually, not. You still have to override CC because it is
> $(CROSS_COMPILE)*g*cc. Any thoughts how to handle this?

One possibility would be something like

	ifdef CROSS_COMPILE
	CC = $(CROSS_COMPILE)gcc
	else
	CC = cc
	endif

Or as Felipe says, you can try to lobby your distro to install the
symlink.

Thanks,
Jonathan

Re: [PATCH 04/12] Makefile: introduce CROSS_COMPILE variable

[Felipe Contreras]

Jonathan Nieder wrote:
> Marat Radchenko wrote:
> > On Mon, Apr 28, 2014 at 12:37:42PM -0500, Felipe Contreras wrote:
> 
> >>> +CC = $(CROSS_COMPILE)cc
> >>
> >> Nice.
> >
> > Actually, not. You still have to override CC because it is
> > $(CROSS_COMPILE)*g*cc. Any thoughts how to handle this?
> 
> One possibility would be something like
> 
> 	ifdef CROSS_COMPILE
> 	CC = $(CROSS_COMPILE)gcc
> 	else
> 	CC = cc
> 	endif

Or just:

  CC = $(CROSS_COMPILE)gcc

Which is what the Linux kernel does.

> Or as Felipe says, you can try to lobby your distro to install the
> symlink.

He can add the symlink into his ~/bin, or any $PATH.

-- 
Felipe Contreras

Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Marat Radchenko]

On Mon, Apr 28, 2014 at 05:23:25PM +0200, Erik Faye-Lund wrote:
> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko <marat@slonopotamus.org> wrote:
> > nedalloc was initially added in f0ed82 to fix slowness of standard WinXP
> > memory allocator. Since WinXP is EOLed, this point is no longer valid.
> >
> > The actual reason behind this commit is incompatibility of malloc.c.h
> > with MinGW-W64 headers. Alternative solution implies updating nedalloc
> > to something newer.
> 
> Did you measure that malloc on newer Windows-versions are actually
> faster? AFAIK, malloc does a lot more inside the CRT than in the
> kernel...

Windows 8, msysGit.

git repack -adf on msysgit/git (best of 3 runs)

+ nedalloc: 10.5s
- nedalloc: 11s

git repack -adf on torvalds/linux (best of 3 runs)

+ nedalloc: 3m 24s
- nedalloc: 3m 47s

We need to make a decision: drop nedalloc, update nedalloc to later release,
patch nedalloc to make it work under MinGW-W64 or disable nedalloc under
MinGW-W64 (still leaving it enabled under MinGW).

P.S. Waiting for "Resolving deltas" when cloning torvalds/linux is a pain,
perhaps someone should run gprof on it.

Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Felipe Contreras]

Marat Radchenko wrote:

> We need to make a decision: drop nedalloc, update nedalloc to later release,
> patch nedalloc to make it work under MinGW-W64 or disable nedalloc under
> MinGW-W64 (still leaving it enabled under MinGW).

I say go for the latter (disable for mingw-264). It can be fixed later, and in
the meantime nobody gets affected negatively.

-- 
Felipe Contreras

Re: [PATCH 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR

[Marat Radchenko]

> Did you measure that malloc on newer Windows-versions are actually faster?

No, I didn't. As I said, real reason for this patch is that Git version of nedalloc fails to compile under MinGW-W64. If we still want to use nedalloc under MinGW, this patch should be replaced with:

 a) Updating nedalloc to newer release that succeeds to compile under MinGW-W64 (I'd prefer this option). As a bonus point, we can drop most (if not all) fixes that Git currently has on top of nedallox.

 OR

 b) Patching specific problems that prevent compilation.

 OR

 c) Introducing separate MinGW/MinGW-W64 behavior to compat.mak.uname, least preferred option.