Re: [patch #7983] Submission of ax_lib_samtools.m4

[Peter Johansson]

Hi Timothy,

Here are just my thoughts. Please ignore if you find them stupid.

> # ===========================================================================
> #      http://www.gnu.org/software/autoconf-archive/ax_lib_samtools.html
> # ===========================================================================
> #
> # SYNOPSIS
> #
> #   AX_LIB_SAMTOOLS([ACTION-IF-TRUE], [ACTION-IF-FALSE])

This documentation suggests that first argument is a shell code that 
will be executed if test is successful; likewise that second argument is 
shell code that is executed if test fails. On the contrary, if test 
fails the macro will error out (AC_MSG_ERROR). I think you should 
ACTION-iF* above or change the code so it matches the expectation.

> #
> # DESCRIPTION
> #
> #   This macro searches for an installed samtools library. If nothing was
> #   specified when calling configure, it searches first in /usr/local and
> #   then in /usr. If the --with-samtools=DIR is specified, it will try to
> #   find it in DIR/include/bam/sam.h and DIR/lib/libbam.a. As a final try it
> #   will look in DIR/sam.h and DIR/libbam.a as the samtools library does not
> #   contain an install rule.

What if I have libbam installed in /usr/lib64/? My linker is configured 
to find libraries in that directory, but the macro will not find it, 
which is a bit confusing.


> #
> #   If --without-samtools is specified, the library is not searched at all.
> #
> #   If either the header file (sam.h) or the library (libbam) is not found,
> #   the configuration exits on error, asking for a valid samtools
> #   installation directory or --without-samtools.
> #
> #   The macro defines the symbol HAVE_SAMTOOLS if the library is found. You
> #   should use autoheader to include a definition for this symbol in a
> #   config.h file. Sample usage in a C/C++ source is as follows:
> #
> #     #ifdef HAVE_SAMTOOLS
> #     #include<sam.h>
> #     #endif /* HAVE_SAMTOOLS */
> #
> #   The following output variables are set with AC_SUBST:
> #
> #     SAMTOOLS_CPPFLAGS
> #     SAMTOOLS_LDFLAGS
> #     SAMTOOLS_LIBS
> #
> #   You can use them like this in Makefile.am:
> #
> #     AM_CPPFLAGS = $(SAMTOOLS_CPPFLAGS)
> #     AM_LDFLAGS = $(SAMTOOLS_LDFLAGS)
> #     program_LDADD = $(SAMTOOLS_LIBS)
> #
> # LICENSE
> #
> #   Copyright (c) 2013 Timothy Brown<address@hidden>
> #
> #   This program is free software; you can redistribute it and/or modify it
> #   under the terms of the GNU General Public License as published by the
> #   Free Software Foundation; either version 3 of the License, or (at your
> #   option) any later version.
> #
> #   This program is distributed in the hope that it will be useful, but
> #   WITHOUT ANY WARRANTY; without even the implied warranty of
> #   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
> #   Public License for more details.
> #
> #   You should have received a copy of the GNU General Public License along
> #   with this program. If not, see<http://www.gnu.org/licenses/>.
> #
> #   As a special exception, the respective Autoconf Macro's copyright owner
> #   gives unlimited permission to copy, distribute and modify the configure
> #   scripts that are the output of Autoconf when processing the Macro. You
> #   need not follow the terms of the GNU General Public License when using
> #   or distributing such scripts, even though portions of the text of the
> #   Macro appear in them. The GNU General Public License (GPL) does govern
> #   all other use of the material that constitutes the Autoconf Macro.
> #
> #   This special exception to the GPL applies to versions of the Autoconf
> #   Macro released by the Autoconf Archive. When you make and distribute a
> #   modified version of the Autoconf Macro, you may extend this special
> #   exception to the GPL to apply to your modified version as well.
>
> #serial 1
>
> AU_ALIAS([AC_LIB_SAMTOOLS], [AX_LIB_SAMTOOLS])

Is this really needed? AU_ALIASes are typically used when a macro has 
existed for a while and then changes name. As a this is a new macro the 
alias seems superfluous.

> AC_DEFUN([AX_LIB_SAMTOOLS],
> #
> # Handle user hints
> #
> [AC_MSG_CHECKING(if samtools is wanted)
> AC_ARG_WITH([samtools],
>    AS_HELP_STRING([--with-samtools],
>                   [search for samtools in DIR/include and DIR/lib]),
>   [if test "$withval" != no ; then
>     AC_MSG_RESULT(yes)
It's a good habit to use proper quoting, especially as many people look 
in the archives to learn writing macros, so s/yes/[yes]/
>     if test -d "$withval" ; then
>       SAMTOOLS_HOME="$withval"
>     else
>       AC_MSG_WARN([Sorry, $withval does not exist, checking usual places])
>     fi
>   else
>     AC_MSG_RESULT(no)
s/no/[no]/
>   fi],
>   [AC_MSG_RESULT(yes)])
s/yes/[yes]/
> if test -f "${SAMTOOLS_HOME}/include/bam/sam.h" ; then
>          SAMTOOLS_INCDIR="${SAMTOOLS_HOME}/include/bam"
>          SAMTOOLS_LIBDIR="${SAMTOOLS_HOME}/lib"
> elif test -f "${SAMTOOLS_HOME}/include/sam.h" ; then
>          SAMTOOLS_INCDIR="${SAMTOOLS_HOME}/include"
>          SAMTOOLS_LIBDIR="${SAMTOOLS_HOME}/lib"
> elif test -f "${SAMTOOLS_HOME}/sam.h" ; then
>          SAMTOOLS_INCDIR="${SAMTOOLS_HOME}"
>          SAMTOOLS_LIBDIR="${SAMTOOLS_HOME}"
> elif test -f "/usr/local/include/bam/sam.h" ; then
>          SAMTOOLS_HOME="/usr/local"
>          SAMTOOLS_INCDIR="${SAMTOOLS_HOME}/include/bam"
>          SAMTOOLS_LIBDIR="${SAMTOOLS_HOME}/lib"
> else
>          SAMTOOLS_HOME="/usr"
>          SAMTOOLS_INCDIR="${SAMTOOLS_HOME}/include/bam"
>          SAMTOOLS_LIBDIR="${SAMTOOLS_HOME}/lib"
> fi
>
> #
> # Locate samtools, if wanted
> #
> if test -n "${SAMTOOLS_HOME}" ; then
>
>          SAMTOOLS_OLD_LDFLAGS=$LDFLAGS
>          SAMTOOLS_OLD_CPPFLAGS=$LDFLAGS
>          LDFLAGS="$LDFLAGS -L${SAMTOOLS_LIBDIR}"
>          CPPFLAGS="$CPPFLAGS -I${SAMTOOLS_INCDIR}"
>          AC_LANG_SAVE
>          AC_LANG_C
>          AC_CHECK_HEADER(sam.h, [ac_cv_sam_h=yes], [ac_cv_sam_h=no])
s/sam.h/[sam.h]/
>          AC_CHECK_LIB(bam, sam_open, [ac_cv_libbam=yes], [ac_cv_libbam=no])
s/bam, samopen/[bam], [sam_open]/
>          AC_LANG_RESTORE
>          if test "$ac_cv_libbam" = "yes" -a "$ac_cv_sam_h" = "yes" ; then
-a is not portable, prefer 'test cond1 && test cond2'

>                  #
>                  # If both library and header were found, use them
>                  #
>                  AC_MSG_CHECKING(samtools)
quoting
>                  AC_MSG_RESULT(ok)
quoting
>                  with_samtools=yes
>          else
>                  #
>                  # If either header or library was not found, revert and bomb
>                  #
>                  LDFLAGS="$SAMTOOLS_OLD_LDFLAGS"
>                  CPPFLAGS="$SAMTOOLS_OLD_CPPFLAGS"
>                  AC_MSG_CHECKING(samtools)
quoting
>                  AC_MSG_RESULT(failed)
quoting
>                  AC_MSG_ERROR(either specify a valid samtools installation with 
> --with-samtools=DIR or disable samtools usage with --without-samtools)
quoting
>          fi
> fi
> ])

Cheers,
Peter

--
Peter Johansson