Bug 165258
Summary: | Review Request: hmmer - Profile HMM software for protein sequence analysis | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Christian Iseli <Christian.Iseli> |
Component: | Package Review | Assignee: | Matthias Saou <matthias> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://hmmer.wustl.edu | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2005-08-08 11:00:38 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Christian Iseli
2005-08-05 22:31:27 UTC
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 :-) > - 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 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 ;-) FYI, you didn't (yet) add the new component to the owners.list file in CVS. Don't forget to do it. 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 ? 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 There's at least one warning printed by GCC 4 during compilation, which would be worth examining and getting rid of. (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 :-) (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. Ah, okay. Sounds like something upstream might want to get rid of. |