Bug 697836 - Review Request: mmseq - Haplotype and isoform specific expression estimation for RNA-seq
Summary: Review Request: mmseq - Haplotype and isoform specific expression estimation ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Volker Fröhlich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-04-19 11:58 UTC by Adam Huffman
Modified: 2011-07-12 15:00 UTC (History)
3 users (show)

Fixed In Version: mmseq-0.9.11-1.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-12 15:00:14 UTC
Type: ---
Embargoed:
volker27: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Adam Huffman 2011-04-19 11:58:28 UTC
Spec URL: http://verdurin.fedorapeople.org/reviews/mmseq/mmseq.spec
SRPM URL: http://verdurin.fedorapeople.org/reviews/mmseq/mmseq-0.9.9-2.fc16.src.rpm
Description: 
Software and instructions for fast, scalable isoform expression
estimation using multi-mapping RNA-seq reads.

Comment 1 Volker Fröhlich 2011-05-06 22:44:39 UTC
Just some comments:

Please include the license file. The postal addresses of the FSF are outdated in several files.

Use -p on the install commands, at least for the scripts. One of the scripts uses the samtools binary, so you'll need an explicit Require on that, as RPM will only install samtools-libs

Besides the first part of the package description is pretty generic, it says, it would contain instructions. I can't see any and especially the mmseq binary is especially sparse on saying anything. Why does the description not have the word "Haplotype"?

Why did you choose to not install the Perl script?

You might want to add "-b .name_of_the_patch" to the patch macros. Please also comment on the patches.

