Bug 239282 - Review Request: seaview - Graphical multiple sequence alignment editor
Summary: Review Request: seaview - Graphical multiple sequence alignment editor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-05-07 12:31 UTC by Christian Iseli
Modified: 2007-11-30 22:12 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-07-05 22:34:49 UTC
Type: ---
Embargoed:
j: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Christian Iseli 2007-05-07 12:31:58 UTC
Spec URL: ftp://ftp.licr.org/pub/seaview.spec
SRPM URL: ftp://ftp.licr.org/pub/seaview-0.20070417-0.src.rpm
Description:
SeaView is a graphical multiple sequence alignment editor developed by Manolo
Gouy.  SeaView is able to read and write various alignment formats (NEXUS, MSF,
CLUSTAL, FASTA, PHYLIP, MASE).  It allows to manually edit the alignment, and
also to run DOT-PLOT or CLUSTALW/MUSCLE programs to locally improve the
alignment.


Another bioinformatics tool.  The code is under the GPL, though the author
didn't include the GPL license itself in the source tarball (the source
files are clearly marked though).

Another minor annoyance is that there is no version info anywhere...
so I used the date, which is what the OA seems to do as well.

The code can optionnaly make use of the PDF-libs, but since this apparently
can't go in Fedora, this package compiles using the PostScript output.

Compiles in mock, has a clean rpmlint output, and WorksForMe(tm) :-)

Comment 1 Bernard Johnson 2007-05-08 04:53:05 UTC
(In reply to comment #0)
> SRPM URL: ftp://ftp.licr.org/pub/seaview-0.20070417-0.src.rpm

I can't find anything in the packaging guidelines that specifically address "no
version", so I could be wrong when I say this...

To me it seems pointless to prefix the date with a 0. in this case.  You will
never have a number low enough that upstream can't trump you and release
something like 0.01 which would be < EVR than yours.  I would think this would
be the perfect case for an epoch if upstream gets a clue and versions the
package at a later date.

Comment 2 Christian Iseli 2007-05-08 07:29:12 UTC
(In reply to comment #1)
> To me it seems pointless to prefix the date with a 0. in this case.  You will
> never have a number low enough that upstream can't trump you and release
> something like 0.01 which would be < EVR than yours.  I would think this would
> be the perfect case for an epoch if upstream gets a clue and versions the
> package at a later date.

This got me thinking, and I had another look at what we currently have in FC-6.
I think it would actually make more sense to put zero as a version number, and
follow the pre-release snapshot guidelines for the release number.  This way,
when upstream so decides, there is a very good probability that the version
assigned will be higher than zero (0) and we avoid the epoch thing.  Deal ?

new SPEC: ftp://ftp.licr.org/pub/seaview.spec
new SRPM: ftp://ftp.licr.org/pub/seaview-0-0.20070417.src.rpm


Comment 3 Jason Tibbitts 2007-06-12 22:19:16 UTC
This has been sitting for a while and looks kind of interesting; I'll take a look.

The naming guidelines actually cover this situation with an example

  Release Tag for Pre-Release Packages:
    * 0.%{X}.%{alphatag}
  Example (pre-release svn checkout):
    kismet-0-0.1.20040110svn (this is a pre-release, svn checkout of kismet)
    kismet-0-0.2.20040110svn (this is a bugfix to the previous package)
(etc.)

So I guess another digit is in order.

One fundamental problem I see is that the tarball doesn't match what I get from
upstream.  It looks like there's been another release.  Can you verify?  (Three
chears for insane upstreams who don't bother to version anything.)

Comment 4 Christian Iseli 2007-06-13 06:30:15 UTC
(In reply to comment #3)
> This has been sitting for a while and looks kind of interesting; I'll take a look.

Thanks tibbs

> So I guess another digit is in order.

Right.  I guess I need to learn to read some day :-)
Fixed

> One fundamental problem I see is that the tarball doesn't match what I get from
> upstream.  It looks like there's been another release.  Can you verify?  (Three
> chears for insane upstreams who don't bother to version anything.)

Yea, upstream put a new "version"... /me cries...

Here's the updated stuff:
SPEC: ftp://ftp.licr.org/pub/seaview.spec
SRPM: ftp://ftp.licr.org/pub/seaview-0-0.1.20070515.fc7.src.rpm

Comment 5 Jason Tibbitts 2007-06-28 02:20:56 UTC
Sorry for not getting back to this sooner.  It looks like I need to be quicker
or something else is amiss, because I'm still seeing a different upstream
tarball that what's in this package.

Also, this does seem to be a graphical application and so probably deserves a
.desktop file.  An icon is even included in the tarball.


Comment 6 Christian Iseli 2007-06-28 13:51:00 UTC
Yup, tarball changed again...

I added a desktop file.

Here's the updated stuff:
SPEC: ftp://ftp.licr.org/pub/seaview.spec
SRPM: ftp://ftp.licr.org/pub/seaview-0-0.1.20070615.fc7.src.rpm


Comment 7 Jason Tibbitts 2007-06-28 14:59:13 UTC
I'd better get to this one fast before the tarball changes again....


Comment 8 Jason Tibbitts 2007-06-28 16:03:16 UTC
OK, the only issue I see is that we don't use the X-Fedora category these days,
so you shouldn't have the "--add-category" bit.

But that's really minor; you can fix it when you check in.

APPROVED

Review:
* source files match upstream:
   ce08adfd4f177082c6ff9eb049d4405a9db0ed9383c2f41705b1c8a719036880
   seaview.tar
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   seaview = 0-0.1.20070615.fc8
  =
   libX11.so.6()(64bit)
   libfltk.so.1.1()(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
* %check is not present (or at least it's empty); no test suite upstream.
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
X desktop file is present and looks OK but uses X-Fedora category.

Comment 9 Christian Iseli 2007-06-28 19:31:26 UTC
Thanks tibbs

(In reply to comment #8)
> OK, the only issue I see is that we don't use the X-Fedora category these days,
> so you shouldn't have the "--add-category" bit.

Hmm, I supposed I should have snarfed the .desktop file and spec bits from
another package :)
I'll fix it.
A quick grep for X-Fedora in all the devel/*.spec files turns about 400 matches,
though about half of them are changelog entries about the removal of X-Fedora.

Still, I think we have about 200 packages that need fixing.  If I find some
time, I'll try to mass-BZ the issue.

Comment 10 Christian Iseli 2007-06-28 19:36:00 UTC
New Package CVS Request
=======================
Package Name: seaview
Short Description: Graphical multiple sequence alignment editor
Owners: Christian.Iseli
Branches: FC-6 F-7
InitialCC: 

Comment 11 Kevin Fenzi 2007-06-28 20:03:29 UTC
cvs done.

Comment 12 Christian Iseli 2007-07-05 22:34:49 UTC
imported and built.  Thanks for the review :)


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