Bug 597307 - Review Request: fastx_toolkit - Tools to process short-reads FASTA/FASTQ files
Summary: Review Request: fastx_toolkit - Tools to process short-reads FASTA/FASTQ files
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 598511
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-05-28 15:56 UTC by Adam Huffman
Modified: 2011-05-27 10:35 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-04-07 21:40:36 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Adam Huffman 2010-05-28 15:56:21 UTC
Spec URL: http://verdurin.org.uk/~verdurin/fedora/reviews/fastx_toolkit/fastx_toolkit.spec
SRPM URL: http://verdurin.org.uk/~verdurin/fedora/reviews/fastx_toolkit/fastx_toolkit-0.0.13-1.fc12.src.rpm
Description: 

The FASTX-Toolkit is a collection of command line tools for
Short-Reads FASTA/FASTQ files preprocessing.

Next-Generation sequencing machines usually produce FASTA or FASTQ
files, containing multiple short-reads sequences (possibly with
quality information).

The main processing of such FASTA/FASTQ files is mapping (aka
aligning) the sequences to reference genomes or other databases using
specialized programs. Example of such mapping programs are: Blat,
SHRiMP, LastZ, MAQ and many many others.

However, It is sometimes more productive to preprocess the FASTA/FASTQ
files before mapping the sequences to the genome - manipulating the
sequences to produce better mapping results.

The FASTX-Toolkit tools perform some of these preprocessing tasks.

Comment 1 Takanori MATSUURA 2010-06-01 10:21:36 UTC
This is an informal review. Formal review will follow.

Critical issue:
MUST: The package MUST successfully compile and build into binary rpms.
SHOULD: The package builds in mock.
$ mock --rebuild fastx_toolkit-0.0.13-1.fc12.src.rpm
is failed because libgtextutils-devel which is set as BuildRequires is not available by Fedora.

You should add libgtextutils-devel package to Fedora first.


Issues:
$ rpmlint fastx_toolkit.spec
fastx_toolkit.spec:8: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 8)
Please fix mixed usage of spaces and tabs.


Lists confirmed:
+ rpmlint against SRPM returns spelling-error warning. However the words pointed by rpmlint are from official website and seem to be no problem.

+ Spec file name meets Packaging Guidelines.
+ License: AGPLv3 meets Licensing Guidelines.
+ Source file match with upstream one with md5sum and sha1sum.


MUST: The package does not yet exist in Fedora. The Review Request is not a
duplicate. OK
MUST: The spec file for the package is legible and macros are used
consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the 
Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license.
OK
MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from
upstream, the packager should query upstream to include it. OK

The following item will be checked after the critical issue is solved.
MUST: Optflags are used and time stamps preserved.
MUST: Packages containing shared library files must call ldconfig.
MUST: A package must own all directories that it creates or require the package
that owns the directory.
MUST: Files only listed once in %files listings.
MUST: Debuginfo package is complete.
MUST: Permissions on files must be set properly.
MUST: All relevant items are included in %doc. Items in %doc do not affect
runtime of application.
MUST: Header files must be in a -devel package.
MUST: Static libraries must be in a -static package.
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
MUST: If a package contains library files with a suffix then library files
ending in .so must go in a -devel package.
MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency.
MUST: Packages does not contain any .la libtool archives.
MUST: Desktop files are installed properly.
MUST: No file conflicts with other packages and no general names.

Comment 2 Adam Huffman 2010-06-01 14:34:29 UTC
(In reply to comment #1)
> This is an informal review. Formal review will follow.
> 

Thanks for taking a look.

> Critical issue:
> MUST: The package MUST successfully compile and build into binary rpms.
> SHOULD: The package builds in mock.
> $ mock --rebuild fastx_toolkit-0.0.13-1.fc12.src.rpm
> is failed because libgtextutils-devel which is set as BuildRequires is not
> available by Fedora.
> 
> You should add libgtextutils-devel package to Fedora first.
> 

Yes, that's right.  I uploaded a bunch of new requests late on Friday, after having installed them locally.  The request for libgtextutils is at:

https://bugzilla.redhat.com/show_bug.cgi?id=598511

> 
> Issues:
> $ rpmlint fastx_toolkit.spec
> fastx_toolkit.spec:8: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab:
> line 8)
> Please fix mixed usage of spaces and tabs.
> 

