Bug 470066 (R-qtl) - Review Request: R-qtl - Quantitative trait loci (qtl) functionality for R
Summary: Review Request: R-qtl - Quantitative trait loci (qtl) functionality for R
Keywords:
Status: CLOSED NEXTRELEASE
Alias: R-qtl
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Pierre-YvesChibon
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-05 15:50 UTC by Mattias Ellert
Modified: 2009-03-19 07:08 UTC (History)
5 users (show)

Fixed In Version: 1.10-2.fc10
Clone Of:
Environment:
Last Closed: 2009-03-18 19:11:07 UTC
Type: ---
Embargoed:
pingou: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Mattias Ellert 2008-11-05 15:50:57 UTC
Spec URL: http://www3.tsl.uu.se/~ellert/R-qtl/R-qtl.spec
SRPM URL: http://www3.tsl.uu.se/~ellert/R-qtl/R-qtl-1.09-1.fc9.src.rpm
Description: 
Analysis of experimental crosses to identify genes (called quantitative
trait loci, QTLs) contributing to variation in quantitative traits.

Specfile based on template in https://fedoraproject.org/wiki/Packaging/R

Comment 1 Jason Tibbitts 2008-11-05 15:56:46 UTC
Do you already have a sponsor?  I see that you've applied for membership in the packager group but it hasn't been approved.

Comment 2 Mattias Ellert 2008-11-05 21:11:48 UTC
I have a few other packages that I have submitted that are actively being reviewed, but not any approved package yet. I did add the NEED SPONSOR tag to the first package I submitted. As far as I have understood I can not become a member of the packager group until my first package is approved.

It might well be that this package will be the one approved first, since it is less complicated than the others, has no patches applied, and very closely follows the template in the guidelines.

Comment 3 Mattias Ellert 2008-11-09 09:36:10 UTC
The sysadmins have closed the www3.tsl.uu.se server and moved all pages to the www5.tsl.uu.se server, without putting an alias in DNS. I am arguing with them to put an alias in, but I don't know if I will succeed. So (at least for now) use these URLs instead of the once above:

Spec URL: http://www5.tsl.uu.se/~ellert/R-qtl/R-qtl.spec
SRPM URL: http://www5.tsl.uu.se/~ellert/R-qtl/R-qtl-1.09-1.fc9.src.rpm

Comment 4 Alec Leamas 2008-11-19 11:24:52 UTC
(Hej!)

