apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@rkbloom.net>
Subject Re: [PATCH] Compile APR (HEAD) with MinGW
Date Sat, 01 May 2004 17:43:18 GMT


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


Mime
View raw message