Re: Please review wip/htslib

[Jason Bacon <bacon4000%gmail.com@localhost>]

On 05/15/17 14:55, Johnny C. Lam wrote:
> On Mon, May 15, 2017 at 10:48:04AM -0500, Jason Bacon wrote:
> > I believe this package should be about ready to commit, but I welcome any
> > suggestions.
> >
> > This is the first of dozens of scientific packages we hope to commit in the
> > near future, so I'd like to get as much feedback as possible now and save
> > some needless chatter about future packages.
> >
> > Note: I am aware that htslib 1.4 is available, but it has API changes that
> > make it incompatible with a number of dependent packages, so it will be a
> > while before it becomes mainstream in genomics research.
> 1) Why does the package Makefile set CPPFLAGS to -I${PREFIX}/include?
>     If it's to find the zlib headers, you shouldn't need to do anything
>     at all after including the zlib/buildlink3.mk file.
>
> 2) The package Makefile sets MAKE_ENV to various macros for installing
>     files.  INSTALL_DIR should probably be set to ${INSTALL_LIB_DIR}.
>
> 3) patch-Makefile is incorrect.  The package should be respecting the
>     value of ${PKGMANDIR} for the location of the manpages.
>
> 4) Why does the patch to configure.ac set the version to 1.3 while the
>     package version is 1.3.2?
1) I honestly don't remember, but it doesn't appear necessary in 1.3.2, 
so I removed it.

2) I'll consider using INSTALL_LIB_DIR, INSTALL_PROGRAM_DIR, etc.  This 
will require more extensive patching, though...

3) Good catch, thanks.

4) This is the risk of doing a SUBST on top of a static patch. When the 
patch was updated, it picked up the SUBST version change as well.  This 
approach was adopted from the FreeBSD port and I generally avoid it 
myself.  Unnecessary in this case, so I removed the patch file and do 
everything in the SUBST now.

Thanks for the feedback,

    JB

--
Earth is a beta site.