I need a sponsor, so I need to make some informal reviews, see below, Please feel free to do the same for me, by request is bug 471575 (if it looks strange to you, it's nothing compared to what this stuff looks for me ;-)

Summary: OK besides the license, which should be GPLv2


MUST stuff:
rpmlint must be run on every package...
  - No errors or warnings on srpm or spec file.

The package must be named according to the  Package Naming Guidelines .
   - OK (see section on R-modules in the Naming Gauidelines).

The spec file name must match the base package %{name}, 
   - OK

The package must meet the  Packaging Guidelines .
   - OK besides licensing, see below. (follows closely the specific 
     R guidelines)
  
The package must be licensed with a Fedora approved license...
   - NOK, see below

The License field in the package spec file must match the actual license.
   - The License tag is set to GPLv2+, but the actual source license
     is GPLv2. This is true both for the LICENSE file and at least 
     some copyright notices.

The text of the license(s) for the package must be included in %doc
   - OK

The spec file for the package MUST be legible.
   - OK

The sources used to build the package must match the upstream source
   - OK (b62289d268a09b72c5e804f35df53a67)

The package must successfully compile and build into binary rpms.
   - OK (Mock test, Fedora-9/X86_64)

All build dependencies must be listed in BuildRequires
   - OK (since mock is OK)

The spec file MUST handle locales properly.
   - NA

Every binary RPM package which stores shared library files 
   - NA

If the package is designed to be relocatable...
   - NA

A package must own all directories that it creates
   - OK

A package must not contain any duplicate files in the %files listing.
   - OK

Permissions on files must be set properly
   - OK

Each package must have a %clean section, rm -rf %{buildroot} 
   - OK

Each package must consistently use macros...
   - OK

The package must contain code, or permissable content.
   - OK

Large documentation files should go in a -doc subpackage
   - OK (According to the specific R Guidelines, otherwise a separate
         -doc subpackage would definitely be on the agenda)

If it is in %doc, the program must run properly if it is not present
   - OK (Specific R Guidelines example).

Header files must be in a -devel package.
   - NA

Static libraries must be in a -static package.
   - NA

Packages containing pkgconfig(.pc) files must...
   - NA

If a package contains library files with a suffix (e.g. libfoo.so.1.1
   - NA

If a package contains library files with a suffix
   - NA

devel packages must require the base package using...
   - NA

Packages must NOT contain any .la libtool archives
   - NA (no autoconf!)

Packages containing GUI applications...
   - NA

Packages must not own files or directories already owned by other packages
   - OK 

At the beginning of %install, each package MUST run rm -rf %{buildroot}
   - OK

All filenames in rpm packages must be valid UTF-8.
   - OK

SHOULD 

   - The upstream license file (GPLv2) is present.
   - Localized descriptions are not applicable.
   - Build OK in mock, se above.
   - Built also on my "normal" Fedora 9 x86_64 box.
   - Scriptlets are "sane"
   - There are no subpackages, pkgconfig  .pc file or file deps.

Comment 5 Mattias Ellert 2008-12-02 13:06:15 UTC
(In reply to comment #4)

Thank you for your review, I will comment on your objections below.

> The package must be licensed with a Fedora approved license...
>    - NOK, see below

Well, GPLv2+ is a Fedora approved license, so I see no violation on this point. I guess your objection is not about that the the tag I used is not approved, but that it is the wrong one. Which is really your next point. So I will comment further below.

> The License field in the package spec file must match the actual license.
>    - The License tag is set to GPLv2+, but the actual source license
>      is GPLv2. This is true both for the LICENSE file and at least 
>      some copyright notices.

I based the tag I used on the text in the included LICENSE.txt file which says:

"This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or (at
your option) any later version."

I.e. this is GPLv2+. But as you pointed out all the copyright notices in the source file comments say:

"Licensed under the GNU General Public License version 2 (June, 1991)"

So there is an inconsistency between the LICENSE.txt file and the source file comments. Since the source file comments are more restrictive (GPLv2) than the LICENSE.txt file (GPLv2+) I have changed the label in the spec file to GPLv2. New versions are available here:

Spec URL: http://www3.tsl.uu.se/~ellert/R-qtl/R-qtl.spec
SRPM URL: http://www3.tsl.uu.se/~ellert/R-qtl/R-qtl-1.09-2.fc9.src.rpm

Comment 6 Pierre-YvesChibon 2009-02-19 14:10:17 UTC
You could also try to ask upstream to clarify the situation regarding the license of this package.

Comment 7 Pierre-YvesChibon 2009-02-19 14:19:35 UTC
BTW the url tag is incorrect:
http://cran.r-project.org/web/packages/qtl/index.html

This page shows again a GPLv2+ license, I think you should ask upstream :)

And there is also a new release of R/qtl

Comment 8 Mattias Ellert 2009-02-28 15:53:40 UTC
After contacting the upstream developers they sent me a new version of the code where they have fixed the inconsistent license information. I have created an updated package based on this version:

Spec URL: http://www3.tsl.uu.se/~ellert/R-qtl/R-qtl.spec
SRPM URL: http://www3.tsl.uu.se/~ellert/R-qtl/R-qtl-1.10-1.fc9.src.rpm

The source (qtl_1.10-28.tar.gz) differs from the version you can download from the website (qtl_1.10-27.tar.gz), but the only difference is the updated license information.

Since the license information now is consistent I have reverted the License tag in the spec file to be GPLv2+.

Comment 9 Pierre-YvesChibon 2009-03-17 18:22:31 UTC
Since I see you have been approved I will review this package

Comment 10 Pierre-YvesChibon 2009-03-17 19:00:22 UTC
Here is the review:
* You should change your %prefer to %global see : https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define (although it mentions draft it has been accepted).

X can't check upstream source.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
  -> Although I am wondering if we should not used the same than the one provided in R while doing "library()" ie: Tools for analyzing QTL experiments
* description is OK.
  -> You might though develop it a bit more by taking some descriptive sentence in http://www.rqtl.org/
* dist tag is present.
* build root is OK.
* license text included in package.
  -> the LICENSE file and some headers have been corrected in the git repo to GPLv2+
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in koji (rawhide).
-> http://koji.fedoraproject.org/koji/taskinfo?taskID=1246648
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent (barring the two expected R complaints)
* final provides and requires are sane
* %check is present and all tests pass.
* 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.
* scriptlets are OK (R package registration).
* code, not content.
* documentation is small, so no -doc 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.


I let you change %global to %define and the description and summary (if you feel like) before commiting to the cvs (but please do change %define).

------------------------------------------------------------------------
--   R-qtl is approved by Pierre-Yves Chibon ~pingou 
------------------------------------------------------------------------

Thanks for bringing a new R package into Fedora and you might be interested by http://rpms.famillecollet.com/rpmphp and http://rpms.famillecollet.com/rpmphp/rpm.php?type=R

Comment 11 Mattias Ellert 2009-03-17 21:40:04 UTC
New Package CVS Request
=======================
Package Name: R-qtl
Short Description: Tools for analyzing QTL experiments
Owners: ellert
Branches: F-9 F-10 EL-4 EL-5
InitialCC:

Comment 12 Kevin Fenzi 2009-03-18 03:17:41 UTC
cvs done.

Comment 13 Fedora Update System 2009-03-18 14:20:00 UTC
R-qtl-1.10-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/R-qtl-1.10-2.fc9

Comment 14 Fedora Update System 2009-03-18 14:20:06 UTC
R-qtl-1.10-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/R-qtl-1.10-2.fc10

Comment 15 Fedora Update System 2009-03-18 19:11:02 UTC
R-qtl-1.10-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2009-03-18 19:14:20 UTC
R-qtl-1.10-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Mattias Ellert 2009-03-19 07:08:57 UTC
(In reply to comment #10)

Thank you for the review. Your suggestions were implemented in the final version that was uploaded to CVS. With this morning's yum update I got the new package from the Fedora repo.


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