MSVC build broken (on cygwin)

MSVC build broken (on cygwin)

[Ramsay Jones]

Hi Marius,

I know that I'm somewhat late to comment on your recent MSVC
build patches, but I was busy at the time; better late than
never... maybe ;-)

While the patches were traversing the list, I was feeling
somewhat nervous about the effect of the patches on the
cygwin build; in fact I remember thinking that they had
*probably* broken the build. But I was busy...

Well I finally found time, yesterday, to take a closer look.
I spent 10-15 minutes squinting at the code in order to
convince myself that you had in fact *not* broken the cygwin
build. :)

(which I already suspected, since they were committed some time 
ago and nobody else had screamed!)

[Note: I was mainly concerned about commit 435bdf8 and, to a
lesser degree, commit 71064e3]

I'm sure you are probably aware of the following, but for the
benefit of others, the following session on cygwin may help to
explain my nervousness:

    $ cat -n hello.c
         1	#include <stdio.h>
         2	
         3	#ifdef IW_H
         4	# include <windows.h>
         5	#endif
         6	
         7	int main(int argc, char *argv[])
         8	{
         9	
        10	#ifdef __CYGWIN__
        11		printf("__CYGWIN__\n");
        12	#endif
        13	#ifdef __MINGW32__
        14		printf("__MINGW32__\n");
        15	#endif
        16	#ifdef _WIN32
        17		printf("_WIN32\n");
        18	#endif
        19	#ifdef WIN32
        20		printf("WIN32\n");
        21	#endif
        22		printf("Hello world\n");
        23		return 0;
        24	}
        25	
    $ 

    $ gcc hello.c
    $ ./a.exe
    __CYGWIN__
    Hello world
    $ 

    $ gcc -DIW_H hello.c
    $ ./a.exe
    __CYGWIN__
    _WIN32
    WIN32
    Hello world
    $ 

    $ gcc -mno-cygwin hello.c
    $ ./a.exe
    __MINGW32__
    _WIN32
    WIN32
    Hello world
    $ 
[Note: I don't know if the above is exactly equivalent to an
MSYS/Mingw-gcc installation, but it does, at least, not link with
the cygwin dll]

However, while squinting at the code, I noticed what I think is a
problem with the MSVC build on cygwin. Viz:

    $ cl hello.c
    [...compiler output snipped...]
    $ ./hello.exe
    _WIN32
    Hello world
    $ 

    $ cl -DIW_H hello.c
    [...compiler output snipped...]
    $ ./hello.exe
    _WIN32
    WIN32
    Hello world
    $ 

    $ cl -DWIN32-D_CONSOLE hello.c
    [...compiler output snipped...]
    $ ./hello.exe
    _WIN32
    Hello world
    $ 

Note the last compiler command line above. As part of commit 164a5e3,
the Makefile (on line 917) sets the BASIC_CFLAGS macro to contain the
above string. I had expected the compiler to complain about this
malformed -Define (gcc does), but it remains quiet and seems to be
ignoring the parameter entirely. So I tried upping the warning level:

    $ cl -W4 -DWIN32-D_CONSOLE hello.c
    Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 15.00.30729.01 for 80x86
    Copyright (C) Microsoft Corporation.  All rights reserved.

    hello.c
    hello.c(7) : warning C4100: 'argv' : unreferenced formal parameter
    hello.c(7) : warning C4100: 'argc' : unreferenced formal parameter
    Microsoft (R) Incremental Linker Version 9.00.30729.01
    Copyright (C) Microsoft Corporation.  All rights reserved.

    /out:hello.exe 
    hello.obj 
    $ ./hello.exe
    _WIN32
    Hello world
    $ 