Comment 2 Adam Huffman 2011-05-08 12:01:13 UTC
(In reply to comment #1)
> Just some comments:

Thanks for looking

> 
> Please include the license file. The postal addresses of the FSF are outdated
> in several files.
> 

File included and I'll contact upstream about the out of data FSF address.

> Use -p on the install commands, at least for the scripts. One of the scripts
> uses the samtools binary, so you'll need an explicit Require on that, as RPM
> will only install samtools-libs
> 

Done.

> Besides the first part of the package description is pretty generic, it says,
> it would contain instructions. I can't see any and especially the mmseq binary
> is especially sparse on saying anything. Why does the description not have the
> word "Haplotype"?
>

I was trying to differentiate the description from the summary but have improved it now.
 
> Why did you choose to not install the Perl script?
> 

Fixed.

> You might want to add "-b .name_of_the_patch" to the patch macros. Please also
> comment on the patches.

Done.

I've also updated to the latest upstream release.

http://verdurin.fedorapeople.org/reviews/mmseq/mmseq.spec

http://verdurin.fedorapeople.org/reviews/mmseq/mmseq-0.9.10b-1.fc16.src.rpm

Comment 3 Adam Huffman 2011-05-13 23:41:32 UTC
The upstream developer has said he'll fix the FSF address problems in the next release.

Comment 4 Volker Fröhlich 2011-05-14 09:55:08 UTC
The naming guidelines are not met for this pre-release version. Please see the review below for details and correct it.

Concerning the current FSF address, please see https://bugzilla.redhat.com/show_bug.cgi?id=690919#c5
It is not considered as a blocker, so it is up to you.

Please correct the VERSION file prior building, as it contains the wrong version number. I'd also exclude it from the file section, as it serves no use as far as I can see. 

I'd prefer having a new line for every BuildRequires, as it is clearer. You can also be more specific on the script file names in the files section.

If you don't plan to introduce the package into EPEL, you can drop the clean section, the rm -rf %{buildroot} and the BuildRoot definition. It seems like, there are no samtools in EPEL yet.

Remove the extra white-space in front of the word "Example".

I'm not sure if zlib is used at all. It's in the flags, but I can't see how they'd use it. This might not be important.

--------------------------------------------------------
Review:

[+] Good
[-] Needs work
[0] Does not apply

MUST:
=====

[+] rpmlint:

[makerpm@fedora14 mmseq_0.9.10b]$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/mmseq-*
mmseq.src: W: spelling-error Summary(en_US) Haplotype -> Haplology, Holotype, Haplography
mmseq.src: W: spelling-error Summary(en_US) isoform -> iodoform, isomorph, proforma
mmseq.src: W: spelling-error %description -l en_US haplotype -> haplology, holotype, haplography
mmseq.src: W: spelling-error %description -l en_US isoform -> iodoform, isomorph, proforma
mmseq.src: W: spelling-error %description -l en_US multi -> mulch, mufti
mmseq.x86_64: W: spelling-error Summary(en_US) Haplotype -> Haplology, Holotype, Haplography
mmseq.x86_64: W: spelling-error Summary(en_US) isoform -> iodoform, isomorph, proforma
mmseq.x86_64: W: spelling-error %description -l en_US haplotype -> haplology, holotype, haplography
mmseq.x86_64: W: spelling-error %description -l en_US isoform -> iodoform, isomorph, proforma
mmseq.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
mmseq.x86_64: E: incorrect-fsf-address /usr/bin/testregexp.rb
mmseq.x86_64: E: incorrect-fsf-address /usr/bin/sam2hits.rb
mmseq.x86_64: E: incorrect-fsf-address /usr/bin/filterGTF.rb
mmseq.x86_64: E: incorrect-fsf-address /usr/bin/fastagrep.sh
mmseq.x86_64: E: incorrect-fsf-address /usr/bin/haploref.rb
mmseq.x86_64: W: no-manual-page-for-binary haploref.rb
mmseq.x86_64: W: no-manual-page-for-binary ensembl_gtf_to_gff.pl
mmseq.x86_64: W: no-manual-page-for-binary mmseq
mmseq.x86_64: W: no-manual-page-for-binary get_isize.rb
mmseq.x86_64: W: no-manual-page-for-binary bam2hits
mmseq.x86_64: W: no-manual-page-for-binary fastagrep.sh
mmseq.x86_64: W: no-manual-page-for-binary filterGTF.rb
mmseq.x86_64: W: no-manual-page-for-binary sam2hits.rb
mmseq.x86_64: W: no-manual-page-for-binary testregexp.rb
mmseq.x86_64: W: no-manual-page-for-binary pileup.sh
mmseq-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/mmseq_0.9.10b/mmseq.cpp
mmseq-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/mmseq_0.9.10b/sokal.cc
3 packages and 0 specfiles checked; 7 errors, 20 warnings.

[-] Naming according to the Package Naming Guidelines: Please see http://fedoraproject.org/wiki/PackageNamingGuidelines#NonNumericRelease
[+] Spec file matches base package name
[+] Packaging guidelines met
[+] License approved for Fedora
[+] License field in spec matches code
[+] License file included, if source package includes it
[+] Spec in American English
[+] Spec is legible
[+] Sources match upstream md5sum: 1f1c5b338eec23994fd84edb7e00b17a
[+] Compiles and builds into binary RPMs on at least one primary architecture:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3071848

[0] ExcludeArch is specified and commented
[0] Locales are handled correctly
[+] All build dependencies listed
[0] Calls ldconfig for its shared libraries
[+] No bundled system libraries
[0] Stated as relocatable package
[+] Owns all its directories or requires package that does
[+] No file listing duplicates
[+] File permissions correct
[+] Consistent use of macros
[+] Code or permissible content
[0] Large documentation in -doc subpackage
[+] No runtime dependency of files listed as %doc
[0] Header files in -devel subpackage
[0] Static files in -static subpackage
[0] Library files without suffix in -devel subpackage
[0] Devel-package requires base package
[0] No .la libtool archives
[0] GUI application includes properly installed %{name}.desktop file
[+] No files or directories owned, that other packages own
[+] Filenames in packages are UTF-8

SHOULD:
=======

[0] Query upstream if no license text is included
[+] Package builds in mock: Tried fedora-rawhide-x86_64, fedora-rawhide-i386
[?] Package works as described -- Haven't tried
[0] Scriptlets are sane, if used
[0] Subpackages other than -devel should require base package (versioned)
[0] pkgconfig files in -devel subpackage
[0] Dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider
requiring the package which provides the file instead of the file itself
[-] Contain man pages, where they make sense -- Please try to query upstream on this.

Comment 5 Volker Fröhlich 2011-05-14 13:44:06 UTC
Replacing "-lz" with "-Wl,--as-needed -lz" proves right: zlib is unnecessary. You can drop from the BRs and remove it from the Makefile.

Comment 6 Volker Fröhlich 2011-05-14 19:01:13 UTC
Please also delete the 4 pre-built binaries in the prep section!

http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries

Comment 7 Adam Huffman 2011-05-27 14:13:59 UTC
New version at:

http://verdurin.fedorapeople.org/reviews/mmseq/mmseq.spec

http://verdurin.fedorapeople.org/reviews/mmseq/mmseq-0.9.10b-3.fc16.src.rpm

I patched the Makefile as you suggested but without the zlib-devel BR builds in Mock failed.

I will be building for EPEL and samtools will be there soon (I maintain the EPEL branches).

The upstream author explained that 0.9.10b is a post-release update, which according to the guidelines at http://fedoraproject.org/wiki/PackageNamingGuidelines#NonNumericRelease is acceptable as is.

I've removed VERSION.

Comment 8 Adam Huffman 2011-05-27 14:24:12 UTC
The wrong version was uploaded - I've fixed that now.

Comment 9 Adam Huffman 2011-05-28 10:23:11 UTC
New upstream release:

http://verdurin.fedorapeople.org/reviews/mmseq/mmseq.spec

http://verdurin.fedorapeople.org/reviews/mmseq/mmseq-0.9.11-1.fc16.src.rpm

I've reminded the upstream author about the incorrect FSF addresses.

Comment 10 Volker Fröhlich 2011-05-28 13:47:53 UTC
As the name issue is solved, the package is fine.

md5sum: 6c75e6866eb702b7b22892b63340280e

http://koji.fedoraproject.org/koji/taskinfo?taskID=3097653

========
APPROVED
========

Comment 11 Adam Huffman 2011-05-29 14:42:26 UTC
New Package SCM Request
=======================
Package Name: mmseq
Short Description: Haplotype and isoform specific expression estimation for RNA-seq
Owners: verdurin
Branches: f14 f15 el5 el6
InitialCC:

Comment 12 Jason Tibbitts 2011-05-29 15:21:42 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2011-06-17 15:26:52 UTC
mmseq-0.9.11-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/mmseq-0.9.11-1.el5

Comment 14 Fedora Update System 2011-06-21 17:13:46 UTC
mmseq-0.9.11-1.el5 has been pushed to the Fedora EPEL 5 testing repository.

Comment 15 Fedora Update System 2011-07-12 15:00:07 UTC
mmseq-0.9.11-1.el5 has been pushed to the Fedora EPEL 5 stable repository.


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