Will take a look at that.

> 
> Lists confirmed:
> + rpmlint against SRPM returns spelling-error warning. However the words
> pointed by rpmlint are from official website and seem to be no problem.
> 
> + Spec file name meets Packaging Guidelines.
> + License: AGPLv3 meets Licensing Guidelines.
> + Source file match with upstream one with md5sum and sha1sum.
> 
> 
> MUST: The package does not yet exist in Fedora. The Review Request is not a
> duplicate. OK
> MUST: The spec file for the package is legible and macros are used
> consistently. OK
> MUST: The package must be named according to the Package Naming Guidelines. OK
> MUST: The spec file name must match the base package %{name}. OK
> MUST: The package must be licensed with a Fedora approved license and meet the 
> Licensing Guidelines. OK
> MUST: The License field in the package spec file must match the actual license.
> OK
> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. OK
> MUST: The spec file MUST handle locales properly. N/A
> MUST: Clean section exists. OK
> MUST: Large documentation files must go in a -doc subpackage. N/A
> MUST: Buildroot cleaned before install. OK
> SHOULD: %{?dist} tag is used in release. OK
> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. OK
> 
> The following item will be checked after the critical issue is solved.
> MUST: Optflags are used and time stamps preserved.
> MUST: Packages containing shared library files must call ldconfig.
> MUST: A package must own all directories that it creates or require the package
> that owns the directory.
> MUST: Files only listed once in %files listings.
> MUST: Debuginfo package is complete.
> MUST: Permissions on files must be set properly.
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application.
> MUST: Header files must be in a -devel package.
> MUST: Static libraries must be in a -static package.
> MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
> MUST: If a package contains library files with a suffix then library files
> ending in .so must go in a -devel package.
> MUST: In the vast majority of cases, devel packages must require the base
> package using a fully versioned dependency.
> MUST: Packages does not contain any .la libtool archives.
> MUST: Desktop files are installed properly.
> MUST: No file conflicts with other packages and no general names.

Comment 3 Michael Schwendt 2010-06-21 09:52:45 UTC
> MUST: Optflags are used and time stamps preserved.

Not entirely. The source code overrides Fedora's optflags.


* Description of the -devel subpackage doesn't match its contents:

| The fastx_toolkit-devel package contains libraries and
| header files for developing applications that use fastx_toolkit.

It doesn't contain any libraries and header files.

It contains two M4 macro definition files from the GNU Autoconf Archive. It's questionable to assign them to this package and place them in the public aclocal directory.

$ rpmls -p fastx_toolkit-devel-0.0.13-1.fc13.x86_64.rpm|grep local
-rw-r--r--  /usr/share/aclocal/ax_c_long_long.m4
-rw-r--r--  /usr/share/aclocal/ax_cxx_header_stdcxx_tr1.m4

The only other contents of the -devel package is the big tree at:
/usr/share/doc/fastx_toolkit-devel-0.0.13/galaxy

Licensed differently (!).
Lots of unusable Makefile* files.
Icon files, XML files, not really any documentation about all that.
Strange place to dump that into %doc and a -devel pkg.

Comment 4 Adam Huffman 2010-06-21 10:29:52 UTC
Thanks for taking a look.  Yes, the description is a bit generic - I thought I'd put in a note to this bug about the Galaxy files but clearly forgot.  They're for integrating the package with a local Galaxy installation.  I wasn't quite sure how to handle them.  Perhaps a different subpackage - fastx_toolkit-galaxy?

Would you recommend not packaging the M4 files?

Comment 5 Michael Schwendt 2010-06-21 11:49:49 UTC
> Would you recommend not packaging the M4 files?

Yes.