[Note: I also tried the above using the "Visual Studio 2008 command
prompt" with exactly the same result]

So, at least on cygwin with the version of msvc I'm using (see above),
the build should be broken; as a quick check I made the following
change (on top of commit f5c3178):

-- >8 --
diff --git a/git-compat-util.h b/git-compat-util.h
index 8d6e29c..72275a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -69,6 +69,8 @@
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
 #include <winsock2.h>
 #include <windows.h>
+#else
+#error "WIN32 *not* defined in MSVC build"
 #endif
 
 #include <unistd.h>
-- >8 --

and then tried to build using msvc, thus:

    $ make MSVC=1
    GIT_VERSION = 1.6.5.rc1.37.gf5c31.dirty
        * new build flags or prefix
        CC fast-import.o
    fast-import.c
    c:\cygwin\home\ramsay\git\git-compat-util.h(73) : fatal error C1189: #error :  "WIN32 *not* defined in MSVC build"
    [...lots of similar output (940 lines) snipped...]
    $ 

Finally, I removed the above change and applied the patch given below.
Now, I didn't expect this to work because I don't have all of the
dependencies installed, and those that I do have installed are not
where the Makefile expects them to be (eg zlib is at C:\zlib).
However, the build does at least compile all of the C source files, but
then all of the link's fail since it can't find zlib.lib.

[Note: I was a little surprised that it got that far, since I didn't
expect it to find the zlib header files. However, I have set the
INCLUDE environment variable which msvc is respecting! yeah, a bit old
fashioned!  Having also set the LIB environment variable, I was then
a bit surprised that the linker didn't find the library; until I
noticed that my library is called libz.lib *not* zlib.lib!]

Note that the patch below includes some line-wrapping which you can
ignore if you like, it just makes the Makefile easier to read.
The only change that matters is inserting a space between -DWIN32 and
-D_CONSOLE.

Anyway, the point is *not* to get the msvc build to work for me; rather
it is to understand why the build *works* for you. ;-)

ATB,
Ramsay Jones

-- >8 --
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Wed, 30 Sep 2009 20:08:41 +0100
Subject: [PATCH] Fix the MSVC build on cygwin


Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 Makefile |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 12defd4..e6ec8ed 100644
--- a/Makefile
+++ b/Makefile
@@ -914,10 +914,17 @@ ifdef MSVC
 	CC = compat/vcbuild/scripts/clink.pl
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
-	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32-D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
+	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild \
+		       -Icompat/vcbuild/include -DWIN32 -D_CONSOLE \
+		       -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS \
+		       -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o
-	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -DSTRIP_EXTENSION=\".exe\"
-	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
+	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H \
+			-DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch \
+			-Icompat/regex -Icompat/fnmatch \
+			-DSTRIP_EXTENSION=\".exe\"
+	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE \
+			-NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
 	lib =
 ifndef DEBUG
-- 
1.6.4

Re: MSVC build broken (on cygwin)

[Marius Storm-Olsen]

Ramsay Jones said the following on 01.10.2009 19:11:
> Hi Marius,
> 
> I know that I'm somewhat late to comment on your recent MSVC
> build patches, but I was busy at the time; better late than
> never... maybe ;-)
> 
> While the patches were traversing the list, I was feeling
> somewhat nervous about the effect of the patches on the
> cygwin build; in fact I remember thinking that they had
> *probably* broken the build. But I was busy...
> 
> Well I finally found time, yesterday, to take a closer look.
> I spent 10-15 minutes squinting at the code in order to
> convince myself that you had in fact *not* broken the cygwin
> build. :)
...
> [Note: I was a little surprised that it got that far, since I didn't
> expect it to find the zlib header files. However, I have set the
> INCLUDE environment variable which msvc is respecting! yeah, a bit old
> fashioned!  Having also set the LIB environment variable, I was then
> a bit surprised that the linker didn't find the library; until I
> noticed that my library is called libz.lib *not* zlib.lib!]
...
Clone the git://repo.or.cz/msvcgit.git, and run the 
setup_32bit_env.cmd script in there, and you should have everything 
you need to both compile and link Git with MSVC.

> Note that the patch below includes some line-wrapping which you can
> ignore if you like, it just makes the Makefile easier to read.
> The only change that matters is inserting a space between -DWIN32 and
> -D_CONSOLE.
> 
> Anyway, the point is *not* to get the msvc build to work for me; rather
> it is to understand why the build *works* for you. ;-)

First of all, thanks for the thorough report! :)
Second, I just recompiled, and it magically works for me. Why is a 
good question, since I also think it shouldn't at this point. The 
_WIN32 define is added by the compiler, and the WIN32 is added by 
windows.h, so our define guards *should* be testing for the _WIN32 define.

To how the guarded code reacts, I preprocessed the run-command.c with 
both version of the command line, and the result was the same:

( 9:54:49 - D:\msvc\git)
 > cl -E ... -DWIN32-D_CONSOLE ... run-command.c | grep run_thread
run-command.c
static unsigned __stdcall run_thread(void *data)
         async->tid = (HANDLE) _beginthreadex(((void *)0), 0, 
run_thread, async, 0, ((void *)0));

