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

[Timothy Brown]

On Sat, Mar 30, 2013 at 10:57:31AM +1000, Peter Johansson wrote:
> Hi Timothy,
> 
> Here are just my thoughts. Please ignore if you find them stupid.

Hi, Definitely not. I appreciate all comments. I am new to writing
these macros.

> 
> ># ===========================================================================
> >#      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.

Ok, my mistake I'll change it to ACTION-iF*.

> 
> >#
> ># 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.
> 

True, I didn't think of all situations. I'll change the last attempt
to default to what ld.so.conf has as a search path.

> >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.

No, it isn't needed. I'll remove it. My misunderstanding of it's use.

> >    AC_MSG_RESULT(no)
> s/no/[no]/
> >  fi],
> >  [AC_MSG_RESULT(yes)])
> >
> s/yes/[yes]/
> >         if test "$ac_cv_libbam" = "yes" -a "$ac_cv_sam_h" = "yes" ; then
> -a is not portable, prefer 'test cond1 && test cond2'
> 
> >                 AC_MSG_CHECKING(samtools)
> quoting
> >                 AC_MSG_RESULT(ok)
> quoting
> >                 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

Thanks. I'll update all these errors. All of your comments are valid.
I will also update the ax_lib_tabix macro I submitted at the same time
to reflect the changes pointed out here.

Once again, thanks.

Timothy