Bug 822896 - Review Request: pari-elldata - PARI/GP Computer Algebra System elliptic curves
Review Request: pari-elldata - PARI/GP Computer Algebra System elliptic curves
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paulo Andrade
Paulo Andrade
:
Depends On:
Blocks: 821191
  Show dependency treegraph
 
Reported: 2012-05-18 08:57 EDT by Paul Howarth
Modified: 2012-06-19 13:48 EDT (History)
4 users (show)

See Also:
Fixed In Version: pari-elldata-20120415-5.fc18
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-19 13:48:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
paulo.cesar.pereira.de.andrade: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paul Howarth 2012-05-18 08:57:39 EDT
Spec URL: http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/pari-elldata.spec
SRPM URL: http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-1.src.rpm

Description:
This package contains the optional PARI package elldata, which provides the
Elliptic Curve Database of J. E. Cremona Elliptic, which can be queried by
ellsearch and ellidentify.
Comment 1 Paulo Andrade 2012-05-20 13:01:22 EDT
I would like to review this package.

The first issues I noticed:

data/elldata/README is not %doc tagged.

I could not find information about how to verify the signed file.
But after some searching this should give some hints:

-%<-
$ gpg --verify SOURCES/elldata.tgz.asc SOURCES/elldata.tgz
# get key number

gpg --recv-keys --keyserver hkp://pgp.mit.edu/ key B5444815
gpg: "key" not a key ID: skipping
gpg: requesting key B5444815 from hkp server pgp.mit.edu
gpg: key B5444815: public key "Bill Allombert <ballombe@debian.org>" imported
gpg: no ultimately trusted keys found
gpg: Total number processed: 1
gpg:               imported: 1

$ gpg --verify SOURCES/elldata.tgz.asc SOURCES/elldata.tgz
gpg: Signature made Sun 15 Apr 2012 11:37:04 AM BRT using DSA key ID B5444815
gpg: Good signature from "Bill Allombert <ballombe@debian.org>"
gpg:                 aka "Bill Allombert (Lab. A2X) <allomber@math.u-bordeaux.fr>"
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 4940 AE28 C5F8 E8A3 5E4D  8D28 7833 ECF1 B544 4815
-%<-

README file also appears to be quite outdated, and only says the
data is under the terms of the GNU GPL, no license version
information.

The README file also says it was last updated 11/04/2012, so, I
think a better approach for version should be used. As
Version:  20120415
has some flaws; personal experience with non versioned texlive
packages... and suddenly a version being added to it, but those
are rare cases, so, just a suggestion, e.g. either use
<pari-version>.<date> or 0.<date>.
Comment 2 Paul Howarth 2012-05-21 14:02:58 EDT
(In reply to comment #1)
> I would like to review this package.
> 
> The first issues I noticed:
> 
> data/elldata/README is not %doc tagged.

Fixed in -2:

Spec URL: http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/pari-elldata.spec
SRPM URL: http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-2.src.rpm

> I could not find information about how to verify the signed file.
> But after some searching this should give some hints:
> 
> -%<-
> $ gpg --verify SOURCES/elldata.tgz.asc SOURCES/elldata.tgz
> # get key number
> 
> gpg --recv-keys --keyserver hkp://pgp.mit.edu/ key B5444815
> gpg: "key" not a key ID: skipping
> gpg: requesting key B5444815 from hkp server pgp.mit.edu
> gpg: key B5444815: public key "Bill Allombert <ballombe@debian.org>" imported
> gpg: no ultimately trusted keys found
> gpg: Total number processed: 1
> gpg:               imported: 1
> 
> $ gpg --verify SOURCES/elldata.tgz.asc SOURCES/elldata.tgz

You can actually shorten this to:
$ gpg --verify SOURCES/elldata.tgz.asc

> README file also appears to be quite outdated, and only says the
> data is under the terms of the GNU GPL, no license version
> information.

Not much we can do about outdated documentation unfortunately. I took the license information from the upstream "packages" page:

http://pari.math.u-bordeaux.fr/packages.html

which says:

  "All packages are © the PARI group, distributed under the terms of the GNU
   General Public License (GPL version 2 or any later version)"

> The README file also says it was last updated 11/04/2012, so, I
> think a better approach for version should be used. As
> Version:  20120415
> has some flaws; personal experience with non versioned texlive
> packages... and suddenly a version being added to it, but those
> are rare cases, so, just a suggestion, e.g. either use
> <pari-version>.<date> or 0.<date>.

Upstream lists the release date alongside each of the packages, so in the absence of anything better I think it's reasonable to use that for a version number. Clearly that will be a problem if a conventional versioning scheme is introduced (though I think that's unlikely) but there's always the possibility of an epoch bump to handle that eventuality. If you're dead against that, another possibility would be to set the package version number to zero and use the release date as the rpm release, which would handle an upgrade to any sane versioning scheme by upstream without requiring an epoch bump.
Comment 3 Paulo Andrade 2012-05-21 15:07:09 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > I would like to review this package.
> > 
> > The first issues I noticed:
> > 
> > data/elldata/README is not %doc tagged.
> 
> Fixed in -2:

  Ok.

