[PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Ramsay Jones]

When the NO_MMAP build variable is set, which is the case by
default on cygwin, the msvc linker complains:

    error LNK2001: unresolved external symbol _getpagesize

The mingw build has a version of the getpagesize() function
defined in the libgcc.a library. In addition, this function
is replaced, if USE_WIN32_MMAP is defined, by a version more
suitable for use with the native mmap.

The msvc libraries do not define any getpagesize() function,
so we move the mingw_getpagesize() implementation from the
conditionally built win32mmap.c file to mingw.c, which will
always be included in the MSVC build.

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

This alone won't make the MSVC build work on cygwin; I have to

    $ make MSVC=1 NEEDS_LIBICONV=

otherwise the linker complains about the absence of the iconv.lib.
(I have downloaded the win32 version of the library, but I haven't
got around to installing it! see http://gnuwin32.sourceforge.net/
packages/libiconv.htm)

The real problem, of course, is that the cygwin and MSVC configuration
sections are not mutually exclusive. (see patch 3/4)

ATB,
Ramsay Jones

 compat/mingw.c     |   12 ++++++++++++
 compat/mingw.h     |    2 +-
 compat/win32mmap.c |   12 ------------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6b5b5b2..15fe33e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1000,6 +1000,18 @@ repeat:
 	return -1;
 }
 
+/*
+ * Note that this doesn't return the actual pagesize, but
+ * the allocation granularity. If future Windows specific git code
+ * needs the real getpagesize function, we need to find another solution.
+ */
+int mingw_getpagesize(void)
+{
+	SYSTEM_INFO si;
+	GetSystemInfo(&si);
+	return si.dwAllocationGranularity;
+}
+
 struct passwd *getpwuid(int uid)
 {
 	static char user_name[100];
diff --git a/compat/mingw.h b/compat/mingw.h
index 5b5258b..26c4027 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -166,7 +166,7 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
-#ifdef USE_WIN32_MMAP
+#if defined(USE_WIN32_MMAP) || defined(_MSC_VER)
 int mingw_getpagesize(void);
 #define getpagesize mingw_getpagesize
 #endif
diff --git a/compat/win32mmap.c b/compat/win32mmap.c
index 779d796..1c5a149 100644
--- a/compat/win32mmap.c
+++ b/compat/win32mmap.c
@@ -1,17 +1,5 @@
 #include "../git-compat-util.h"
 
-/*
- * Note that this doesn't return the actual pagesize, but
- * the allocation granularity. If future Windows specific git code
- * needs the real getpagesize function, we need to find another solution.
- */
-int mingw_getpagesize(void)
-{
-	SYSTEM_INFO si;
-	GetSystemInfo(&si);
-	return si.dwAllocationGranularity;
-}
-
 void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
 {
 	HANDLE hmap;
-- 
1.6.5

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Johannes Sixt]

Ramsay Jones schrieb:
> When the NO_MMAP build variable is set, which is the case by
> default on cygwin, the msvc linker complains:
> 
>     error LNK2001: unresolved external symbol _getpagesize

Make up your mind: use the cygwin configuration or use the MSVC
configuration. MSVC doesn't define NO_MMAP for a reason. Where is the problem?

I understand that you run into this error if you define NO_MMAP in your
config.mak. I don't know whether we support NO_MMAP as a knob for to tweak
the builds on all platforms. If this is the case (Junio?), then your
justification must be updated.

-- Hannes

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Ramsay Jones schrieb:
>> When the NO_MMAP build variable is set, which is the case by
>> default on cygwin, the msvc linker complains:
>> 
>>     error LNK2001: unresolved external symbol _getpagesize
>
> Make up your mind: use the cygwin configuration or use the MSVC
> configuration. MSVC doesn't define NO_MMAP for a reason. Where is the problem?
>
> I understand that you run into this error if you define NO_MMAP in your
> config.mak. I don't know whether we support NO_MMAP as a knob for to tweak
> the builds on all platforms. If this is the case (Junio?), then your
> justification must be updated.

I don't _mind_ per-se if platform people add good support for building
with both using and not using mmap(), even though I do not see a strong
need for supporting NO_MMAP configuration, on a platform with a working
good-quality mmap(), either emulation or native.

How does Cygwin-ness of the build environment affect the end result when
you build with MSVC?  I am not a Windows person, so I am only guessing,
but I suspect that the result does not pull any library nor crt0 from what
people usually consider "Cygwin environment".  It feels that the "default
configuration of Cygwin" that insists on NO_MMAP is the guilty party here.

The logic to set NO_MMAP on Cygwin obviously predates MSVC support, so it
probably was perfectly sane to say "We are compiling on Cygwin, and by
definition we use gcc to compile, link with the Cygwin libraries, and
mmap() support there is weak and we are better off with NO_MMAP from
performance (or correctness) standpoint".

If we are supporting use of MSVC as compiler toolchain on Cygwin, that
statement may no longer be true [*1*].

Shouldn't this be solved by teaching the Makefile about this new "Cygwin
but using MSVC as compiler toolchain" combination?  "Even on Cygwin, if we
are using MSVC, here are the compat layers we want to use and here are
what the underlying C library and POSIX emulation layer gives us."

So it really boils down to this question.  Is there _anything_ in the end
result that should be affected by Cygwin-ness of the build environment
when you build with MSVC?

Here are what I see in the Cygwin section:

        NO_D_TYPE_IN_DIRENT = YesPlease
        NO_D_INO_IN_DIRENT = YesPlease
        NO_STRCASESTR = YesPlease
        NO_MEMMEM = YesPlease
        NO_MKSTEMPS = YesPlease
        NO_SYMLINK_HEAD = YesPlease
        NEEDS_LIBICONV = YesPlease
        NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
        NO_TRUSTABLE_FILEMODE = UnfortunatelyYes
        OLD_ICONV = UnfortunatelyYes
        NO_MMAP = YesPlease
        NO_IPV6 = YesPlease

These all look very much like they depend on the way how C runtime library
and the POSIX emulation layer are set up.  Perhaps these should be used
only when compiling with gcc and linking with glibc on Cygwin (which is
the norm)?


[Footnote]

*1* Notice "if" at the beginning of this sentence---I am not qualified to
make a judgement without help from area experts here.  Is it a sane thing
to run Cygwin and still use MSVC as the compiler toolchain?  Is it
commonly done?  I have no idea.

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Ramsay Jones]