Those two files either belong into a comprehensive autoconf-archive package (for the scripts included in the GNU Autoconf Archive) or into the local autotools build framework of software that wants to use these macros (e.g. in a local "m4" subdir).

If more packages started placing arbitrary M4 files from the Autoconf Archive into /usr/share/aclocal, this would increase the risk of causing conflicts.

Comment 6 Susi Lehtola 2010-08-24 16:58:07 UTC
rpmlint output:
fastx_toolkit.src: W: spelling-error %description -l en_US preprocessing -> reprocessing, p reprocessing, preprocessed
fastx_toolkit.src: W: spelling-error %description -l en_US preprocess -> reprocess, p reprocess, processors
fastx_toolkit.src:8: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 8)
fastx_toolkit.x86_64: W: spelling-error %description -l en_US preprocessing -> reprocessing, p reprocessing, preprocessed
fastx_toolkit.x86_64: W: spelling-error %description -l en_US preprocess -> reprocess, p reprocess, processors
fastx_toolkit.x86_64: W: no-manual-page-for-binary fasta_clipping_histogram.pl
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_trimmer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_quality_stats
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_filter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_trimmer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_boxplot_graph.sh
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_barcode_splitter.pl
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_converter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_nucleotide_distribution_line_graph.sh
fastx_toolkit.x86_64: W: no-manual-page-for-binary fasta_formatter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_collapser
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_renamer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fasta_nucleotide_changer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_reverse_complement
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_to_fasta
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_nucleotide_distribution_graph.sh
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_masker
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_artifacts_filter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_uncollapser
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_clipper
fastx_toolkit-debuginfo.x86_64: W: spelling-error Summary(en_US) fastx -> fast, fasts, fast x
fastx_toolkit-debuginfo.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
fastx_toolkit-devel.x86_64: W: spelling-error Summary(en_US) fastx -> fast, fasts, fast x
fastx_toolkit-devel.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
fastx_toolkit-devel.x86_64: W: spurious-executable-perm /usr/share/doc/fastx_toolkit-devel-0.0.13/galaxy/tools/fastx_toolkit/fastx_barcode_splitter_galaxy_wrapper.sh
4 packages and 0 specfiles checked; 0 errors, 30 warnings.

- Check your tabs vs spaces.

- Placing the galaxy stuff somewhere else than %doc gets rid of the exec perm warning. When packaging the galaxy stuff, be sure not to include any Makefiles or the like.

- Please use
 http://hannonlab.cshl.edu/fastx_toolkit/
as the URL. It's nicer not to have macros in the URL, so one can cut'n'paste from the spec.

- Don't ship the m4 files, as instructed by Michael.

**

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSWORK
- License is AGPLv3+, not AGPLv3.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
6d233ff4ae3d52c457d447179f073a56  fastx_toolkit-0.0.13.tar.bz2
6d233ff4ae3d52c457d447179f073a56  ../SOURCES/fastx_toolkit-0.0.13.tar.bz2

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- As already pointed out by Michael, the build process overrides the Fedora optimization flags.
- Use
 make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" CXXFLAGS="$RPM_OPT_FLAGS"
to correct this.
- Also, please use
 make install INSTALL="install -p"
to keep the time stamps in %install.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK

MUST: Files only listed once in %files listings. OK
- Maybe again make things a bit more verbose with
 %{_bindir}/fast*
?

MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
EPEL: Clean section exists. OK
EPEL: Buildroot cleaned before install. OK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A

Comment 7 Chen Lei 2010-08-25 00:59:49 UTC
I'm sure that galaxy is not for development purpose, and m4 macros is completely useless for this package.

Comment 8 Adam Huffman 2010-08-25 16:23:18 UTC
New version at:

http://verdurin.org.uk/~verdurin/fedora/reviews/fastx_toolkit/fastx_toolkit.spec

http://verdurin.org.uk/~verdurin/fedora/reviews/fastx_toolkit/fastx_toolkit-0.0.13-2.fc12.src.rpm

As with libgtextutils, there's no apparent acceptable license tag "AGPLv3+".