> Spec URL:
> http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/
> pari-elldata.spec
> SRPM URL:
> http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-2.src.rpm

> > README file also appears to be quite outdated, and only says the
> > data is under the terms of the GNU GPL, no license version
> > information.
> 
> Not much we can do about outdated documentation unfortunately. I took the
> license information from the upstream "packages" page:
> 
> http://pari.math.u-bordeaux.fr/packages.html
> 
> which says:
> 
>   "All packages are © the PARI group, distributed under the terms of the GNU
>    General Public License (GPL version 2 or any later version)"

  For elldata it should be clear the source is GPL and the author
is very active in sagemath development at least, but the other
pari- packages may need further investigation. For example, there
is no license information about the original data at
http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/data/INDEX.html

> > The README file also says it was last updated 11/04/2012, so, I
> > think a better approach for version should be used. As
> > Version:  20120415
> > has some flaws; personal experience with non versioned texlive
> > packages... and suddenly a version being added to it, but those
> > are rare cases, so, just a suggestion, e.g. either use
> > <pari-version>.<date> or 0.<date>.
> 
> Upstream lists the release date alongside each of the packages, so in the
> absence of anything better I think it's reasonable to use that for a version
> number. Clearly that will be a problem if a conventional versioning scheme
> is introduced (though I think that's unlikely) but there's always the
> possibility of an epoch bump to handle that eventuality. If you're dead
> against that, another possibility would be to set the package version number
> to zero and use the release date as the rpm release, which would handle an
> upgrade to any sane versioning scheme by upstream without requiring an epoch
> bump.

  No problems with it, was just asking for more clarification, and
any change would just make the yyyymmdd format longer.

  Only issue I see now is that it should have a "Requires: pari-gp"
for proper resolution of dependencies.
Comment 4 Ralf Corsepius 2012-05-22 00:33:52 EDT
Some personal remarks:

- Shipping the *.asc sig seems pretty meaningless to me.

- I personally, would encourage you to use use %{?dist} in the %release tag.
Sure, it's not mandatory and not using it helps avoiding rebuilds, if a package's contents doesn't change between Fedora/RHEL releases, however it also makes packages vulnerable to changes in rpm and to the file system layout between releases (A CentOS5's xxx-*.rpm is something entirely different than a Fedora 18's xxx-*.rpm, even though it's NEVR are identical).

