Build system nits

[GoogleCodeExporter]

IMHO, the build system could do with a little work:

* The various bits generated from and added by the autotools shouldn't be 
committed. autoreconf -i works really well these days. That's INSTALL 
Makefile.in aclocal.m4 compile config.guess config.h.in config.sub configure 
depcomp install-sh ltmain.sh missing mkinstalldirs.

configure.ac:

* Needs to call AC_SUBST([LIBTOOL_DEPS]) or else the rule to rebuild libtool in 
Makefile.am won't work.

* A lot of macro calls are underquoted. It'll probably work fine, but it's poor 
style.

* The dance with EXTRA_LIBSNAPPY_LDFLAGS seems odd. It'd be more conventional 
to do something like:

    SNAPPY_LTVERSION=snappy_version
    AC_SUBST([SNAPPY_LTVERSION])

and set the -version-info flag directly in Makefile.am. If it's to allow the 
user to provide custom LDFLAGS, it's unnecessary: LDFLAGS is part of 
libsnappy_la_LINK. Here's the snippet from Makefile.in:

    libsnappy_la_LINK = $(LIBTOOL) --tag=CXX $(AM_LIBTOOLFLAGS) \
            $(LIBTOOLFLAGS) --mode=link $(CXXLD) $(AM_CXXFLAGS) \
            $(CXXFLAGS) $(libsnappy_la_LDFLAGS) $(LDFLAGS) -o $@

* There should be an AC_ARG_WITH for gflags, because automagic dependencies 
aren't cool: http://www.gentoo.org/proj/en/qa/automagic.xml

* Shell variables starting with ac_ are in autoconf's namespace. Setting things 
like ac_have_builtin_ctz is therefore equally uncool. See 
http://www.gnu.org/s/hello/manual/autoconf/Macro-Names.html :

> To ensure that your macros don't conflict with present or future Autoconf 
macros, you should prefix your own macro names and any shell variables they use 
with some other sequence. Possibilities include your initials, or an 
abbreviation for the name of your organization or software package.

* Use AS_IF instead of directly using the shell's `if`: 
http://www.gnu.org/software/hello/manual/autoconf/Limitations-of-Builtins.html#L
imitations-of-Builtins and 
http://www.gnu.org/s/hello/manual/autoconf/Common-Shell-Constructs.html .

* Consider adding -Wall to either AUTOMAKE_OPTIONS in Makefile.am or as an 
argument to AM_INIT_AUTOMAKE. If you don't mind using a modern automake (1.11 
or later), also call AM_SILENT_RULES([yes]). Even MSYS has automake-1.11 these 
days.

Makefile.am:

* Adding $(GTEST_CPPFLAGS) to both snappy_unittest_CPPFLAGS and 
snappy_unittest_CXXFLAGS is redundant. See this part of Makefile.in:

    snappy_unittest-snappy-test.o: snappy-test.cc
    @am__fastdepCXX_TRUE@   $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(snappy_unittest_CPPFLAGS) $(CPPFLAGS) $(snappy_unittest_CXXFLAGS) $(CXXFLAGS) -MT snappy_unittest-snappy-test.o -MD -MP -MF $(DEPDIR)/snappy_unittest-snappy-test.Tpo -c -o snappy_unittest-snappy-test.o `test -f 'snappy-test.cc' || echo '$(srcdir)/'`snappy-test.cc
    @am__fastdepCXX_TRUE@   $(am__mv) $(DEPDIR)/snappy_unittest-snappy-test.Tpo $(DEPDIR)/snappy_unittest-snappy-test.Po
    @AMDEP_TRUE@@am__fastdepCXX_FALSE@      source='snappy-test.cc' object='snappy_unittest-snappy-test.o' libtool=no @AMDEPBACKSLASH@
    @AMDEP_TRUE@@am__fastdepCXX_FALSE@      DEPDIR=$(DEPDIR) $(CXXDEPMODE) $(depcomp) @AMDEPBACKSLASH@
    @am__fastdepCXX_FALSE@  $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(snappy_unittest_CPPFLAGS) $(CPPFLAGS) $(snappy_unittest_CXXFLAGS) $(CXXFLAGS) -c -o snappy_unittest-snappy-test.o `test -f 'snappy-test.cc' || echo '$(srcdir)/'`snappy-test.cc

* snappy_unittest should be in check_PROGRAMS, not noinst_PROGRAMS. That way, 
it's built as part of `make check`, not `make all`.

Original issue reported on code.google.com by endgame....@gmail.com on 22 Mar 2011 at 11:14

[GoogleCodeExporter]

Hi!

Thanks for your bug report. Do you think you can split these into multiple 
bugs? It's a lot easier keeping track of them that way (some of them are 
relatively easy, some will require some more thought due to the way we export 
our source tree).

Original comment by sgunder...@bigfoot.com on 22 Mar 2011 at 11:18

[GoogleCodeExporter]

All done. This can be closed now.

Original comment by endgame....@gmail.com on 23 Mar 2011 at 12:02

[GoogleCodeExporter]

Original comment by se...@google.com on 23 Mar 2011 at 12:19

• Changed state: Duplicate