Bug 444735 - Review Request: staden-io_lib - General purpose library to handle gene sequencing machine trace files
Summary: Review Request: staden-io_lib - General purpose library to handle gene sequen...
Keywords:
Status: CLOSED CURRENTRELEASE
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 Extras Quality Assurance
URL:
Whiteboard:
Depends On: 450889
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-04-30 12:28 UTC by Christian Iseli
Modified: 2008-08-10 12:29 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-10 12:29:03 UTC
Type: ---
Embargoed:
j: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Christian Iseli 2008-04-30 12:28:37 UTC
Here is a bioinformatics package, quite useful to read and
work with the various file formats produced by sequencing machines.

I have verified that the license tag is ok:
https://www.redhat.com/archives/fedora-devel-list/2008-April/msg02302.html

The name sounds a bit too general, but it is the upstream name...
Another possibility would be something like:
staden-io_lib or a variant thereof.

This package produces a bunch of binaries useful for
handling various file formats found in bioinformatics,
plus a static lib that can potentially be used by other
tools.  There is no shared library, so I just packaged
the static lib as per the guidelines.  If someone feels
motivated to write a patch to produce a shared lib, I'll
gladly accept patches and try to send them upstream.

This builds in mock for F-8 and devel, and the apps seem
to run fine as far as I know.

Spec URL: http://c4chris.fedorapeople.org/io_lib.spec
SRPM URL: http://c4chris.fedorapeople.org/io_lib-1.11.0-0.fc8.src.rpm
Description:
Io_lib is a library of file reading and writing code to provide a general
purpose trace file (and Experiment File) reading interface. The programmer
simply calls the (eg) read_reading to create a "Read" C structure with the
data loaded into memory. It has been compiled and tested on a variety
of unix systems, MacOS X and MS Windows.

Comment 1 Chris Ricker 2008-05-01 02:03:15 UTC
As someone who's used Staden, I'd recommend naming it as staden-io_lib - I'd
recognize it then, but probably not if I just saw a package named io_lib


Comment 2 Christian Iseli 2008-05-01 14:44:48 UTC
(In reply to comment #1)
> As someone who's used Staden, I'd recommend naming it as staden-io_lib - I'd
> recognize it then, but probably not if I just saw a package named io_lib

Ok, I'll change the name.


Comment 3 Christian Iseli 2008-05-06 12:54:08 UTC
Changing the name in the summary.  Will change it in the spec if/when there are
other comments or a review.

Comment 4 Jason Tibbitts 2008-06-06 03:07:07 UTC
I'll assume the changed name throughout.

It's not a big deal, but reading the summary makes me think that this parses
output from strace or something.  Perhaps you could elaborate on "trace file",
maybe adding something like "gene sequencing machine" before it if that's
actually appropriate.

It looks like 1.11.2 is currently available, although it doesn't seem like
anything significant has changed so I see no reason to wait for you to update
the package before I look at it.

I have to say, "libread" is a rather unfortunate name for a library.  Is there
any possibility of renaming it to something more unique?  It should be pretty
easy to do this given the -config binary.

* source files match upstream:
   3fc0f26aff8e95b7f37f55c4bcf6c462f413a0488a4b8ee1c3a81e3db7f591a4  
   io_lib-1.11.0.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summaries are OK.
* descriptions are OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
  io_lib-1.11.0-0.fc10.x86_64.rpm
   io_lib = 1.11.0-0.fc10
  =
   libcurl.so.4()(64bit)
   libz.so.1()(64bit)

  io_lib-devel-1.11.0-0.fc10.x86_64.rpm
   io_lib-static = 1.11.0-0.fc10
   io_lib-devel = 1.11.0-0.fc10
  =
   /bin/sh
   io_lib = 1.11.0-0.fc10

* %check is not present; no test suite upstream.  I haven't a clue how to test 
   this.
* 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 -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no pkgconfig files.
* static libraries are present:
   no dynamic libraries, so static libs are OK in the -devel package.
   -static provide is present.
* no libtool .la files.

Comment 5 Christian Iseli 2008-06-06 18:29:51 UTC
Thanks for looking into this, tibbs.

Here is an updated version:
http://c4chris.fedorapeople.org/staden-io_lib-1.11.2.1-0.fc8.src.rpm
http://c4chris.fedorapeople.org/staden-io_lib.spec

Version updated to latest upstream
Summary changed, as suggested
Name changed
libread changed to libstaden-read
io_lib-config changed to staden-io_lib-config and changed to output the proper
lib name

I hope I nailed them all... :)


