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 13:00:26 GMT

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


Mime
View raw message