autoconf-archive-maintainers | |
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [patch #7983] Submission of ax_lib_samtools.m4
From: |
Timothy Brown |
Subject: |
Re: [patch #7983] Submission of ax_lib_samtools.m4 |
Date: |
Sat, 30 Mar 2013 07:50:55 -0600 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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