( 9:55:43 - D:\msvc\git)
 > cl -E ... -DWIN32 -D_CONSOLE ... run-command.c | grep run_thread
run-command.c
static unsigned __stdcall run_thread(void *data)
         async->tid = (HANDLE) _beginthreadex(((void *)0), 0, 
run_thread, async, 0, ((void *)0));

So, obviously, some magic in there is making it work for me. I have a 
hard time locating the magic in question though. :-/
That being said, does adding the space between the defines fix the 
MSVC compilation using Cygwin's GNU Make? It's none-the-less a correct 
patch, so you get an ack from me. Thanks!

Acked-by: Marius Storm-Olsen <mstormo@gmail.com>

--
.marius

Re: MSVC build broken (on cygwin)

[Alex Riesen]

MSVC (all versions) define a compiler specific _MSC_VER, if that's of any use.

Re: MSVC build broken (on cygwin)

[Marius Storm-Olsen]

Alex Riesen said the following on 02.10.2009 10:23:
> MSVC (all versions) define a compiler specific _MSC_VER, if that's of any use.

In this case it was define guards to let both MSVC and MinGW through 
:) Both use _WIN32 and WIN32, which Cygwin gcc normally doesn't, 
unless, as Ramsay said, you specify -mno-cygwin, or include windows.h 
apparently.

Maybe we should allow Cygwin to also include the LEAN_AND_MEAN 
windows.h in git-compat-util.h, and rather fix up the guards to 
cleanly differ between Cygwin and non-Cygwin on Windows?

Apparently, nothing is broken in neither Cygwin, MinGW or MSVC after 
Ramsays whitespace fix, but I'm sure it might get hairy later, if/when 
we get more Windows contributions. Keeping the guards right could get 
tricky.

So, something like this maybe, in git-compat-util.h:

#if defined(__MINGW32__) || defined(_MSC_VER)
#  defined API_WIN32
#  defined OS_WINDOWS
#elif defined(__CYGWIN__)
#  defined API_POSIX
#  defined OS_WINDOWS
#else
#  defined API_POSIX
#endif

So, then we can use #ifdef API_WIN32 when using the Win32 API is the 
only option/preferred for MinGW or MSVC; and use #ifdef OS_WINDOWS 
when there are things that affect all the Windows builds.

Opinions?

--
.marius

Re: MSVC build broken (on cygwin)

[Ramsay Jones]

Marius Storm-Olsen wrote:
> ...
> Clone the git://repo.or.cz/msvcgit.git, and run the 
> setup_32bit_env.cmd script in there, and you should have everything 
> you need to both compile and link Git with MSVC.
> 

Hmm, I'm trying to avoid YASORD (Yet Another Set Of Redundant
Dependencies ;-)  On my laptop, I currently have 4 zlib installations,
5 openssl installations, 3 iconv's ...

As I said earlier (see below), I'm not so interested in getting the
msvc build to work for me, rather than understand why it works for you,
since it should not work!

Having said that, I *may* try to get it working on my cygwin
installation. However, I'm much more likely to make some changes to
the build/Makefile to allow the dependent libraries to be installed
in different locations :) There is nothing wrong with my zlib at
C:\zlib.

>> Anyway, the point is *not* to get the msvc build to work for me; rather
>> it is to understand why the build *works* for you. ;-)
> 
> First of all, thanks for the thorough report! :)

You're welcome.

> Second, I just recompiled, and it magically works for me. Why is a 
> good question, since I also think it shouldn't at this point. The

Oh, you *really* don't want "magic" to be the answer... :P

> So, obviously, some magic in there is making it work for me. I have a 
> hard time locating the magic in question though. :-/

Which shell are you using? MSYS-bash?
Which make are you using? MSYS-GNU?
Which Perl are you using? ActiveState? MSYS?

I'm using cygwin 1.5.22, along with the cygwin versions of
bash 3.2, GNU make 3.81, perl 5.8.7

I noticed that the clink.pl script was not returning the correct exit
code to the Makefile, which is why I ended up snipping 940 lines of
output from the earlier #error demonstration; the Makefile does not
notice when the compile exits with an error.

In order to fix this issue for me, I made the following change:

-- >8 --
diff --git a/compat/vcbuild/scripts/clink.pl b/compat/vcbuild/scripts/clink.pl
index 0ffd59f..3e4e501 100644
--- a/compat/vcbuild/scripts/clink.pl
+++ b/compat/vcbuild/scripts/clink.pl
@@ -45,4 +45,18 @@ if ($is_linking) {
 	push(@args, @cflags);
 }
 #printf("**** @args\n");
