Bug 241052 - Review Request: R-Matrix - R module, Classes etc. for dense and sparse matrices and matrix ops
Review Request: R-Matrix - R module, Classes etc. for dense and sparse matric...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-23 15:10 EDT by Tom Moertel
Modified: 2008-02-12 09:34 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-12 09:34:42 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Tom Moertel 2007-05-23 15:10:19 EDT
Spec URL: http://community.moertel.com/rpms/fedora/6/SPECS/R-Matrix.spec
SRPM URL: http://community.moertel.com/rpms/fedora/6/SRPMS/R-Matrix-0.9975-1.src.rpm
Description:

I have packaged a number of CRAN packages for the R statistical-computing system.  See the following for the full list:

http://blog.moertel.com/articles/2007/04/25/new-fedora-core-rpms-for-cran-packages

After discussions about the packages on the fedora-r-devel-list, I am submitting them for review, starting with R-Matrix.  For background, please see:

https://www.redhat.com/archives/fedora-r-devel-list/2007-April/msg00003.html

If there are any problems with this package's spec, they are likely to exist in my other packages as well, so I would like to flush them out with an initial submission before submitting the rest.

Please let me know if there is anything I can do to improve my packaging technique.

Cheers,
Tom
Comment 1 Tom Moertel 2007-05-23 15:28:16 EDT
Please note that this is my first package submission, and I am seeking a sponsor.
Comment 2 Tom "spot" Callaway 2007-05-23 15:30:29 EDT
Couple of minor items:

You probably want to have: BuildRequires: libgfortran-devel (you're linking to it)

The BuildRequires: R is correct, but BuildRequires: R-devel may be needed for
other CRAN packages (if they're looking for libR.pc), and it will drag in R as a
dependency. Might not be a bad idea to script it that way to be safe.

You need to mkdir -p %{buildroot}%{_libdir}/R/library before running R CMD
INSTALL into it. :)
Comment 3 Tom "spot" Callaway 2007-05-23 15:34:38 EDT
Also:

[spot@localhost ~]$ rpmlint
/home/spot/rpmbuild/RPMS/x86_64/R-Matrix-0.9975-1.fc7.x86_64.rpm
/home/spot/rpmbuild/SRPMS/R-Matrix-0.9975-1.fc7.src.rpm
/home/spot/rpmbuild/RPMS/x86_64/R-Matrix-debuginfo-0.9975-1.fc7.x86_64.rpm
W: R-Matrix devel-file-in-non-devel-package
/usr/lib64/R/library/Matrix/include/Matrix.h
W: R-Matrix devel-file-in-non-devel-package
/usr/lib64/R/library/Matrix/include/Matrix_stubs.c
W: R-Matrix devel-file-in-non-devel-package
/usr/lib64/R/library/Matrix/include/cholmod.h
W: R-Matrix incoherent-version-in-changelog 1.0-1 0.9975-1.fc7

The devel files look like they belong in a -devel subpackage. The changelog
entry needs to be corrected, your script seems to be spitting out a generic one
that doesn't match the package version (and is omitting your name).
Comment 4 Patrice Dumas 2007-05-23 16:24:49 EDT
(In reply to comment #2)
> Couple of minor items:
> 
> You probably want to have: BuildRequires: libgfortran-devel (you're linking to it)

libgfortran-devel doesn't exist as such, it is linked automatically 
in when linking with gcc -lgfortran/gfortran. A

BuildRequires: gcc-gfortran

is missing however. 

Also looking at the log, it seems that during the link gcc
is used and there is a -lgfortran. In my recalling this is not 
what should be done, instead gfortran should be used during the
link and -lgfortran should be omitted. This may be for upstream, or
a R bug, however, I don't know exactly.
Comment 5 Tom Moertel 2007-05-23 17:26:00 EDT
Tom "spot" Callaway: Is the preferred way of placing devel files in a -devel
subpackage documented anywhere?  (I didn't see anything on the topic in the
Packaging Guidelines.)  If there is a preferred way of doing it, I'd rather not
guess what it is.  :)

Thanks for your help.
Comment 6 Tom Moertel 2007-05-24 02:01:25 EDT
I have revised the spec at the aforelinked-to location to fix the problems noted
earlier.

Note that the -devel package is built for the same arch as the main pacakge, but
it contains only .c and .h files, so rpmlint complains that it should be built
as noarch.
Comment 7 Tom Moertel 2007-05-24 02:09:57 EDT
Note: I also updated the package to the recently released upstream version,
0.99875-1.  The corresponding SRPM can be downloaded here:

http://community.moertel.com/rpms/fedora/6/SRPMS/R-Matrix-0.99875-1.src.rpm
Comment 8 Tom "spot" Callaway 2007-05-25 12:32:17 EDT
There is a very minor issue with this package, but it doesn't hold up approval.

- rpmlint says you've got mixed spaces and tabs, look at line 29. 

Fix that item before requesting builds, and we're good. Nice work with the
-devel package.

Good:

- rpmlint checks return:

W: R-Matrix mixed-use-of-spaces-and-tabs (spaces: line 29, tab: line 5)
W: R-Matrix-devel no-documentation
E: R-Matrix-devel only-non-binary-in-usr-lib

Safe to ignore the Error (all R packages do that), and the no-docs in -devel.

- package meets naming guidelines
- package meets packaging guidelines
- license (Distributable) OK, text in package, matches source (code is a mix of
MIT, GPL, LGPL, documented in spec)
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files 

APPROVED. I'll also sponsor you, thanks for your good work.
Comment 9 Jason Tibbitts 2008-01-20 17:31:24 EST
Did anything ever happen here?  The package is approved and sponsorship was
offered eight months ago, yet I don't see Tom in the cvsextras group and of
course the package is not in the distro.
Comment 10 Tom Moertel 2008-01-22 07:41:45 EST
Jason, thanks for the ping.

I suspect that momentum on this packaging attempt got lost during the Extras
merge, which occurred around the time of the submission.

I'd like to resume the process.  What's the best way to restart it?

Cheers,
Tom
Comment 11 Jason Tibbitts 2008-01-22 13:19:39 EST
Since it's been quite some time, I'd go ahead and check the package against the
latest R packaging guidelines at http://fedoraproject.org/wiki/Packaging/R, then
put links to the spec and srpm here, and then spot can give it another look. 
hopefully spot's still listening and still willing to sponsor you.
Comment 12 Tom "spot" Callaway 2008-01-22 20:55:48 EST
Yeah, please make sure the spec meets the R packaging guidelines, then post it
here (if any changes need to be made).

I'll give it another quick pass, and then sponsor you upon completion. :)
Comment 13 Tom Moertel 2008-01-26 12:11:46 EST
Great!  I tweaked the spec per the most-recent R-packaging guidelines and also
updated to Matrix version 0.999375-4.  New spec and SRPM:

http://community.moertel.com/rpms/fedora/8/SPECS/R-Matrix.spec
http://community.moertel.com/rpms/fedora/8/SRPMS/R-Matrix-0.999375-1.fc8.src.rpm

Please let me know if you need any changes.
Comment 14 Tom Moertel 2008-01-30 19:36:30 EST
Just checking in:  Do I need to do anything else before review?  (I've linked to
the spec and srpm in the previous comment.)

Cheers,
Tom
Comment 15 Tom "spot" Callaway 2008-01-31 14:31:43 EST
Only items I see:

* Your changelog says you're at -4, but you forgot to increment release to 4.
Just make sure that your %changelog matches. :)

Good:

- rpmlint checks return:
R-Matrix.x86_64: W: incoherent-version-in-changelog 0.999375-4 0.999375-1.fc9
R-Matrix.x86_64: W: one-line-command-in-%post /usr/lib/rpm/R-make-search-index.sh
R-Matrix.x86_64: W: one-line-command-in-%postun /usr/lib/rpm/R-make-search-index.sh
R-Matrix-devel.x86_64: W: no-documentation
No blockers there, just be careful with matching n-v-r and changelog.

- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2+) OK, text in %doc, matches source (explanation in spec)
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 
- devel package ok
- no .la files
- devel requires base package n-v-r 

APPROVED (just fix the release before committing).

Get your account here, and follow the rest of the steps:
http://fedoraproject.org/wiki/PackageMaintainers/Join#head-a601c13b0950a89568deafa65f505b4b58ee869b
Comment 16 Tom Moertel 2008-01-31 20:20:14 EST
Thanks for the re-review.  I'll fix the changelog.

I already have an account.  I just need cvsextras approval to move forward.   :-)

Note: This is the point where things got bogged down last time, so maybe
something is wrong with the state of my account ("tmoertel").  Please let me
know if you find any problems.

Cheers,
Tom
Comment 17 Tom Moertel 2008-02-06 22:55:59 EST
Just a quick check-in ping.  :-)

Can we move the process forward somehow?  (Are you waiting on something from me?)

Cheers,
Tom
Comment 18 Tom "spot" Callaway 2008-02-06 23:03:56 EST
Sorry, I never saw the sponsor request come through. You're sponsored now, and
have cvsextras approval. Follow the next steps on the Join link I pasted above.
Comment 19 Tom Moertel 2008-02-11 20:34:29 EST
New Package CVS Request
=======================
Package Name: R-Matrix
Short Description: R module, Classes etc. for dense and sparse matrices and
matrix ops
Owners: tmoertel
Branches: F-7 F-8
InitialCC:
Cvsextras Commits: yes
Comment 20 Kevin Fenzi 2008-02-11 21:32:44 EST
cvs done.
Removing NEEDSPONSOR.

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