Re: Patch for supporting VPATH builds

On Wed, Jan 24, 2001 at 02:56:38AM +0100, Branko Čibej wrote:
> Kevin Pilch-Bisson wrote:
>
> > +dnl $MKDIR is required for configuring apr and neon in a vpath build
> > +MKDIR=mkdir
> > +
>
> Are you sure this is right? APR uses mkdir.sh from apr/helpers. I think
> we should do the same.
We could, I was just looking at the GNU Makefile conventions, and it
said mkdir was one of the utilities we could expect all platforms to
have, thus we could use it directly. However, automake generates some
stuff which relies on $MKDIR, so I just set it to mkdir.
>
>
> >
> > ## Sources needed to build each library (names canonicalized)
> > -libexpat_la_SOURCES = hashtable.c xmlparse.c xmlrole.c xmltok.c
> > +libexpat_la_SOURCES = asciitab.h hashtable.c hashtable.h iasciitab.h \
> > + latin1tab.h nametab.h utf8tab.h xmldef.h xmlparse.c \
> > + xmlparse.h xmlrole.c xmlrole.h xmltok.c xmltok.h \
> > + xmltok_impl.h
> > +
> > +## This is needed because they are included into xmltok.c, but shouldn't be
> > +## put in libexpat_la_SOURCES, since it should not be compiled directly
> > +EXTRA_DIST = xmltok_impl.c xmltok_ns.c
>
> Shouldn't the header files go into EXTRA_DIST instead?

Again, automake docs say that headers can go in source, and automake
won't try to compile them, but will distribute them
>
>
> > + include/svn_types.h \
> > + include/svn_wc.h \
> > include/svn_xml.h
> > + #include/svn_parse.h \
>
> ^^^ :-)
This is commented out, because I don't have svn_parse.h in my include
directory.
>
>
> > ## Flags needed when compiling:
> > -INCLUDES = -I. -I../include -I../../apr/include -I../../expat-lite
> > +INCLUDES = -I@srcdir@ -I@top_srcdir@/subversion/include \
> > + -I@top_srcdir@/apr/include -I@top_srcdir@/expat-lite \
> > + -I$(top_builddir)/apr/include
>
> I'd rather define APR_INCLUDES, EXPAT_INCLUDES, etc., and just use those
> in the makefiles, and the same for various libraries. In that way you
> only have to make any change in one place, not in every makefile.

I think that is an excellent idea.
>
>
> > Index: subversion/libsvn_subr/io.c
> > ===================================================================
> > RCS file: /cvs/subversion/subversion/libsvn_subr/io.c,v
> > retrieving revision 1.26
> > diff -u -p -r1.26 io.c
> > --- subversion/libsvn_subr/io.c 2000/12/24 12:52:59 1.26
> > +++ subversion/libsvn_subr/io.c 2001/01/23 20:30:15
> > @@ -44,7 +44,7 @@ svn_io_check_path (const svn_string_t *p
> > apr_finfo_t finfo;
> > apr_status_t apr_err;
> >
> > - apr_err = apr_stat (&finfo, path->data, pool);
> > + apr_err = apr_stat (&finfo, path->data, APR_FINFO_NORM, pool);
>
> Oops. This is part of the stat patch, it has nothing to do with srcdir
> != builddir. Don't combine different patches in one, it'll just confuse
> people trying to review it.

See NOTE re PATCH 1 in my second email.

-- 
>~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson
kevin@pilch-bisson.net
http://www.pilch-bisson.net