[PATCH 2/2] cygwin: Remove the CYGWIN_V15_WIN32API build variable

[PATCH 2/2] cygwin: Remove the CYGWIN_V15_WIN32API build variable

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 Makefile         | 7 -------
 compat/cygwin.c  | 5 -----
 config.mak.uname | 1 -
 3 files changed, 13 deletions(-)

diff --git a/Makefile b/Makefile
index 0f931a2..32c49ba 100644
--- a/Makefile
+++ b/Makefile
@@ -290,10 +290,6 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
-# Define CYGWIN_V15_WIN32API if you are using Cygwin v1.7.x but are not
-# using the current w32api packages. The recommended approach, however,
-# is to update your installation if compilation errors occur.
-#
 # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
 # user.
 #
@@ -1449,9 +1445,6 @@ ifdef NO_REGEX
 	COMPAT_CFLAGS += -Icompat/regex
 	COMPAT_OBJS += compat/regex/regex.o
 endif
-ifdef CYGWIN_V15_WIN32API
-	COMPAT_CFLAGS += -DCYGWIN_V15_WIN32API
-endif
 
 ifdef USE_NED_ALLOCATOR
        COMPAT_CFLAGS += -Icompat/nedmalloc
diff --git a/compat/cygwin.c b/compat/cygwin.c
index 871b41d..91ce5d4 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -1,14 +1,9 @@
 #define CYGWIN_C
 #define WIN32_LEAN_AND_MEAN
-#ifdef CYGWIN_V15_WIN32API
-#include "../git-compat-util.h"
-#include "win32.h"
-#else
 #include <sys/stat.h>
 #include <sys/errno.h>
 #include "win32.h"
 #include "../git-compat-util.h"
-#endif
 #include "../cache.h" /* to read configuration */
 
 /*
diff --git a/config.mak.uname b/config.mak.uname
index d78fd3d..28aa239 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -158,7 +158,6 @@ ifeq ($(uname_O),Cygwin)
 		NO_SYMLINK_HEAD = YesPlease
 		NO_IPV6 = YesPlease
 		OLD_ICONV = UnfortunatelyYes
-		CYGWIN_V15_WIN32API = YesPlease
 	endif
 	NO_THREAD_SAFE_PREAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
-- 
1.8.2

Re: [PATCH 2/2] cygwin: Remove the CYGWIN_V15_WIN32API build variable

[Jonathan Nieder]

Hi,

Ramsay Jones wrote:
 
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

Let me try to understand this.

Before v1.8.1.1~7^2~2 (Update cygwin.c for new mingw-64 win32 api
headers, 2012-11-11), compat/cygwin.c did

	#define CYGWIN_C
	#define WIN32_LEAN_AND_MEAN
	#include "../git-compat-util.h"
	#include "win32.h"

Afterward, on modern Cygwin it changed to

	#define CYGWIN_C
	#define WIN32_LEAN_AND_MEAN
	#include <sys/stat.h>
	#include <sys/errno.h>
	#include "win32.h"
	#include "../git-compat-util.h"

compat/win32.h does

	#ifndef WIN32         /* Not defined by Cygwin */
	#include <windows.h>
	#endif

and then defines some inline functions relying on the win32 api.

git-compat-util.h defines various feature request macros and includes
many system headers, so that simple swap of lines was a huge change.
It's not obvious to me what part was important for making this work
with modern Cygwin win32api.  Mark or others, do you remember what
went wrong with the original "git-compat-util.h and then
compat/win32.h" order (e.g., did it break the build?  what was the
compiler message?) when upgrading win32api?

Unfortunately this was a breaking change: systems with the *old*
version of win32api would only compile with the old order of header
inclusions.  The new ordering produced the following symptom:

	In file included from compat/../git-compat-util.h:90,
			 from compat/cygwin.c:9:
	/usr/lib/gcc/i686-pc-cygwin/3.4.4/../../../../include/w32api/winsock2.h:103:2: 
	warning: #warning "fd_set and associated macros have been defined in sys/types.      
	This may cause runtime problems with W32 sockets"
	In file included from /usr/include/sys/socket.h:16,
			 from compat/../git-compat-util.h:131,
			 from compat/cygwin.c:9:
	/usr/include/cygwin/socket.h:29: error: redefinition of `struct sockaddr'
 [...]

Alex Riesen (cc-ed) noticed that removing the following lines from
git-compat-util.h alleviated this new symptom:

	#ifdef WIN32 /* Both MinGW and MSVC */
	# if defined (_MSC_VER)
	#  define _WIN32_WINNT 0x0502
	# endif
	#define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
	#include <winsock2.h>
	#include <windows.h>
	#endif

After all, on Cygwin there is no reason to include winsock or the
win32api from git-compat-util.h.  Presumably one of <sys/stat.h>,
<sys/errno.h>, and <windows.h> causes the WIN32 macro to be defined on
these systems with old win32api, and chaos ensues.

All in all, it wasn't a bad thing, since it revealed that the WIN32
macro doesn't mean what most of the git codebase assumes it means
(hence patch 1/2, which fixes that).

The reordering made in v1.8.1.1~7^2~2 still seems like voodoo to me,
but at least it works.  This patch applies that same order for
everyone.  Systems that would previously use the "I have old win32api
and don't need that reordering" codepath don't need to be
special-cased any more, since *their* particular brand of trouble is
avoided by being careful about how to use the WIN32 macro.

The upshot:

 - No change on modern setups.  To uninformed people like me I feel
   like there is still something subtle going on that is not well
   understood, but hey, this patch doesn't break it. :)

 - Tested to still work on setups that previously needed
   CYGWIN_V15_WIN32API.  Yay!

 - This drops an #ifdef, which means less code that is never tested
   to keep up to date.

With or without a few words of explanation in the commit message to
save some time for the next confused person looking this over,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/2] cygwin: Remove the CYGWIN_V15_WIN32API build variable

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The reordering made in v1.8.1.1~7^2~2 still seems like voodoo to me,
> but at least it works.  This patch applies that same order for
> everyone.  Systems that would previously use the "I have old win32api
> and don't need that reordering" codepath don't need to be
> special-cased any more, since *their* particular brand of trouble is
> avoided by being careful about how to use the WIN32 macro.
>
> The upshot:
>
>  - No change on modern setups.  To uninformed people like me I feel
>    like there is still something subtle going on that is not well
>    understood, but hey, this patch doesn't break it. :)
>
>  - Tested to still work on setups that previously needed
>    CYGWIN_V15_WIN32API.  Yay!
>
>  - This drops an #ifdef, which means less code that is never tested
>    to keep up to date.
>
> With or without a few words of explanation in the commit message to
> save some time for the next confused person looking this over,
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.  Ramsay, I tend to agree with Jonathan that this change
deserves a bit more explanation.