Bug 490723 - Review Request: R-IRanges - Low-level containers for storing sets of integer ranges
Summary: Review Request: R-IRanges - Low-level containers for storing sets of integer ...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mattias Ellert
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 490724
Blocks: 490721
TreeView+ depends on / blocked
 
Reported: 2009-03-17 18:17 UTC by Pierre-YvesChibon
Modified: 2009-04-09 16:15 UTC (History)
2 users (show)

Fixed In Version: 1.1.55-1.fc9
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-09 16:11:31 UTC
Type: ---
Embargoed:
mattias.ellert: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Review result (3.69 KB, text/plain)
2009-03-22 09:47 UTC, Mattias Ellert
no flags Details

Description Pierre-YvesChibon 2009-03-17 18:17:27 UTC
Spec URL: http://pingou.fedorapeople.org/RPMs/R-IRanges.spec
SRPM URL: http://pingou.fedorapeople.org/RPMs/R-IRanges-1.0.14-1.fc10.src.rpm
Description: 
The IRanges class and its extensions are low-level containers
for storing sets of integer ranges. A typical use of these containers
in biology is for representing a set of chromosome regions.
More specific extensions of the IRanges class will typically
allow the storage of additional information attached to each
chromosome region as well as a hierarchical relationship between
these regions.

Comment 1 Mattias Ellert 2009-03-21 12:22:00 UTC
Package does not build for me - missing BuildRequires?

** R
** inst
** preparing package for lazy loading
Error in loadNamespace(i[[1]], c(lib.loc, .libPaths())) :
  there is no package called 'Matrix'
Calls: <Anonymous> ... namespaceImportFrom -> asNamespace -> loadNamespace
Execution halted
ERROR: lazy loading failed for package 'IRanges'
** Removing '/builddir/build/BUILDROOT/R-IRanges-1.0.14-1.fc10.x86_64/usr/lib64\
/R/library/IRanges'
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.9f4Ede (%install)

Comment 2 Pierre-YvesChibon 2009-03-21 16:49:21 UTC
Indeed R-Matrix is needed for build, I will update the spec in that way.

Comment 4 Mattias Ellert 2009-03-22 09:47:38 UTC
Created attachment 336194 [details]
Review result

I attach my review. In summary it looks quite good.

Comment 5 Pierre-YvesChibon 2009-03-22 10:05:24 UTC
> ! The package license corresponds to the license as mentioned in the
>  DESCRIPTION file and in the website, however several source files
>  contain the statement:
> 
> * This file is copyright 2002 Jim Kent, but license is hereby
> * granted for all use - public, private or commercial. */
> 
>  which is not the same as "Artistic licence 2.0". Needs some
>  consultation with upstream.

I will ask upstream for clarification

> ! Specfile is written in legible English and uses macro consitently,
>  however:
> 
>  The Source and URL fields uses the macro %{BioC} with is not defined
>  anywhere. The urls point to the right location if this macro is
>  assumed to be an empty string, so techncally they are correct, but
>  it is a source of confusion. Suggestion - remove the macros.

The macro is defined (line 2 of the file), however the %define should be changed to %global according to the newly approved guidelines.

>  The comment that says "#i368 arch" should probaly read something
>  like "#architecture dependent package", because that I think is what
>  you really mean.

That comment is generated by R2spec, it can be changed/ignored/removed
(I will change R2spec to this)

> ! Package compiles, but there are warnings that should be fixed:
> 
> IntervalTree.c:48: warning: missing braces around initializer
> memalloc.c:293: warning: format '%d' expects type 'int', but argument 2 has type 'size_t'
> memalloc.c:293: warning: format '%d' expects type 'int', but argument 3 has type 'size_t'
> 
>  The "... may be used uninitialized" warnings might be harmless (or
>  not, ...)