Junio C Hamano wrote:
> How does Cygwin-ness of the build environment affect the end result when
> you build with MSVC?

Not at all. This is an MSVC/NO_MMAP combo problem. (The "problem" also exists
with the msvc build on msysGit[1])

>                      I am not a Windows person, so I am only guessing,
> but I suspect that the result does not pull any library nor crt0 from what
> people usually consider "Cygwin environment".  It feels that the "default
> configuration of Cygwin" that insists on NO_MMAP is the guilty party here.
> 

See patch #3.

> Shouldn't this be solved by teaching the Makefile about this new "Cygwin
> but using MSVC as compiler toolchain" combination?

Yes. Err... see patch #3 :-P

> [Footnote]
> 
> *1* Notice "if" at the beginning of this sentence---I am not qualified to
> make a judgement without help from area experts here.  Is it a sane thing
> to run Cygwin and still use MSVC as the compiler toolchain?

About as sane as running msysGit and still use MSVC as the compiler! :D

>  Is it
> commonly done?  I have no idea.
> 

Nor me. I just tried it, and it works (after applying these patches!); for
exactly the same reason, and to the same extent, that it works on msysGit.

[Footnote]
*1* I admit to sometimes being a bit sloppy with naming (and maybe confused
also!). Now, IIUC, MinGW is basically gcc + binutils, MSYS is bash + some
unix tools and msysGit is MinGW + MSYS + some additional packages needed to
build and run git (eg perl). So, the "MinGW build" should really be called the
msysGit build ;-) (but the config section specifically picks out the MINGW string
from uname_S)
Also, the "msvc build on MinGW" should really be the "msvc build on msysGit".
Or something like that!

