Bug 498846 - Review Request: R-RM2 - Revenue Management and Pricing for R
Summary: Review Request: R-RM2 - Revenue Management and Pricing for R
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
Depends On: 498845
TreeView+ depends on / blocked
Reported: 2009-05-03 23:14 UTC by Denis Arnaud
Modified: 2009-09-26 19:37 UTC (History)
4 users (show)

Fixed In Version: 0.0-4.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2009-07-16 07:14:37 UTC
Type: ---
tibbs: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)
R-mvtnorm spec file (1.98 KB, text/plain)
2009-06-24 13:59 UTC, R P Herrold
no flags Details
R-RM2 spec (1.93 KB, text/plain)
2009-06-24 14:00 UTC, R P Herrold
no flags Details
R-msm spec (2.36 KB, text/plain)
2009-06-24 14:01 UTC, R P Herrold
no flags Details

Description Denis Arnaud 2009-05-03 23:14:52 UTC
Spec URL: http://denisarnaud.fedorapeople.org/R/RM2/R-RM2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/R/RM2/R-RM2-0.0-1.fc10.src.rpm
[FE-NEEDSPONSOR] That is my fourth package for Fedora, but the second one for R,
and I thus still need a sponsor.
Description: RM2 is a simple package that implements functions used in revenue management and pricing environments.

Web site: http://cran.r-project.org/web/packages/RM2/

Additional notes:
 * I followed the R-specific guidelines
 * That package requires the R-msm package, which I submitted for review/approval separately (https://bugzilla.redhat.com/show_bug.cgi?id=498845)

Comment 1 Jason Tibbitts 2009-06-24 01:48:51 UTC
This fails to build for me in the %install section:

* Installing *source* package 'RM2' ...
** R
** preparing package for lazy loading
Error in loadNamespace(i[[1L]], c(lib.loc, .libPaths())) :
  there is no package called 'mvtnorm'
Error : package 'msm' could not be loaded
ERROR: lazy loading failed for package 'RM2'

Does this need a build dependency on R-mvtnorm?

Comment 2 R P Herrold 2009-06-24 13:59:02 UTC
Created attachment 349240 [details]
R-mvtnorm spec file

Comment 3 R P Herrold 2009-06-24 13:59:59 UTC
File attach bit me -- this should preceed my attachment

Many other packages need R-mvtnorm as well:
        R-mvtnorm is needed by (installed) R-pscl-1.03-1orc.01.x86_64
        R-mvtnorm is needed by (installed) R-sampleSelection-0.6.4-1orc.noarch
        R-mvtnorm is needed by (installed) R-vcd-1.2.4-1orc.01.noarch
        R-mvtnorm is needed by (installed) R-colorspace-1.0.1-1orc.10.x86_64

There is also a needed R and BR dependency on: R-msm to pass %check

The R2spec tool, in Fedora, produces a much more useful URL, and a nice set of pointers to BR's and R's --- I attach .spec's for R-msm, R-mvtnorm, and a revised one for RM2 made using it for comparison, which fedora may prefer to use

-- Russ herrold

Comment 4 R P Herrold 2009-06-24 14:00:43 UTC
Created attachment 349243 [details]
R-RM2 spec

Comment 5 R P Herrold 2009-06-24 14:01:37 UTC
Created attachment 349244 [details]
R-msm spec

Comment 6 Denis Arnaud 2009-06-24 16:00:55 UTC
Thanks for the update. I'll have a look at it this week-end.

That package (R-RM2) depends on R-msm, which is now available in Fedora (but was not at the time I started with R-RM2). Now that I gained experience on packaging R-msm, I'll retrofit it (as well as your feedback) into the R-RM2 packaging.

Comment 7 Denis Arnaud 2009-06-27 20:10:21 UTC
The dependency on R-msm (and R-mvtnorm) has been taken into account. The new versions are accessible at the following URLs:
Spec URL: http://denisarnaud.fedorapeople.org/R/RM2/2/R-RM2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/R/RM2/2/R-RM2-0.0-2.fc11.src.rpm

rpmlint just throws the classical warning (https://fedoraproject.org/wiki/Packaging/R#Generating_the_search_index.txt). So, the package seems fine for me.

Comment 8 Jason Tibbitts 2009-07-06 03:16:37 UTC
Indeed, this builds fine now and rpmlint has only the usual two complaints.

Please don't define a macro (%packrel) for your release; this makes it difficult for others (or the mass rebuild scripts) who may do maintenance on the package.  Just use
  Release: 2%{?dist}

The license for this package seems to be GPLv3+, not GPLv3.  Or did I miss somewhere where later versions of the GPL or not permitted?

* source files match upstream.  sha256sum:       
* 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.
X license field does not match the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
   R-RM2 = 0.0-2.fc12

* %check is present and all tests pass.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* 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.

The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.

Comment 9 Denis Arnaud 2009-07-06 19:51:28 UTC
Thanks for that review!

1. You are absolutely right about the license. The authors specify "GPL >= 3"; so, GPLv3+ must be used (instead of GPLv3).

2. As for the %{packrel} macro, as I now understand, it should be used for the release of the upstream R package (and not the RPM release).

The updated specification file and source RPM are to be found at the following URLs:
Spec URL: http://denisarnaud.fedorapeople.org/R/RM2/3/R-RM2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/R/RM2/3/R-RM2-0.0-3.fc11.src.rpm

As for the package review process, besides a few reviews, I watch almost all the C++-related packages, so as to try to help where there is no other reviewer assigned (it is not that much, but I own quite a few upstream open source projects, which I have to push if I ever want them to reach a critical mass). However, you are right, and I do fully understand that we are never enough to perform peer reviews :)

Comment 10 Jason Tibbitts 2009-07-07 02:08:26 UTC
Looks good; the license is fixed and Release: isn't obfuscated.  In general I'd suggest removing commented lines from the %files section and simply not defining unused macros like %packrel, but these are hardly blockers.


Comment 11 Denis Arnaud 2009-07-07 05:35:06 UTC
New Package CVS Request
Package Name: R-RM2
Short Description: Revenue Management and Pricing for R
Owners: denisarnaud
Branches: EL-4 EL-5 F-10 F-11

Comment 12 Denis Arnaud 2009-07-07 05:43:02 UTC
Taking into account comment #10, the updated specification file and source RPM are to be found at the following URLs:
Spec URL: http://denisarnaud.fedorapeople.org/R/RM2/4/R-RM2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/R/RM2/4/R-RM2-0.0-4.fc11.src.rpm

If nobody vetoes, that is the version I will import in CVS.

Comment 13 Jason Tibbitts 2009-07-08 16:23:31 UTC
CVS done.

Comment 14 Fedora Update System 2009-07-11 23:37:49 UTC
R-RM2-0.0-4.fc11 has been submitted as an update for Fedora 11.

Comment 15 Fedora Update System 2009-07-16 07:14:31 UTC
R-RM2-0.0-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Denis Arnaud 2009-09-26 15:15:34 UTC
Package Change Request
Package Name: R-RM2
New Branches: F-12
Owners: denisarnaud

Comment 17 Kevin Fenzi 2009-09-26 19:37:02 UTC
cvs done.

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