I've made a -galaxy subpackage for the galaxy/ directory.

Comment 9 Susi Lehtola 2010-11-06 13:15:11 UTC
Whoops, I'm terribly sorry that this has slipped under my radar, as I've been quite busy at $DAYJOB. I'll try to look through this ASAP.

Comment 10 Susi Lehtola 2010-11-07 09:41:14 UTC
rpmlint output now at:

fastx_toolkit.src: W: spelling-error %description -l en_US preprocessing -> reprocessing, p reprocessing, preprocessed
fastx_toolkit.src: W: spelling-error %description -l en_US preprocess -> reprocess, p reprocess, procession
fastx_toolkit.x86_64: W: spelling-error %description -l en_US preprocessing -> reprocessing, p reprocessing, preprocessed
fastx_toolkit.x86_64: W: spelling-error %description -l en_US preprocess -> reprocess, p reprocess, procession
fastx_toolkit.x86_64: W: no-manual-page-for-binary fasta_clipping_histogram.pl
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_trimmer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_quality_stats
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_filter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_trimmer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_boxplot_graph.sh
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_barcode_splitter.pl
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_quality_converter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_nucleotide_distribution_line_graph.sh
fastx_toolkit.x86_64: W: no-manual-page-for-binary fasta_formatter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_collapser
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_renamer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fasta_nucleotide_changer
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_reverse_complement
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_to_fasta
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_nucleotide_distribution_graph.sh
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastq_masker
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_artifacts_filter
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_uncollapser
fastx_toolkit.x86_64: W: no-manual-page-for-binary fastx_clipper
fastx_toolkit-debuginfo.x86_64: W: spelling-error Summary(en_US) fastx -> fast, fasts, fast x
fastx_toolkit-debuginfo.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
fastx_toolkit-galaxy.x86_64: W: spelling-error Summary(en_US) fastx -> fast, fasts, fast x
fastx_toolkit-galaxy.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
fastx_toolkit-galaxy.x86_64: W: spelling-error %description -l en_US metagenomic -> meta genomic, meta-genomic, metagenesis
fastx_toolkit-galaxy.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 30 warnings.

These are OK.

Most of the issues have been fixed. However, some still remain:

NEEDSWORK:
 - The -galaxy package contains autotools Makefiles. Get rid of them.
 - The license is AGPLv3+ as in libgtextutils.

Comment 12 Susi Lehtola 2010-12-05 16:54:16 UTC
Ping Adam, what's the status?

Comment 13 Takanori MATSUURA 2010-12-17 11:42:07 UTC
Autoconf-archive is now in review at bug 663925.

Comment 14 Adam Huffman 2011-03-22 11:32:49 UTC
Jussi,

Have you had a change to look at the latest version?

Comment 15 Susi Lehtola 2011-03-22 11:45:27 UTC
Crap, I had forgotten all about this review. Taking a look shortly.

Comment 16 Susi Lehtola 2011-03-22 17:07:59 UTC
You're missing ownership of
 %{_datadir}/%{name}
It should be owned by -galaxy. Otherwise things should be fine.

Please fix the ownership before git import; what you need to do is change
 %{_datadir}/%{name}/galaxy/
to
 %{_datadir}/%{name}/
in -galaxy.

This package has been

APPROVED

Comment 17 Adam Huffman 2011-03-22 17:30:00 UTC
Many thanks for the review - I've made that last change in -4

New Package SCM Request
=======================
Package Name: fastx_toolkit
Short Description:  Tools to process short-reads FASTA/FASTQ files
Owners: verdurin
Branches: f14 f15 el5 el6
InitialCC:

Comment 18 Jason Tibbitts 2011-03-22 17:51:58 UTC
Git done (by process-git-requests).

Comment 19 Pierre-YvesChibon 2011-05-25 12:15:37 UTC
Two month have past but nothing has been imported in the git.

Comment 20 Adam Huffman 2011-05-27 10:35:38 UTC
Oops - I forgot to import the package...

Many thanks for spotting that.  Have just done so and submitted updates for all four branches.


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