ATB,
Ramsay Jones

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Johannes Sixt]

Ramsay Jones schrieb:
> Junio C Hamano wrote:
>> Shouldn't this be solved by teaching the Makefile about this new "Cygwin
>> but using MSVC as compiler toolchain" combination?
> 
> Yes. Err... see patch #3 :-P

A clarifiction: Junio talks about using the MSVC tools to build Cygwin
programs, that is, to merely substitute Cygwin's gcc by MSVC, but to still
link against cygwin's C runtime.

But this is not what this series is about.

When the "MSVC build" of git is made, then the MSVC compiler is used, and
this will always use Microsoft libraries in the resulting executables,
regardless of whether "make MSVC=1" was called from a "cygwin environment"
or from and "msys environment".

This series is about fixing "make MSVC=1" when it is run from a "cygwin
environment" by disentangling the MSVC and Cygwin configuration sections
in the Makefile.

-- Hannes

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Ramsay Jones schrieb:
>> Junio C Hamano wrote:
>>> Shouldn't this be solved by teaching the Makefile about this new "Cygwin
>>> but using MSVC as compiler toolchain" combination?
>> 
>> Yes. Err... see patch #3 :-P
>
> A clarifiction: Junio talks about using the MSVC tools to build Cygwin
> programs, that is, to merely substitute Cygwin's gcc by MSVC, but to still
> link against cygwin's C runtime.

Actually I do not talk about that; at least I meant to.

> When the "MSVC build" of git is made, then the MSVC compiler is used, and
> this will always use Microsoft libraries in the resulting executables,
> regardless of whether "make MSVC=1" was called from a "cygwin environment"
> or from and "msys environment".

> This series is about fixing "make MSVC=1" when it is run from a "cygwin
> environment" by disentangling the MSVC and Cygwin configuration sections
> in the Makefile.

Yup.

I think we all three are in agreement that "When on Cygwin we do this"
part of the Makefile should be adjusted.  Even though I lost track of the
exact text in "patch #3", I can tell from Ramsay's response that the
series is about that.

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Ramsay Jones]

Johannes Sixt wrote:
> Ramsay Jones schrieb:
>> When the NO_MMAP build variable is set, which is the case by
>> default on cygwin, the msvc linker complains:
>>
>>     error LNK2001: unresolved external symbol _getpagesize
> 
> Make up your mind: use the cygwin configuration or use the MSVC
> configuration. MSVC doesn't define NO_MMAP for a reason. Where is the problem?

Heh, well as I said elsewhere in this email, the "real problem" is that the
MSVC and cygwin configuration sections are not mutually exclusive and that
is fixed in patch #3. So, if you apply patch #3, this "problem" disappears.

However, ...

> I understand that you run into this error if you define NO_MMAP in your
> config.mak. I don't know whether we support NO_MMAP as a knob for to tweak
> the builds on all platforms. If this is the case (Junio?), then your
> justification must be updated.

AFAICT, the only build to not support NO_MMAP is MSVC (on cygwin *or* msysGit).
The solution was obvious and low impact, so why not remove this anomaly?

(It may even prove to be a useful capability ;-)

ATB,
Ramsay Jones

Re: [PATCH 1/4] MSVC: Fix an "unresolved symbol" linker error on cygwin

[Johannes Sixt]

Ramsay Jones schrieb:
> Johannes Sixt wrote:
>> I understand that you run into this error if you define NO_MMAP in your
>> config.mak. I don't know whether we support NO_MMAP as a knob for to tweak
>> the builds on all platforms. If this is the case (Junio?), then your
>> justification must be updated.
> 
> AFAICT, the only build to not support NO_MMAP is MSVC (on cygwin *or* msysGit).
> The solution was obvious and low impact, so why not remove this anomaly?

Sure, why not? But then the justification is different, as I said.

-- Hannes