Comment 6 Jason Tibbitts 2008-06-07 02:02:47 UTC
Did you also change the license?  rpmlint is yelling about it now and I don't
think it complained previously.  It seems to now be "BSD-like" when it doesn't
actually seem to be BSD-like at all.  I think you used to have MIT, which is
correct.  See
http://fedoraproject.org/wiki/Licensing/MIT#Minimal_variant_.28found_in_io_lib.29
which happens to have the license verbatim.

Besides the license I think this is fine; I'll approve this and you can change
it back when you check in.

APPROVED

Comment 7 Christian Iseli 2008-06-07 07:46:45 UTC
(In reply to comment #6)
> Did you also change the license?

Yes, I goofed... :-(

I'll fix it, glad you noticed.

Thanks for the review.


Comment 8 Christian Iseli 2008-06-10 07:40:59 UTC
New Package CVS Request
=======================
Package Name: staden-io_lib
Short Description: General purpose library to handle gene sequencing machine
trace files
Owners: c4chris
Branches: F-8 F-9
InitialCC: 
Cvsextras Commits: yes

Comment 9 Kevin Fenzi 2008-06-10 16:39:40 UTC
cvs done.

Comment 10 Mamoru TASAKA 2008-06-12 13:29:06 UTC
Just a question:

Why does this package need to call configure twice?
(#%configure does not suppress configure call as:
 http://koji.fedoraproject.org/koji/getfile?taskID=659093&name=build.log )

Comment 11 Christian Iseli 2008-06-12 13:51:33 UTC
Oh dear, I forgot about the RPM macro expansion weirdness again...
Thanks for catching this.


Comment 12 Christian Iseli 2008-06-12 14:23:18 UTC
Imported.
Built in F-8.
Can't build in F-9 and devel yet, due to BZ #450889 getting gcc ICE on ppc machines.

Leaving open, waiting to see what happens with gcc on ppc.


Comment 13 Jason Tibbitts 2008-06-12 15:07:28 UTC
Somehow I missed the extra configure run; thanks to Mamoru for spotting it.  If
I see Ville around I'll ask him how hard it would be to detect
commented-but-not-really macros in rpmlint.

Comment 14 Christian Iseli 2008-06-12 15:55:43 UTC
yeah, that would be good to have in rpmlint

Comment 15 Jason Tibbitts 2008-08-08 16:34:25 UTC
I think the issue that was holding this up has been fixed; can this ticket be closed now?

Comment 16 Mamoru TASAKA 2008-08-08 16:41:54 UTC
Well, as I as checked,
* this package has already been rebuilt on devel, F-8, but not on F-9
* No push requests are submitted on bodhi.

So would you update the status,  Christian?

Comment 17 Christian Iseli 2008-08-10 11:21:09 UTC
(In reply to comment #15)
> I think the issue that was holding this up has been fixed; can this ticket be
> closed now?

Building still fails on PPC due to the GCC internal error on F-9...
Are you sure a new GCC has been pushed ?

Comment 18 Jason Tibbitts 2008-08-10 11:34:35 UTC
I do not know if Jakub intends to fix the issue on F-9; I pinged on bug 450889.

Comment 19 Christian Iseli 2008-08-10 11:44:54 UTC
(In reply to comment #18)
> I do not know if Jakub intends to fix the issue on F-9; I pinged on bug 450889.

Thanks tibbs.

I requested a push of staden-io_lib for F-8.  If GCC won't be fixed in F-9 I'll just close this ticket, but let's wait a bit to see what are Jakub's intentions.

Comment 20 Mamoru TASAKA 2008-08-10 11:52:14 UTC
Well, by the time gcc issue is fixed on F-9, I propose a workaround like:

%build
#%% configure --disable-static
%configure --enable-shared
%if 0%{?fedora} == 9
# First build source as much as possible
# Next downgrade optimization level due to gcc ICE (bug 450889)
make -k %{?_smp_mflags} || :
make %{?_smp_mflags} CFLAGS="`echo %optflags | sed -e 's|-O2|-O|'`"
%else
make %{?_smp_mflags}
%endif

Comment 21 Christian Iseli 2008-08-10 12:27:05 UTC
(In reply to comment #20)
> Well, by the time gcc issue is fixed on F-9, I propose a workaround like:
> ...

Oh, ok.  I applied your workaround and the build went fine.
Good idea, thanks.

I'll close this ticket now.


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