Bug 165258 - Review Request: hmmer - Profile HMM software for protein sequence analysis
Review Request: hmmer - Profile HMM software for protein sequence analysis
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Saou
David Lawrence
http://hmmer.wustl.edu
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-05 18:31 EDT by Christian Iseli
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-08-08 07:00:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Christian Iseli 2005-08-05 18:31:27 EDT
Spec Name or Url: ftp://ftp.licr.org/pub/software/unix/hmmer.spec
SRPM Name or Url: ftp://ftp.licr.org/pub/software/unix/hmmer-2.3.2-1.src.rpm
Description:
Profile hidden Markov models (profile HMMs) can be used to do sensitive
database searching using statistical descriptions of a sequence family's
consensus.  HMMER is a freely distributable implementation of profile HMM
software for protein sequence analysis.

License is GPL.
Builds in mock.
Rpmlint appears clean.
Comment 1 Matthias Saou 2005-08-06 05:46:32 EDT
A few questions first :
- Why not use %{version} in the source line? It lowers the eventuality of
changing the "Version:" line, forgetting to change the source on too, and ending
up with a package that contains the old version by seems like it contains the
new one.
- Same logic applies to the "%ifarch ppc" part : If you have do this, you'll
have less chances of forgetting to update the configure options for either of
ppc or non-ppc :

%configure \
%ifarch ppc
    --enable-altivec \
%endif
    --enable-lfs \
    --enable-threads

- Shouldn't the "make check" go into a separate %check section?

One remark to point out that the DESTDIR patch is somewhat overkill, and it
would have been easier to simply use :

make install BINDIR=$RPM_BUILD_ROOT%{_bindir} MANDIR=$RPM_BUILD_ROOT%{_mandir}

Since the Makefile only uses these two locations.

Last, you don't seem to include any of the files from the "tutorial" directory,
is that on purpose?

Other than that, things look pretty good :-)
Comment 2 Christian Iseli 2005-08-06 12:41:54 EDT
> - Why not use %{version} in the source line?

Oversight.  I simply copied the URL from the homepage...  Fixed.

> - Same logic applies to the "%ifarch ppc" part

Agreed.  Fixed.

> - Shouldn't the "make check" go into a separate %check section?

Yup.  I'm still not used to having a %check section... (and vim won't colorize
it either).  Fixed.

> the DESTDIR patch is somewhat overkill

Ok.  Fixed.

> Last, you don't seem to include any of the files from the "tutorial"
> directory, is that on purpose?

Nope.  Oversight.  Fixed.

Updated stuff available here:
ftp://ftp.licr.org/pub/software/unix/hmmer.spec
ftp://ftp.licr.org/pub/software/unix/hmmer-2.3.2-2.src.rpm

Please re-review, and thanks for your comments :)

Cheers,
 Christian
Comment 3 Matthias Saou 2005-08-06 18:48:56 EDT
Seems all fine to me now!
I'm just not able to test the actual functionality of the binaries, because I
don't know what to do with them, but the package itself is fine, so I'll approve it.

Next step is for you to import the package, and once it's built, you can close
this bug as NEXTRELEASE ;-)
Comment 4 Matthias Saou 2005-08-08 07:05:27 EDT
FYI, you didn't (yet) add the new component to the owners.list file in CVS.
Don't forget to do it.
Comment 5 Christian Iseli 2005-08-08 07:16:27 EDT
I'm probably in dumb mode, but:
 - where is this owners.list file ?
 - where is it explained in the wiki ?

Thanks for the review :-)

Should I still send an "APPROVED" mail to extras-commit, or is this obsoleted by
the new review process ?
Comment 6 Ville Skyttä 2005-08-08 10:54:32 EDT
The owners.list info is not too easy to find, but here it is: 
http://fedoraproject.org/wiki/Extras/BugzillaAdmin 
https://www.redhat.com/archives/fedora-maintainers/2005-July/msg00030.html 
Comment 7 Michael Schwendt 2005-08-08 13:56:19 EDT
There's at least one warning printed by GCC 4 during compilation, which would be
worth examining and getting rid of.
Comment 8 Christian Iseli 2005-08-08 16:16:50 EDT
(In reply to comment #6)
> The owners.list info is not too easy to find, but here it is: 
> http://fedoraproject.org/wiki/Extras/BugzillaAdmin 
> https://www.redhat.com/archives/fedora-maintainers/2005-July/msg00030.html 

Ah, right.  Thanks.  I see the info is now in the wiki.  Great :-)
Comment 9 Christian Iseli 2005-08-08 16:25:20 EDT
(In reply to comment #7)
> There's at least one warning printed by GCC 4 during compilation, which would be
> worth examining and getting rid of.

If you mean this one:
gsi.c: In function 'GSIWriteIndex':
gsi.c:270: warning: comparison is always false due to limited range of data type

I had a look before submitting the package, and it seemed harmless, caused by a
useless test on a 16-bit value.

If not, could you be a bit more precise ?

Thanks.
Comment 10 Michael Schwendt 2005-08-09 09:13:07 EDT
Ah, okay. Sounds like something upstream might want to get rid of.

Note You need to log in before you can comment on or make changes to this bug.