-exit system(@args);
+
+system(@args);
+
+if ($? == -1) {
+	print "clink.pl: failed to execute: $!\n";
+	exit 1;
+}
+elsif ($? & 127) {
+	printf "clink.pl: child died with signal %d%s\n",
+		($? & 127), ($? & 128) ? ', coredump.' : '.';
+	exit 1;
+}
+
+exit $? >> 8;
+
-- >8 --

See "perldoc -f system" for more explanation of the above. This is
how it works on unix and unix-alike systems, so this may not work
too well on (say) ActiveState Perl; Dunno. Also, according to this
documentation, the form of the call to system() should result in a
call to an exec function, rather than using a shell; this may or
may not be true on other platforms.

Having fixed that problem, I modified clink.pl again so that it
would run args.exe rather than cl.exe; this allowed me to see,
using: make -> perl -> "system()" -> args.exe, just what will be
passed to the compiler.

Just in case you can't guess, create args.exe from:

    $ cat -n args.c
         1	#include <stdio.h>
         2	
         3	int main(int argc, char *argv[])
         4	{
         5		int i;
         6	
         7		for (i=0; i< argc; i++) {
         8			printf("argv[%d] = '%s'\n", i, argv[i]);
         9		}
        10		exit(1);
        11	}
        12	
    $ 

and put it somewhere in your path (~/bin for me).

    $ make MSVC=1
    GIT_VERSION = 1.6.5.rc1.38.gb4f27.dirty
        * new build flags or prefix
        CC fast-import.o
    argv[0] = 'args'
    argv[1] = '-Fofast-import.o'
    argv[2] = '-c'
    argv[3] = '-nologo'
    argv[4] = 'fast-import.c'
    argv[5] = '-I.'
    argv[6] = '-I../zlib'
    argv[7] = '-Icompat/vcbuild'
    argv[8] = '-Icompat/vcbuild/include'
    argv[9] = '-DWIN32-D_CONSOLE'
    [...snipped...]
    argv[56] = '-Icompat/regex'
    make: *** [fast-import.o] Error 1

Perhaps you could try a similar exercise?

Hmm, do you have any funny environment variables set which msvc is
picking up? 
Oh, what about the CL variable?

> That being said, does adding the space between the defines fix the 
> MSVC compilation using Cygwin's GNU Make? It's none-the-less a correct 
> patch, so you get an ack from me. Thanks!
> 
> Acked-by: Marius Storm-Olsen <mstormo@gmail.com>
> 

Thanks!

ATB,
Ramsay Jones

Re: MSVC build broken (on cygwin)

[Ramsay Jones]

Marius Storm-Olsen wrote:
> Apparently, nothing is broken in neither Cygwin, MinGW or MSVC after 
> Ramsays whitespace fix, but I'm sure it might get hairy later, if/when 
> we get more Windows contributions. Keeping the guards right could get 
> tricky.
> 

Right! Thus my earlier nervousness. :P

> So, something like this maybe, in git-compat-util.h:
> 
> #if defined(__MINGW32__) || defined(_MSC_VER)
> #  defined API_WIN32
> #  defined OS_WINDOWS
> #elif defined(__CYGWIN__)
> #  defined API_POSIX
> #  defined OS_WINDOWS
> #else
> #  defined API_POSIX
> #endif
> 

This is a much better idea.

Note that I also have Digital-Mars C/C++ 8.50, Open Watcom C/C++ 1.8
and lcc 4.2 installed. So, lets add to our previous tests:

Digital-Mars:

    $ dmc hello.c
    link hello,,,user32+kernel32/noi;

    $ ./hello.exe
    _WIN32
    WIN32
    Hello world
    $ 

    $ dmc -DIW_H hello.c
    link hello,,,user32+kernel32/noi;

    $ ./hello.exe
    _WIN32
    WIN32
    Hello world
    $ 

    $ dmc -DWIN32-D_CONSOLE hello.c
    Command line error: bad -D switch, need '=' after macro name--- errorlevel 1
    $ 