- I would recommend to use "install -m xxx" instead of "mkdir + cp" in %install.
This helps to avoid bad surprizes related to ownership/permissions/umasks.
Comment 5 Paul Howarth 2012-05-22 11:04:37 EDT
(In reply to comment #3)
>   For elldata it should be clear the source is GPL and the author
> is very active in sagemath development at least, but the other
> pari- packages may need further investigation. For example, there
> is no license information about the original data at
> http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/data/INDEX.html

I found something regarding the Echidna algorithms upon which the seadata file is based (http://echidna.maths.usyd.edu.au/kohel/alg/index.html):

  "This page hosts links to code and documentation in areas related to
   research in number theory and arithmetic geometry developed in
   association with research in this area, and made available under the
   GNU Public License version 2 or higher (see GPL) and the GNU Free
   Documentation License (see FDL), respectively"

Can't find any information about how the galdata file was produced but I don't see any reason not to trust that upstream is entitled to publish it under the terms of the GPL.

> Only issue I see now is that it should have a "Requires: pari-gp"
> for proper resolution of dependencies.

Can't do that as it would lead to a circular build dependency for pari itself, and it's also conceivable that other software could use the same data without requiring pari-gp. I did add a "Conflicts: pari-gp < 2.2.11" as that's the oldest version that can use this data, though perhaps that should be "pari" rather than "pari-gp"?

(In reply to comment #4)
> - Shipping the *.asc sig seems pretty meaningless to me.

I just see it being used as an extra check that the tarball hasn't changed, (if we keep the signature in git) given that upstream's releases aren't versioned. An aid to the package maintainer or anyone doing a downstream rebuild rather than anything else. 

> - I personally, would encourage you to use use %{?dist} in the %release tag.
> Sure, it's not mandatory and not using it helps avoiding rebuilds, if a
> package's contents doesn't change between Fedora/RHEL releases, however it
> also makes packages vulnerable to changes in rpm and to the file system
> layout between releases (A CentOS5's xxx-*.rpm is something entirely
> different than a Fedora 18's xxx-*.rpm, even though it's NEVR are identical).

Good point; I'd thought this package was a classic example of something that wouldn't need a dist tag and could be inherited between releases without being rebuilt but given the changes in rpm signature and compression formats over the last few years, the dist tag is still useful even when the package content is completely unchanged, so I've added on for the -3 release.

> - I would recommend to use "install -m xxx" instead of "mkdir + cp" in
> %install.
> This helps to avoid bad surprizes related to ownership/permissions/umasks.

It's a bit awkward to do "install" recursively so instead I added a %{_fixperms} after the copy to ensure that the built package's permissions were sane.
Comment 6 Ralf Corsepius 2012-05-22 11:52:37 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > - Shipping the *.asc sig seems pretty meaningless to me.
> 
> I just see it being used as an extra check that the tarball hasn't changed,
> (if we keep the signature in git) given that upstream's releases aren't
> versioned.
I guess you are aware, Fedora's git is automatically adding m5sums to "sources"?

This would apply if you intend to push the tarball to git's lookaside cache.

However, this would raise the next point, I don't know the answer to: Does Fedora's git lookaside cache allow unversioned tarballs, rsp. can the lookaside cache handle this?

> > - I would recommend to use "install -m xxx" instead of "mkdir + cp" in
> > %install.
> > This helps to avoid bad surprizes related to ownership/permissions/umasks.
> 
> It's a bit awkward to do "install" recursively so instead I added a
> %{_fixperms} after the copy to ensure that the built package's permissions
> were sane.
OK with me.
Comment 7 Paulo Andrade 2012-05-22 12:09:28 EDT
(In reply to comment #5)

> > Only issue I see now is that it should have a "Requires: pari-gp"
> > for proper resolution of dependencies.
> 
> Can't do that as it would lead to a circular build dependency for pari
> itself, and it's also conceivable that other software could use the same
> data without requiring pari-gp. I did add a "Conflicts: pari-gp < 2.2.11" as
> that's the oldest version that can use this data, though perhaps that should
> be "pari" rather than "pari-gp"?

  Do you mean these packages will be build requires of pari now? I
see they could be useful in a very complete %check.

  I think the conflict does not matter much, as pari-gp is a pari
subpackage.

  About data reuse, actually, sagemath ships elldata in a different
format (not sure if complete and optimized for size, but a lot smaller),
as well as it also creates a cremona_mini.db sqlite3 db during build.
Comment 8 Paul Howarth 2012-05-22 13:54:31 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > - Shipping the *.asc sig seems pretty meaningless to me.
> > 
> > I just see it being used as an extra check that the tarball hasn't changed,
> > (if we keep the signature in git) given that upstream's releases aren't
> > versioned.
> I guess you are aware, Fedora's git is automatically adding m5sums to
> "sources"?

True, but I guess the signature is a bit more obvious.

> This would apply if you intend to push the tarball to git's lookaside cache.
> 
> However, this would raise the next point, I don't know the answer to: Does
> Fedora's git lookaside cache allow unversioned tarballs, rsp. can the
> lookaside cache handle this?

Yes: the URL to a tarball in the lookaside cache is:

http://pkgs.fedoraproject.org/repo/pkgs/<PACKAGE>/<TARBALL>/<MD5SUM>/<TARBALL>

So tarballs with the same name for the same package can co-exist as long as their md5sums are different, which is a pretty safe bet.

(In reply to comment #7)
> (In reply to comment #5)
> 
> > > Only issue I see now is that it should have a "Requires: pari-gp"
> > > for proper resolution of dependencies.
> > 
> > Can't do that as it would lead to a circular build dependency for pari
> > itself, and it's also conceivable that other software could use the same
> > data without requiring pari-gp. I did add a "Conflicts: pari-gp < 2.2.11" as
> > that's the oldest version that can use this data, though perhaps that should
> > be "pari" rather than "pari-gp"?
> 
>   Do you mean these packages will be build requires of pari now? I
> see they could be useful in a very complete %check.

Yes, that's the intention - see https://bugzilla.redhat.com/show_bug.cgi?id=821191#c10

>   I think the conflict does not matter much, as pari-gp is a pari
> subpackage.

Don't know what you mean by that. I was wondering whether it was pari-gp that used the data packages, or the underlying library "pari" - the conflict should refer to the part of pari that actually uses the data.

>   About data reuse, actually, sagemath ships elldata in a different
> format (not sure if complete and optimized for size, but a lot smaller),
> as well as it also creates a cremona_mini.db sqlite3 db during build.

But that's derived from this package though, isn't it?
Comment 9 Paulo Andrade 2012-05-22 15:20:43 EDT
(In reply to comment #8)

> >   Do you mean these packages will be build requires of pari now? I
> > see they could be useful in a very complete %check.
> 
> Yes, that's the intention - see
> https://bugzilla.redhat.com/show_bug.cgi?id=821191#c10
> 
> >   I think the conflict does not matter much, as pari-gp is a pari
> > subpackage.
> 
> Don't know what you mean by that. I was wondering whether it was pari-gp
> that used the data packages, or the underlying library "pari" - the conflict
> should refer to the part of pari that actually uses the data.

  pari-gp should require the matching pari version. The code to handle it
is actually in pari library, but the most common way to access it is from
the gp command line.

> >   About data reuse, actually, sagemath ships elldata in a different
> > format (not sure if complete and optimized for size, but a lot smaller),
> > as well as it also creates a cremona_mini.db sqlite3 db during build.
> 
> But that's derived from this package though, isn't it?

  Well, in sagemath it is maintained by the data author, but contents
should be static, only how it is presented changes. In the case of
pari-elldata, it is in a format that pari understands.
Comment 10 Paul Howarth 2012-05-23 08:41:13 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > >   I think the conflict does not matter much, as pari-gp is a pari
> > > subpackage.
> > 
> > Don't know what you mean by that. I was wondering whether it was pari-gp
> > that used the data packages, or the underlying library "pari" - the conflict
> > should refer to the part of pari that actually uses the data.
> 
>   pari-gp should require the matching pari version.

It does.

> The code to handle it
> is actually in pari library, but the most common way to access it is from
> the gp command line.

OK, in view of that I've changed the conflict from pari-gp to pari in -4; given that pari-gp requires the same EVR of pari, that means that it shouldn't be possible to install a too-old version of either pari or pari-gp with this data package.

Spec URL: http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/pari-elldata.spec
SRPM URL: http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-4.src.rpm

I think that resolves all outstanding issues with this package thus far.
Comment 11 Ralf Corsepius 2012-05-26 00:44:55 EDT
Paul, I have been repeatedly reading over the rationale for this "Conflicts:", but still fail to understand it.

Could you try to elaborate/summarize it? Which problem is this trying to resolve?

Without this "Conflicts:", I would likely have approved these packages ;)
Comment 12 Paul Howarth 2012-05-26 05:20:55 EDT
(In reply to comment #11)
> Paul, I have been repeatedly reading over the rationale for this
> "Conflicts:", but still fail to understand it.
> 
> Could you try to elaborate/summarize it? Which problem is this trying to
> resolve?
> 
> Without this "Conflicts:", I would likely have approved these packages ;)

If you look at the upstream site:
http://pari.math.u-bordeaux.fr/packages.html

it says that the elldata package requires "PARI/GP 2.2.11 and up", or in other words, it's not compatible with earlier versions of pari. I can't add a "Requires:" for this version as that would create a circular build dependency (I want to be able to use this data package in pari's %check), so the conflict seems to be the best way of expressing this relationship.
Comment 13 Ralf Corsepius 2012-05-28 00:57:18 EDT
(In reply to comment #12)
> (In reply to comment #11)
> > Paul, I have been repeatedly reading over the rationale for this
> > "Conflicts:", but still fail to understand it.
> > 
> > Could you try to elaborate/summarize it? Which problem is this trying to
> > resolve?
> > 
> > Without this "Conflicts:", I would likely have approved these packages ;)

> it says that the elldata package requires "PARI/GP 2.2.11 and up", or in
> other words, it's not compatible with earlier versions of pari.
Understood, but AFAIS, these are mere data packages, right? 

Do they disturb older releases of pari?

> I can't add
> a "Requires:" for this version as that would create a circular build
> dependency (I want to be able to use this data package in pari's %check), so
> the conflict seems to be the best way of expressing this relationship.
I think, these data packages should not "require pari" nor should they "conflict:", but be treated as arbitrary and independent data packages.
Comment 14 Paul Howarth 2012-05-28 03:39:12 EDT
I've looked through the changelog for pari and it looks like old versions of pari can't use elldata but wouldn't be broken by it (i.e. in this case there wasn't a data format change, whcih was what I was concerned about). So I've dropped the conflict in -5.

Spec URL: http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/pari-elldata.spec
SRPM URL: http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-5.src.rpm
Comment 15 Paulo Andrade 2012-05-31 19:44:49 EDT
(In reply to comment #14)
> I've looked through the changelog for pari and it looks like old versions of
> pari can't use elldata but wouldn't be broken by it (i.e. in this case there
> wasn't a data format change, whcih was what I was concerned about). So I've
> dropped the conflict in -5.
> 
> Spec URL:
> http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/
> pari-elldata.spec
> SRPM URL:
> http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-5.src.rpm

Did you mean ....-5.fc18.src.rpm?  :-)
Comment 16 Paulo Andrade 2012-05-31 20:03:04 EDT
-%<- fedora-review -v -b 822896 -n pari-elldata -%<-

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[!]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
     Note: The package did not built BR could therefore not be checked or the
     package failed to build because of missing BR
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[!]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[-]: MUST License field in the package spec file matches the actual license.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/pcpa/rpmbuild/822896/elldata.tgz :
  MD5SUM this package     : 935c9b3c24e61219ab03bbe4977f3745
  MD5SUM upstream package : 935c9b3c24e61219ab03bbe4977f3745
/home/pcpa/rpmbuild/822896/elldata.tgz.asc :
  MD5SUM this package     : ff1c5ce1d8fb9fdac12e10990af3ab8e
  MD5SUM upstream package : ff1c5ce1d8fb9fdac12e10990af3ab8e

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[!]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
See: http://fedoraproject.org/wiki/Packaging/Guidelines#Architecture_Support
[!]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
     Note: The package did not built BR could therefore not be checked or the
     package failed to build because of missing BR
See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2


Generated by fedora-review 0.1.3
External plugins:
-%<- -%<-

fedora-review failed due to wrong srpm link, but there is the
issue of it installing in %{_datadir}/pari, but not requiring
the "owner" of %{_datadir}/pari.

The entries I left as "-" is because it is N/A or I am not
100% sure either, but not a failure, e.g. about asking
upstream to add a proper license file, but it is already
justified that the data packages have the same license as
the main package that makes use of them.

I only added "!" for:
MUST Package requires other packages for directories it uses.
fedora-review added the others.
Comment 17 Paul Howarth 2012-06-01 08:08:57 EDT
(In reply to comment #15)
> (In reply to comment #14)
> > I've looked through the changelog for pari and it looks like old versions of
> > pari can't use elldata but wouldn't be broken by it (i.e. in this case there
> > wasn't a data format change, whcih was what I was concerned about). So I've
> > dropped the conflict in -5.
> > 
> > Spec URL:
> > http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/
> > pari-elldata.spec
> > SRPM URL:
> > http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-5.src.rpm
> 
> Did you mean ....-5.fc18.src.rpm?  :-)

Yes, I did:

Spec URL:
http://subversion.city-fan.org/repos/cfo-repo/pari-elldata/branches/fedora/pari-elldata.spec
SRPM URL:
http://www.city-fan.org/~paul/extras/pari/pari-elldata-20120415-5.fc18.src.rpm

(In reply to comment #16)
> fedora-review failed due to wrong srpm link, but there is the
> issue of it installing in %{_datadir}/pari, but not requiring
> the "owner" of %{_datadir}/pari.

It cannot require pari (the owner of %{_datadir}/pari) as that would create circular build dependencies. Hence this package also owns %{_datadir}/pari so that the directory is owned if this package is installed without pari itself.

Multiple packages owning the same directory is OK by the guidelines if there is no dependecy relationship between those packages:

http://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function
Comment 18 Paulo Andrade 2012-06-01 21:04:46 EDT
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[!]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint pari-elldata-20120415-5.fc18.noarch.rpm

pari-elldata.noarch: W: spelling-error %description -l en_US Cremona -> Anacreon
pari-elldata.noarch: W: spelling-error %description -l en_US ellsearch -> ell search, ell-search, Chelsea
pari-elldata.noarch: W: spelling-error %description -l en_US ellidentify -> ell identify, ell-identify, identify
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


rpmlint pari-elldata-20120415-5.fc18.src.rpm

pari-elldata.src: W: spelling-error %description -l en_US Cremona -> Anacreon
pari-elldata.src: W: spelling-error %description -l en_US ellsearch -> ell search, ell-search, Chelsea
pari-elldata.src: W: spelling-error %description -l en_US ellidentify -> ell identify, ell-identify, identify
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/pcpa/rpmbuild/822896/elldata.tgz :
  MD5SUM this package     : 935c9b3c24e61219ab03bbe4977f3745
  MD5SUM upstream package : 935c9b3c24e61219ab03bbe4977f3745
/home/pcpa/rpmbuild/822896/elldata.tgz.asc :
  MD5SUM this package     : ff1c5ce1d8fb9fdac12e10990af3ab8e
  MD5SUM upstream package : ff1c5ce1d8fb9fdac12e10990af3ab8e

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[x]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Rpmlint output is silent.
[!]: MUST Package does not own files or directories owned by other packages.

rpmlint pari-elldata-20120415-5.fc18.noarch.rpm

pari-elldata.noarch: W: spelling-error %description -l en_US Cremona -> Anacreon
pari-elldata.noarch: W: spelling-error %description -l en_US ellsearch -> ell search, ell-search, Chelsea
pari-elldata.noarch: W: spelling-error %description -l en_US ellidentify -> ell identify, ell-identify, identify
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


rpmlint pari-elldata-20120415-5.fc18.src.rpm

pari-elldata.src: W: spelling-error %description -l en_US Cremona -> Anacreon
pari-elldata.src: W: spelling-error %description -l en_US ellsearch -> ell search, ell-search, Chelsea
pari-elldata.src: W: spelling-error %description -l en_US ellidentify -> ell identify, ell-identify, identify
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint

-----------------------------------------------------------------
rpmlint spelling should be ignored, and package does not own
directories owned by other packages was discussed already.

I consider the package approved.
Comment 19 Paul Howarth 2012-06-02 09:36:15 EDT
New Package SCM Request
=======================
Package Name: pari-elldata
Short Description: PARI/GP Computer Algebra System elliptic curves
Owners: pghmcfc
Branches: f16 f17 el5 el6
Comment 20 Gwyn Ciesla 2012-06-02 13:32:07 EDT
Git done (by process-git-requests).
Comment 21 Fedora Update System 2012-06-02 16:59:29 EDT
pari-galdata-20080411-3.el6,pari-elldata-20120415-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/pari-galdata-20080411-3.el6,pari-elldata-20120415-5.el6
Comment 22 Fedora Update System 2012-06-02 17:02:23 EDT
pari-galdata-20080411-3.fc16,pari-elldata-20120415-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pari-galdata-20080411-3.fc16,pari-elldata-20120415-5.fc16
Comment 23 Fedora Update System 2012-06-02 17:02:42 EDT
pari-galdata-20080411-3.el5.1,pari-elldata-20120415-5.el5.1 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/pari-galdata-20080411-3.el5.1,pari-elldata-20120415-5.el5.1
Comment 24 Fedora Update System 2012-06-02 17:03:01 EDT
pari-galdata-20080411-3.fc17,pari-elldata-20120415-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/pari-galdata-20080411-3.fc17,pari-elldata-20120415-5.fc17
Comment 25 Fedora Update System 2012-06-11 20:32:55 EDT
pari-galdata-20080411-3.fc16, pari-elldata-20120415-5.fc16 has been pushed to the Fedora 16 stable repository.
Comment 26 Fedora Update System 2012-06-11 20:35:54 EDT
pari-galdata-20080411-3.fc17, pari-elldata-20120415-5.fc17 has been pushed to the Fedora 17 stable repository.
Comment 27 Fedora Update System 2012-06-19 13:00:45 EDT
pari-galdata-20080411-3.el5.1, pari-elldata-20120415-5.el5.1 has been pushed to the Fedora EPEL 5 stable repository.
Comment 28 Fedora Update System 2012-06-19 13:03:47 EDT
pari-galdata-20080411-3.el6, pari-elldata-20120415-5.el6 has been pushed to the Fedora EPEL 6 stable repository.

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