apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From makl <abcdefghijklmn...@gmx.net>
Subject Re: [PATCH] Compile APR (HEAD) with MinGW
Date Sat, 01 May 2004 15:27:52 GMT
rbb@rkbloom.net wrote:

>>Index: Makefile.in
>>===================================================================
>>RCS file: /home/cvspublic/apr/Makefile.in,v
>>retrieving revision 1.97
>>diff -u -3 -r1.97 Makefile.in
>>--- Makefile.in	10 Mar 2004 18:48:25 -0000	1.97
>>+++ Makefile.in	1 May 2004 06:28:17 -0000
>>@@ -17,8 +17,9 @@
>> #
>> INCDIR=./include
>> OSDIR=$(top_srcdir)/include/arch/@OSDIR@
>>+ARCHDIR=$(top_srcdir)/include/arch
>> DEFOSDIR=$(INCDIR)/arch/@DEFAULT_OSDIR@
>>-INCLUDES=-I$(INCDIR) -I$(OSDIR) -I$(DEFOSDIR) -I$(top_srcdir)/include
>>+INCLUDES=-I$(INCDIR) -I$(ARCHDIR) -I$(OSDIR) -I$(DEFOSDIR) -I$(top_srcdir)/include
> 
> 
> I don't understand why you need this.  All of the places that
> apr_private_common.h are included specify the path to the file.

Some windows source files use includes like this one:
#include "win32/apr_arch_atime.h"

And there are two solutions to that problem:

1. Add 'include/arch' to the include path (1 file changed)
2. Change the source files (13 files changed)

Tell me what solution you prefer and I will make the necessary changes.

>>Index: configure.in
>>===================================================================
>>RCS file: /home/cvspublic/apr/configure.in,v
>>retrieving revision 1.579
>>diff -u -3 -r1.579 configure.in
>>--- configure.in	16 Apr 2004 16:32:25 -0000	1.579
>>+++ configure.in	1 May 2004 06:28:21 -0000
> 
> 
> I am not even going to try to review configure.in.  You changed the format
> of the file, meaning that there is a lot of noise in the patch that makes
> it hard to review quickly.  Please re-submit the patch without the
> whitespace differences.

I will send a diff without whitespace changes with a seperate mail. But
for better legibility I recommend to use the patch with the
whitespace changes.

>>Index: include/apr.h.in
>>===================================================================
>>RCS file: /home/cvspublic/apr/include/apr.h.in,v
>>retrieving revision 1.135
>>diff -u -3 -r1.135 apr.h.in
>>--- include/apr.h.in	27 Mar 2004 13:11:17 -0000	1.135
>>+++ include/apr.h.in	1 May 2004 06:28:23 -0000
>>@@ -15,6 +15,11 @@
>>
>>
>> #ifndef APR_H
>>+
>>+#ifdef WIN32
>>+#include "apr.hw"
>>+#else
>>+
> 
> 
> Please don't do this.  The whole purpose of apr.hw is to replace apr.h for
> platforms without autoconf.  In the Windows build system, we copy apr.hw
> to apr.h as a part of the build.  Having apr.h include apr.hw for mingw32
> is not a good solution to this.  Instead, have the build system do the
> same thing that we do in the standard Windows build.

Currently I do the copy from apr.hw to apr.h at install time, since I
havn't found a good way to do that at configure time without making 
config.status useless without configure.

>>Index: test/Makefile.in
>>===================================================================
>>RCS file: /home/cvspublic/apr/test/Makefile.in,v
>>retrieving revision 1.159
>>diff -u -3 -r1.159 Makefile.in
>>--- test/Makefile.in	4 Apr 2004 12:07:46 -0000	1.159
>>+++ test/Makefile.in	1 May 2004 06:28:29 -0000
>>@@ -29,7 +29,10 @@
>> LOCAL_LIBS=../lib@APR_LIBNAME@.la
>>
>> CLEAN_TARGETS = testfile.tmp mod_test.slo proc_child@EXEEXT@ occhild@EXEEXT@ \
>>-readchild@EXEEXT@ tryread@EXEEXT@ sockchid@EXEEXT@
>>+readchild@EXEEXT@ tryread@EXEEXT@ sockchid@EXEEXT@ globalmutexchild.@EXEEXT@ \
>>+sockchild.@EXEEXT@ globalmutexchild occhild proc_child readchild sendfile \
>>+sockchild testall testlockperf testmutexscope testshmconsumer \
>>+testshmproducer tryread
> 
> 
> This is wrong.  First, for some of the programs that you added, you have a
> . between the filename and the @EXEEXT@.  @EXEEXT@ includes the . if it is
> required.
Silly typing error.

> Second, please do not include program names twice, once with an
> extension and once without.  The whole purpose of @EXEEXT@ is to do the
> proper thing with regard to extensions.
libtool generates two wrappers. One with .exe extension and one without 
extension. Thus both are needed in the CLEAN_TARGETS.

>> CLEAN_SUBDIRS = internal
>>
>> INCDIR=../include
>>@@ -39,7 +42,19 @@
>> # libtool wrapper scripts which link an executable when first run.
>> LINK_PROG = $(LIBTOOL) $(LTFLAGS) --mode=link $(LT_LDFLAGS) $(COMPILE) -no-install
$(ALL_LDFLAGS) -o $@
>>
>>+# -no-install has no effect with MingW and the generated wrapper exe doesn't
>>+# work. Thus we copy the exe files and set the path, before we let the
>>+# tests run.
>> check: $(STDTEST_PORTABLE) $(STDTEST_NONPORTABLE)
>>+	case `uname -s` in \
>>+	  *MINGW32*) \
>>+		if test @enable_shared@ = yes; then \
>>+			cp -p .libs/*.exe .; \
>>+			PATH=../.libs:$$PATH; \
>>+		fi; \
>>+		unix2dos -n data/mmap_datafile.txt data/mmap_datafile.mingw; \
>>+	    ;; \
>>+	esac; \
> 
> 
> YUCK!  I understand why this is required, but it is incredibly ugly.  As
> for running unix2dos on the datafile.txt, I don't understand this at all.

I have two reasons for the call of unix2dos:

1. As you noticed, the cvs client that ships with the 'MSYS Developer
Toolkit' uses LF as native line ending.

2. Many tar implementations for windows (including the one in MSYS) 
don't convert LF into CRLF. So you have the same problem for tarballs too.

I can remove the call but then the user has to do it manually.

>>--- test/testpipe.c	22 Mar 2004 20:51:25 -0000	1.28
>>+++ test/testpipe.c	1 May 2004 06:28:31 -0000
>>@@ -43,6 +43,7 @@
>>     char buf[256];
>>
>>     rv = apr_file_close(readp);
>>+    CuAssertIntEquals(tc, APR_SUCCESS, rv);
>>     rv = apr_file_close(writep);
>>     CuAssertIntEquals(tc, APR_SUCCESS, rv);
> 
> 
> Unfortunately, you don't really want to do that.
Removed



Mime
View raw message