Re: Proposed Build System Patch.

On Mon, Sep 03, 2001 at 02:11:55PM -0400, Kevin Pilch-Bisson wrote:
> Since I got such an overwhelming reponse the last time I posted this patch
> for consideration, I thought I would repost it.

Sorry 'bout that...

>...
> --- ./SVN/text-base/Makefile.in Fri Aug 31 09:50:56 2001
> +++ ./Makefile.in Fri Aug 31 13:54:51 2001
> @@ -21,8 +21,11 @@
> prefix = @prefix@
> exec_prefix = @exec_prefix@
> libdir = @libdir@
> +fs_libdir= @libdir@
> sbindir = @sbindir@
> +fs_sbindir=@sbindir@

We can toss the sbin stuff.

> bindir = @bindir@
> +fs_bindir=@bindir@
> includedir = @includedir@
>
> ### should search for these...
> @@ -50,6 +53,11 @@
> APACHE_TARGET = @APACHE_TARGET@
> INSTALL_APACHE_RULE = @INSTALL_APACHE_RULE@
>
> +INSTALL_RULES = @INSTALL_RULES@

While we're at this, let's just toss the $(INSTALL_APACHE_RULE) and use
@INSTALL_RULES@. The only reason for the former was that we didn't have the
latter :-)

> +TEST_DEPS = $(NON_FS_TEST_DEPS) @FS_TEST_DEPS@
> +TEST_PROGRAMS = $(NON_FS_TEST_PROGRAMS) @FS_TEST_PROGRAMS@

We can minimize change by using:

check: $(TEST_DEPS) @FS_TEST_DEPS@

And similarly for the use TEST_PROGRAMS. This way, we create TEST_DEPS from
the test_deps variable, and FS_TEST_DEPS from the fs_test_deps variable.
Nice and clean.

>...
> INSTALL_SBIN = $(LIBTOOL) --mode=install $(INSTALL)
> +INSTALL_FS_SBIN = $(INSTALL_SBIN)

sbin can go

>...
> -local-all: libs programs @FS_RULES@ @BUILD_APACHE_RULE@
> +local-all: @BUILD_RULES@ @BUILD_APACHE_RULE@

And simplify by using only @BUILD_RULES@ here.

>...
> -local-install: install-lib install-include install-bin $(INSTALL_APACHE_RULE)
> +local-install: $(INSTALL_RULES) $(INSTALL_APACHE_RULE)

Let's simplify and juse use @INSTALL_RULES@ right here, rather than creating
a variable for it.

>...
> +### temporary hack. Neon does not have an "extraclean" and neither does db
> +### If we don't have extraclean -- do the next best thing.
> external-extraclean:
> @list='$(EXTERNAL_PROJECT_DIRS)'; \
> for i in $$list; do \
> - if test "$$i" != "neon"; then \
> - echo "------ making extraclean in $$i"; \
> - (cd $$i && $(MAKE) extraclean) || exit 1; \
> - echo "------ completed extraclean in $$i"; \
> + if test "$$i" != "neon" && test "$$i" != "db/dist"; then \
> + echo "------ making extraclean in $$i"; \
> + (cd $$i && $(MAKE) extraclean) || exit 1; \
> + echo "------ completed extraclean in $$i"; \
> + else \
> + echo "------ making distclean(no extraclean) in $$i"; \
> + (cd $$i && $(MAKE) distclean) || exit 1; \
> + echo "------ completed distclean(no extraclean) in $$i"; \

Indentation and maybe using positive logic for the if test? That is:

if test "$$i" = "neon" || test "$$i" = "db/dist"; then

>...
> - elif area != 'test':
> + elif area != 'test' and area != 'fs-test':
> ofile.write('install-%s: %s\n'
> '\t$(MKDIR) $(%sdir)\n'
> - % (area, string.join(files), area))
> + % (area, string.join(files), string.replace(area, '-', '_')))
> for file in files:
> ofile.write('\t$(INSTALL_%s) %s %s\n'
> - % (string.upper(area), file,
> - os.path.join('$(%sdir)' % area,
> - os.path.basename(file))))
> + % (string.replace(string.upper(area), '-', '_'), file,
> + string.replace(os.path.join('$(%sdir)' % area,
> + os.path.basename(file)), '-', '_')))
> ofile.write('\n')

Maybe introduce: area_var = string.replace(area, '-', '_'), then use that
variable name as appropriate (concat'ing, upper casing, etc)

> @@ -235,8 +239,15 @@
> scripts, s_errors = _collect_paths(parser.get('test-scripts', 'paths'))
> errors = errors or s_errors
>
> - ofile.write('TEST_DEPS = %s\n\n' % string.join(test_deps + scripts))
> - ofile.write('TEST_PROGRAMS = %s\n\n' % string.join(test_progs + scripts))
> + fs_scripts, fs_errors = _collect_paths(parser.get('fs-test-scripts', 'paths'))
> + errors = errors or fs_errors
> +
> + ofile.write('FS_TEST_DEPS = %s\n\n' % string.join(fs_test_deps + fs_scripts))
> + ofile.write('FS_TEST_PROGRAMS = %s\n\n' %
> + string.join(fs_test_progs + fs_scripts))
> + ofile.write('NON_FS_TEST_DEPS = %s\n\n' % string.join(test_deps + scripts))
> + ofile.write('NON_FS_TEST_PROGRAMS = %s\n\n' %
> + string.join(test_progs + scripts))

Per my previous comment, we could keep the TEST_* variables, and just add
the FS_TEST_* stuff.

>...
> --- ac-helpers/SVN/text-base/berkeley-db.m4 Fri Aug 31 09:53:18 2001
> +++ ac-helpers/berkeley-db.m4 Sat Sep 1 08:26:39 2001
> @@ -95,8 +95,10 @@
> if test "$status" = "builtin"; then
> # Use the include and lib files in the build dir.
> dbdir=`cd db/dist ; pwd`
> - CPPFLAGS="$CPPFLAGS -I$dbdir"
> - LIBS="$LIBS -L$dbdir -ldb"
> + SVN_DB_INCLUDES="-I$dbdir"
> + # Note that once we upgrade to libtool 1.4 this should be changed to
> + # SVN_DB_LIBS="$dbdir/libdb-3.3.la"
> + SVN_DB_LIBS="-L$dbdir/.libs -ldb-3.3"

Ah. Good. That .m4 file shouldn't have modified CPPFLAGS and LIBS like that.

Looks good to me! Nice work.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org