> I have no idea if these warnings are harmful or not:
> 
> Creating a generic for "cbind" in package  "IRanges"
>    (the supplied definition differs from and overrides the implicit generic in package "base": Signatures differ:  (...), (deparse.level))
> Creating a generic for "rbind" in package  "IRanges"
>    (the supplied definition differs from and overrides the implicit generic in package "base": Signatures differ:  (...), (deparse.level))

I will point these warning to upstream.

> ! %check is present and check pass, but
> 
>  Comparing 'runalltests.Rout' to 'runalltests.Rout.save' ...11,12c11
> < 	 rbind,
> < 	 sapply 
> ---
> > 	 rbind 
> 
>  It test still says "OK", but it looks "strange".

This is just the output of the test run

> ! No package owns the package's main directory /usr/lib64/R/library/IRanges

Oups, I will change this

>  Ideally both main and devel should own the main directory in order
>  to avoid orphaned directories after package removal, but at least
>  main must own it.

I do not agree. Since -devel has the main package has a requirement, only the main need to own the directory.

> ! Since the -devel package does not include a .pc file it should not
>  require pkgconfig (or is there some other reason for this?)

Will be fixed

Comment 6 Pierre-YvesChibon 2009-03-22 10:28:27 UTC
* Sun Mar 22 2009 pingou <pingou -AT- pingoured.fr> 1.0.14-3
- The main package owns the directory
- Remove pkgconfig as R for the devel package
- Define becomes global

SPEC:
http://pingou.fedorapeople.org/RPMs/R-IRanges.spec
SRPM:
http://pingou.fedorapeople.org/RPMs/R-IRanges-1.0.14-3.fc10.src.rpm

Comment 7 Pierre-YvesChibon 2009-03-22 17:20:23 UTC
I got a reply to the mail I sent:
-----------------------------------
> We also see some warnings while compiling it:
>  IntervalTree.c:48: warning: missing braces around initializer
>  memalloc.c:293: warning: format '%d' expects type 'int', but argument 2
> has type 'size_t'
>  memalloc.c:293: warning: format '%d' expects type 'int', but argument 3
> has type 'size_t'
>

These are probably harmless but easy to rectify.


>
>  Creating a generic for "cbind" in package  "IRanges"
>    (the supplied definition differs from and overrides the implicit generic
> in package "base": Signatures differ:  (...), (deparse.level))
>  Creating a generic for "rbind" in package  "IRanges"
>    (the supplied definition differs from and overrides the implicit generic
> in package "base": Signatures differ:  (...), (deparse.level))
>

These are intentional and may be ignored.
-------------------------------------

Nothing regarding the license yet though

Comment 8 Pierre-YvesChibon 2009-03-25 07:04:54 UTC
Answer concerning the license:

> We will add a new field to the DESCRIPTION file, like ExtraLicenses or
> something that will indicate the C files that are under freely licensed for
> all uses by Jim Kent, in a way that is compatible with the Artistic license.

Comment 10 Mattias Ellert 2009-04-03 07:54:18 UTC
Package approved.

Comment 11 Pierre-YvesChibon 2009-04-03 08:04:58 UTC
Thanks for the review :)

New Package CVS Request
=======================
Package Name: R-IRanges
Short Description: Low-level containers for storing set of integer ranges
Owners: pingou
Branches: F-9 F-10
InitialCC:

Comment 12 Kevin Fenzi 2009-04-03 20:33:29 UTC
cvs done.

Comment 13 Fedora Update System 2009-04-07 13:22:41 UTC
R-IRanges-1.1.55-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/R-IRanges-1.1.55-1.fc10

Comment 14 Fedora Update System 2009-04-07 13:22:47 UTC
R-IRanges-1.1.55-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/R-IRanges-1.1.55-1.fc9

Comment 15 Fedora Update System 2009-04-09 16:11:25 UTC
R-IRanges-1.1.55-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2009-04-09 16:15:29 UTC
R-IRanges-1.1.55-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


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