Bug 498846 - Review Request: R-RM2 - Revenue Management and Pricing for R
Review Request: R-RM2 - Revenue Management and Pricing for R
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
Depends On: 498845
  Show dependency treegraph
Reported: 2009-05-03 19:14 EDT by Denis Arnaud
Modified: 2009-09-26 15:37 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.0-4.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2009-07-16 03:14:37 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+

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

  None (edit)
Description Denis Arnaud 2009-05-03 19:14:52 EDT
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-23 21:48:51 EDT
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 09:59:02 EDT
Created attachment 349240 [details]
R-mvtnorm spec file
Comment 3 R P Herrold 2009-06-24 09:59:59 EDT
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 10:00:43 EDT
Created attachment 349243 [details]
R-RM2 spec
Comment 5 R P Herrold 2009-06-24 10:01:37 EDT
Created attachment 349244 [details]
R-msm spec
Comment 6 Denis Arnaud 2009-06-24 12:00:55 EDT
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 16:10:21 EDT
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-05 23:16:37 EDT
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 15:51:28 EDT
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-06 22:08:26 EDT
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 01:35:06 EDT
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 01:43:02 EDT
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 12:23:31 EDT
CVS done.
Comment 14 Fedora Update System 2009-07-11 19:37:49 EDT
R-RM2-0.0-4.fc11 has been submitted as an update for Fedora 11.
Comment 15 Fedora Update System 2009-07-16 03:14:31 EDT
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 11:15:34 EDT
Package Change Request
Package Name: R-RM2
New Branches: F-12
Owners: denisarnaud
Comment 17 Kevin Fenzi 2009-09-26 15:37:02 EDT
cvs done.

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