Open Watcom:

    $ wcl386 hello.c
    [...compiler output snipped...]
    $ ./hello.exe
    _WIN32
    Hello world
    $ 

    $ wcl386 -DIW_H hello.c
    [...compiler output snipped...]
    $ ./hello.exe
    _WIN32
    Hello world
    $ 

    $ wcl386 -DWIN32-D_CONSOLE hello.c
    Open Watcom C/C++32 Compile and Link Utility Version 1.8
    Portions Copyright (c) 1988-2002 Sybase, Inc. All Rights Reserved.
    Source code is available under the Sybase Open Watcom Public License.
    See http://www.openwatcom.org/ for details.
           wcc386 hello.c  -DWIN32 -D_CONSOLE
    Open Watcom C32 Optimizing Compiler Version 1.8
    Portions Copyright (c) 1984-2002 Sybase, Inc. All Rights Reserved.
    Source code is available under the Sybase Open Watcom Public License.
    See http://www.openwatcom.org/ for details.
    hello.c: 25 lines, included 757, 0 warnings, 0 errors
    Code size: 52
           wlink @__wcl__.lnk
    Open Watcom Linker Version 1.8
    Portions Copyright (c) 1985-2002 Sybase, Inc. All Rights Reserved.
    Source code is available under the Sybase Open Watcom Public License.
    See http://www.openwatcom.org/ for details.
    loading object files
    searching libraries
    creating a Windows NT character-mode executable
    $ ./hello.exe
    _WIN32
    WIN32
    Hello world
    $ 
[Note: I didn't snip the compiler output here so that you could see that
the Watcom driver program had "fixed" the malformed -Define and passed
it as two separate parameters to the compiler proper!]

Also note that Open Watcom is currently being ported to Linux, I *think*
Digital-Mars already has a Linux version and lcc does have a Linux
version. However, I think it's reasonably safe to assume we won't see a
Linux version of msvc.

So, I think something like this in git-compat-util.h:

#if defined(_WIN32) && !defined(__CYGWIN__)
# define WIN32_API
# define WIN32_LEAN_AND_MEAN
# include <winsock2.h>
# include <windows.h>
#endif

and replace all #if(n)def WIN32|_WIN32 with #if(n)def WIN32_API.

The only use of the <windows.h> header by cygwin can be moved
into compat/cygwin.c. (I don't much like cygwin using the
Win32 API anyway!)

> So, then we can use #ifdef API_WIN32 when using the Win32 API is the 
> only option/preferred for MinGW or MSVC; and use #ifdef OS_WINDOWS 
> when there are things that affect all the Windows builds.
> 
> Opinions?

see above. I don't think OS_WINDOWS is necessary.

Anyway, *something* like this would be an improvement.

ATB,
Ramsay Jones

Re: MSVC build broken (on cygwin)

[Marius Storm-Olsen]

Ramsay Jones said the following on 03.10.2009 22:06:
> Marius Storm-Olsen wrote:
>> So, something like this maybe, in git-compat-util.h:
>>
>> #if defined(__MINGW32__) || defined(_MSC_VER)
>> #  defined API_WIN32
>> #  defined OS_WINDOWS
>> #elif defined(__CYGWIN__)
>> #  defined API_POSIX
>> #  defined OS_WINDOWS
>> #else
>> #  defined API_POSIX
>> #endif
> 
> This is a much better idea.

OK, I'll write up a patch, tomorrow or Monday.

...
> So, I think something like this in git-compat-util.h:
> 
> #if defined(_WIN32) && !defined(__CYGWIN__)
> # define WIN32_API
> # define WIN32_LEAN_AND_MEAN
> # include <winsock2.h>
> # include <windows.h>
> #endif

I agree with this one. Send a patch, and I'll ack.


> and replace all #if(n)def WIN32|_WIN32 with #if(n)def WIN32_API.

Ok, I might look into that too then.


> The only use of the <windows.h> header by cygwin can be moved
> into compat/cygwin.c. (I don't much like cygwin using the
> Win32 API anyway!)

I don't have Cygwin installed, so I won't touch this one.


>> So, then we can use #ifdef API_WIN32 when using the Win32 API is the 
>> only option/preferred for MinGW or MSVC; and use #ifdef OS_WINDOWS 
>> when there are things that affect all the Windows builds.
>>
>> Opinions?
> 
> see above. I don't think OS_WINDOWS is necessary.

Well, it was mostly intended where we'd have code/algorithms which are 
platform specific, and not really compiler specific; such as the *stat() 
optimizations. They could probably be joined into an OS_WINDOWS section, 
with a POSIX_API hunk for the Cygwin fallbacks.

Not really important though. Hopefully there won't be too much platform 
specific stuff anyways.

--
.marius