You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by makl <ab...@gmx.net> on 2004/05/01 10:05:14 UTC
[PATCH] Compile APR (HEAD) with MinGW
The build was tested with the following components:
MSYS 1.0.10
MSYS Developer Tool Kit 1.0.1
+ update msys-autoconf 2.59
+ update msys-automake 1.8.2
+ update msys-libtool 1.5
MinGW 3.1.0-1
+ update MinGW Runtime 3.2
+ update Windows API 2.5
libtool needs a 'file'-command to work correctly. You can get one from
http://gnuwin32.sourceforge.net
----------------------------------------------------------------------
[[[
* Makefile.in
- Add 'include/arc' to the include path to avoid changes to many
source files.
- Overwrite 'apr.h' with 'apr.hw' during install.
* build.conf
Add some extra files for the Windows build.
* configure.in
- Ensure that the name of the MingW DLL is compatible to the MSVC
build.
- Set some values needed for the build.
- Skip some not needed tests that fail with MinGW.
* build/gen-build.py
- Add a win32 target.
- Add platform independent processing for the extra files.
- Ensure that only forward slashes are written to build-outputs.mk.
* include/apr.h.in
Include apr.hw on win32.
* include/apr.hw
- Make some needed adjustments for MinGW.
- Fix the definition of IN6_IS_ADDR_V4MAPPED.
* include/arch/win32/apr_arch_atime.h
- Fix the inline definition of FileTimeToAprTime and
AprTimeToFileTime.
- Add prototypes for FileTimeToAprTime and AprTimeToFileTime to avoid
compiler warnings.
* include/arch/win32/apr_arch_file_io.h
apr_wchar_t is already defined in apr_arch_utf8.h.
* include/arch/win32/apr_arch_misc.h
- Avoid a compiler warning.
- Add APR_DECLARE_LATE_DLL_FUNC for MinGW.
* include/arch/win32/apr_private.h
Don't define __attribute__ for MinGW.
* misc/win32/apr_app.c
* misc/win32/internal.c
Commented out due to issues with _malloc_dbg.
* misc/win32/misc.c
Don't include crtdbg.h.
* misc/win32/rand.c
Include rpc.h
* misc/win32/start.c
- Don't include crtdbg.h.
- Commented out warrsztoastr and parts of apr_app_initialize due to
issues with _malloc_dbg.
* test/Makefile.in
- Add two programs to CLEAN_TARGETS
- Add the shell wrappers to CLEAN_TARGETS
- Add $(LOCAL_LIBS) to the build of mod_test.la and libmod_test.la.
- Add @EXEEXT@ to testall
- Add some preprocessing before running the tests.
* test/testdso.c
Define MOD_NAME for MinGW
* test/testmmap.c
Use the MinGW version of mmap_datafile
* test/testpipe.c
Check the return code of apr_file_close for both calls.
]]]
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by makl <ab...@gmx.net>.
William A. Rowe, Jr. wrote:
> At 03:05 AM 5/1/2004, makl wrote:
>
> That's the wrong solution - I already explained on an earlier note.
> Use some sort of #ifdef exclusion instead.
Ok. After some research I understand your remark. For release builds
_malloc_dbg and _realloc_dbg are mapped to their non debug counterparts.
I'm happy to port these functions too.
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by makl <ab...@gmx.net>.
William A. Rowe, Jr. wrote:
> At 03:05 AM 5/1/2004, makl wrote:
>
>>* misc/win32/internal.c
>> Commented out due to issues with _malloc_dbg.
>
>
> That's the wrong solution - I already explained on an earlier note.
> Use some sort of #ifdef exclusion instead.
I haven't tried it yet. But if the documentation in the files is correct
it will not work. Both compilers use the same runtime.
>>+++ build.conf 1 May 2004 06:28:17 -0000
>>+# Additional files for the win32 build
>>+win32_extra =
>>+ file_io/unix/copy.lo
>>+ file_io/unix/fileacc.lo
>>+ file_io/unix/filepath_util.lo
>>+ file_io/unix/fullrw.lo
>>+ file_io/unix/mktemp.lo
>>+ file_io/unix/tempdir.lo
>
> Problem - you've now wrapped the ntos kernel with the posix api, inside
> of apr. This has caused, and will cause security issues that are pretty
> well documented on bugtraq. You don't handle win32 filename reserved
> characters or case insensitivity at all.
> I'd be much happier seeing the win32 api, and while we are at it, perhaps
> we pass the POSIX flag to the win32 CreateFile and other api's to ensure
> the filename handling is case sensitive? (I'm guessing you want the
> strict filename handling of unix?)
I use the same files as the Windows build (look at libapr.dsp). If there
are any problems you should try to fix them there. :)
> As an academic exercise your proposed patch is great - but we need to
> evaluate if wraping apr -> mingw -> posix -> win32 isn't going to create
> more security/bug/interop issues than it solves.
MinGW produce a native WIN32 DLL with the same runtime as VC++ 6.0. So
there should be the same problems as with the MSVC build. No more and
not less.
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 03:05 AM 5/1/2004, makl wrote:
>The build was tested with the following components:
Thank you for the toolkit links, assembling that toolchain for testing...
>* misc/win32/internal.c
> Commented out due to issues with _malloc_dbg.
That's the wrong solution - I already explained on an earlier note.
Use some sort of #ifdef exclusion instead.
>* misc/win32/start.c
> - Don't include crtdbg.h.
> - Commented out warrsztoastr and parts of apr_app_initialize due to
> issues with _malloc_dbg.
Ditto :)
>+++ build.conf 1 May 2004 06:28:17 -0000
>+# Additional files for the win32 build
>+win32_extra =
>+ file_io/unix/copy.lo
>+ file_io/unix/fileacc.lo
>+ file_io/unix/filepath_util.lo
>+ file_io/unix/fullrw.lo
>+ file_io/unix/mktemp.lo
>+ file_io/unix/tempdir.lo
Problem - you've now wrapped the ntos kernel with the posix api, inside
of apr. This has caused, and will cause security issues that are pretty
well documented on bugtraq. You don't handle win32 filename reserved
characters or case insensitivity at all.
I'd be much happier seeing the win32 api, and while we are at it, perhaps
we pass the POSIX flag to the win32 CreateFile and other api's to ensure
the filename handling is case sensitive? (I'm guessing you want the
strict filename handling of unix?)
As an academic exercise your proposed patch is great - but we need to
evaluate if wraping apr -> mingw -> posix -> win32 isn't going to create
more security/bug/interop issues than it solves.
And as I mentioned - thank you for the toolchain links, I'll grab those and
see how far I can get w/ your proposal. Thanks!
Bill
Re: [PATCH] Compile APR with MinGW : configure.in whithout whitespace
changes
Posted by makl <ab...@gmx.net>.
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 14:16:08 -0000
@@ -179,7 +179,15 @@
if test "x$use_libtool" = "xyes"; then
lt_compile='$(LIBTOOL) $(LTFLAGS) --mode=compile $(COMPILE) -o $@ -c $< && touch $@'
+ dnl Ensure that the name of the MingW DLL is compatible to the MSVC build.
+ case $host in
+ *mingw32*)
+ LT_VERSION="-avoid-version"
+ ;;
+ *)
LT_VERSION="-version-info `$get_version libtool $version_hdr APR`"
+ ;;
+ esac
link="\$(LIBTOOL) \$(LTFLAGS) --mode=link \$(LT_LDFLAGS) \$(COMPILE) ${LT_VERSION} \$(ALL_LDFLAGS) -o \$@"
so_ext='lo'
lib_target='-rpath $(libdir) $(OBJECTS)'
@@ -414,6 +422,17 @@
OSDIR="unix"
eolstr="\\n"
;;
+ *mingw32*)
+ OSDIR="win32"
+ OBJECTS_PLATFORM='$(OBJECTS_win32)'
+ APR_ADDTO(CFLAGS,-mthreads)
+ APR_ADDTO(LDFLAGS,-no-undefined)
+ APR_ADDTO(LIBS, -lwsock32)
+ APR_ADDTO(LIBS, -lws2_32)
+ APR_ADDTO(LIBS, -lrpcrt4)
+ enable_threads="system_threads"
+ eolstr="\\r\\n"
+ ;;
*)
OSDIR="unix"
eolstr="\\n"
@@ -696,6 +715,11 @@
haveshmgetanon="0"
havemmapzero="0"
havemmapanon="0"
+
+case $host in
+ *mingw32*)
+ ;;
+ *)
APR_BEGIN_DECISION([anonymous shared memory allocation method])
APR_IFALLYES(header:sys/ipc.h header:sys/shm.h header:sys/file.h dnl
func:shmget func:shmat func:shmdt func:shmctl,
@@ -732,6 +756,8 @@
esac
APR_END_DECISION
AC_DEFINE_UNQUOTED($ac_decision)
+;;
+esac
useshmgetanon="0"
usemmapzero="0"
@@ -762,6 +788,10 @@
haveshmget="0"
havebeosarea="0"
haveos2shm="0"
+case $host in
+ *mingw32*)
+ ;;
+ *)
APR_BEGIN_DECISION([namebased memory allocation method])
APR_IFALLYES(header:sys/mman.h func:mmap func:munmap,
[havemmaptmp="1"
@@ -796,6 +826,8 @@
esac
APR_END_DECISION
AC_DEFINE_UNQUOTED($ac_decision)
+;;
+esac
usemmaptmp="0"
usemmapshm="0"
@@ -1387,7 +1419,7 @@
# Everything else:
if test "$dsotype" = "any"; then
case $host in
- *os390|*-os2*|*os400|*-aix*) dsotype=other ;;
+ *os390|*-os2*|*os400|*-aix*|*mingw32*) dsotype=other ;;
esac
fi
fi
@@ -1585,6 +1617,10 @@
# At this stage, we match the ordering in Apache 1.3
# which is (highest to lowest): pthread -> posixsem -> sysvsem -> fcntl -> flock
#
+case $host in
+ *mingw32*)
+ ;;
+ *)
APR_BEGIN_DECISION([apr_lock implementation method])
APR_IFALLYES(func:flock define:LOCK_EX,
APR_DECIDE(USE_FLOCK_SERIALIZE, [4.2BSD-style flock()]))
@@ -1605,6 +1641,8 @@
fi
APR_END_DECISION
AC_DEFINE_UNQUOTED($ac_decision)
+;;
+esac
flockser="0"
sysvser="0"
@@ -1973,6 +2011,7 @@
esac
AC_SUBST(INCLUDE_RULES)
AC_SUBST(INCLUDE_OUTPUTS)
+AC_SUBST(enable_shared)
SAVE_FILES="include/apr.h include/arch/unix/apr_private.h"
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by Branko Čibej <br...@xbc.nu>.
rbb@rkbloom.net wrote:
>On Tue, 4 May 2004, [UTF-8] Branko Čibej wrote:
>
>
>>rbb@rkbloom.net wrote:
>>
>>
>>>We need to solve that problem. Having apr.h include apr.hw isn't the
>>>right solution. I need to think about what a valid solution is, and it
>>>may be having a separate directory that the Windows build systems create
>>>to store this header file, and we include that directory in the INCLUDES
>>>path before the standard includes dir. That would also solve the problem
>>>of doing a CVS diff after having built on Windows. (Namely, the cvs diff
>>>returns a mess of hits in apr.h, IIRC, because apr.hw is copied over
>>>apr.h. Netware would use the same system).
>>>
>>>
>>Maybe I'm dense...what's apr.h doing in CVS? It shoudln't be there,
>>being a generated file.
>>
>>
>
>Nope, I'm the dense one. The CVS argument is bogus, but I was paying
>attention to my daughter while replying to you.
>
>
Heh, you weren't replying to me, either. :-)
Anyway, I agree: apr.h.in has no business including apr.hw. The mingw
build process has to convert apr.hw to apr.h, just like the "regular"
Windows build process. Otherwise everything goes to pot.
-- Brane
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by rb...@rkbloom.net.
On Tue, 4 May 2004, [UTF-8] Branko Čibej wrote:
> rbb@rkbloom.net wrote:
>
> >On Sat, 1 May 2004, makl wrote:
> >
> >
> >> 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.
> >>
> >>
> >
> >We need to solve that problem. Having apr.h include apr.hw isn't the
> >right solution. I need to think about what a valid solution is, and it
> >may be having a separate directory that the Windows build systems create
> >to store this header file, and we include that directory in the INCLUDES
> >path before the standard includes dir. That would also solve the problem
> >of doing a CVS diff after having built on Windows. (Namely, the cvs diff
> >returns a mess of hits in apr.h, IIRC, because apr.hw is copied over
> >apr.h. Netware would use the same system).
> >
> >
> Maybe I'm dense...what's apr.h doing in CVS? It shoudln't be there,
> being a generated file.
Nope, I'm the dense one. The CVS argument is bogus, but I was paying
attention to my daughter while replying to you.
Sorry about that.
Ryan
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by Branko Čibej <br...@xbc.nu>.
rbb@rkbloom.net wrote:
>On Sat, 1 May 2004, makl wrote:
>
>
>> 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.
>>
>>
>
>We need to solve that problem. Having apr.h include apr.hw isn't the
>right solution. I need to think about what a valid solution is, and it
>may be having a separate directory that the Windows build systems create
>to store this header file, and we include that directory in the INCLUDES
>path before the standard includes dir. That would also solve the problem
>of doing a CVS diff after having built on Windows. (Namely, the cvs diff
>returns a mess of hits in apr.h, IIRC, because apr.hw is copied over
>apr.h. Netware would use the same system).
>
>
Maybe I'm dense...what's apr.h doing in CVS? It shoudln't be there,
being a generated file.
-- Brane
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by makl <ab...@gmx.net>.
rbb@rkbloom.net wrote:
>
> On Sat, 1 May 2004, makl wrote:
> Change the source files and the Windows build system to work the way that
> APR works on every other system.
No problem. I will do that.
>>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.
>
> We need to solve that problem. Having apr.h include apr.hw isn't the
> right solution. I need to think about what a valid solution is, and it
> may be having a separate directory that the Windows build systems create
> to store this header file, and we include that directory in the INCLUDES
> path before the standard includes dir. That would also solve the problem
> of doing a CVS diff after having built on Windows. (Namely, the cvs diff
> returns a mess of hits in apr.h, IIRC, because apr.hw is copied over
> apr.h. Netware would use the same system).
Different directories for the platform specific apr.h files would be a
clean solution which can be implemented easily.
>>libtool generates two wrappers. One with .exe extension and one without
>>extension. Thus both are needed in the CLEAN_TARGETS.
>
> We need a cleaner way to solve this. No other platform is creating two
> executables when the build system only specifies one. If libtool in
> mingw32 is doing that, then it is broken and the fix should be in libtool,
> not every build system that uses that version of libtool.
This is the way libtool work and it's not a bug. Having a shell and a
binary wrapper is intended behaviour. See the following ChangeLog entry
from libtool.
2003-01-28 Charles Wilson <cw...@ece.gatech.edu>
* ltmain.in: add code for a binary wrapper
to use with uninstalled executables on cygwin/mingw.
Make sure that --mode=clean gets shell wrapper and
binary wrapper. When sourcing the shell wrapper,
invoke using a terminal `.' on cygwin/mingw to
avoid the automatic append-.exe behavior.
> The call should be removed IMHO. The tarballs aren't an issue, because
> Windows developers should be downloading the zip file, which _should_ have
> all files converted before packaging.
Ok. I will remove the call. By the way, where can I find zip
distributions? I have never seen them.
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by rb...@rkbloom.net.
On Sat, 1 May 2004, makl wrote:
> 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.
Change the source files and the Windows build system to work the way that
APR works on every other system.
>
> >>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.
We need to solve that problem. Having apr.h include apr.hw isn't the
right solution. I need to think about what a valid solution is, and it
may be having a separate directory that the Windows build systems create
to store this header file, and we include that directory in the INCLUDES
path before the standard includes dir. That would also solve the problem
of doing a CVS diff after having built on Windows. (Namely, the cvs diff
returns a mess of hits in apr.h, IIRC, because apr.hw is copied over
apr.h. Netware would use the same system).
> >>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.
We need a cleaner way to solve this. No other platform is creating two
executables when the build system only specifies one. If libtool in
mingw32 is doing that, then it is broken and the fix should be in libtool,
not every build system that uses that version of libtool.
> >> 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.
The call should be removed IMHO. The tarballs aren't an issue, because
Windows developers should be downloading the zip file, which _should_ have
all files converted before packaging. As for the CVS that ships with MSYS
using LF as native, that is broken. You are on a Windows box, which means
that CRLF is native, and CVS should be smart enough to know that.
However, it isn't critical that it be removed. What is critical is that
the data/map_datafile.txt remain as the name of the file. If you have to
convert the file to dos line endings, then we can do that, but you
shouldn't change the file name. There are very few instances of the test
code using #ifdef with a platform type, and all of the ones that are there
are bugs waiting to be solved, we shouldn't add one.
> >>--- 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
Thanks. I'll commit some of the patch today, but some parts will wait
until somebody more familiar with the new build system can review them.
In general, it is a good patch, and I'm glad APR has a new platform. :-)
Ryan
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by makl <ab...@gmx.net>.
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
Re: [PATCH] Compile APR (HEAD) with MinGW
Posted by rb...@rkbloom.net.
Please post patches in-line, it makes it much easier to reply to them with
comments.
> 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.
> 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.
> 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.
> 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. 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.
> 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.
CVS should take care of making sure that your files have dos line endings
after a checkout. If you have to run a conversion script, then your CVS
is broken. This shouldn't be fixed by a Makefile. APR always uses the
native line-endings, so if your CVS is setup properly, this will just
work.
> Index: test/testmmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/test/testmmap.c,v
> retrieving revision 1.44
> diff -u -3 -r1.44 testmmap.c
> --- test/testmmap.c 13 Feb 2004 09:38:34 -0000 1.44
> +++ test/testmmap.c 1 May 2004 06:28:31 -0000
> @@ -56,7 +56,11 @@
> CuAssertTrue(tc, file1[strlen(file1) - 1] != '/');
>
> oldfileptr = file1;
> +#ifdef __MINGW32__
> + file1 = apr_pstrcat(p, file1,"/data/mmap_datafile.mingw" ,NULL);
> +#else
> file1 = apr_pstrcat(p, file1,"/data/mmap_datafile.txt" ,NULL);
> +#endif
This is resolved by not running unix2dos on the file. You can't have a
separate file for each platform, that completely breaks the test.
> --- 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. This is a deficiency in
the test framework (which I am currently re-writing). Basically, if the
first CuAssertIntEquals fails, the second one will never be called,
because the current test framework longjmp's out of the function. This
means that some of the cleanup isn't even attempted, which in the case of
testpipe could be bade, because readp and writep are shared between tests.
This will get resolved as a part of converting to my new test framework.
Ryan