*** Bug 169974 has been marked as a duplicate of this bug. ***

Good:

- package and spec naming OK
- package meets most guidelines
- license is GPL
- spec file written in English and is legible
- sources match upstream
- package builds OK on FC4 and in mock for rawhide (i386)
- proper use of %find_lang for locales
- not relocatable
- no directory ownership or permissions issues
- no duplicate files
- %clean section present and correct
- macro usage is consistent
- code, not content
- no large docs
- docs don't affect runtime
- libtool archive not included
- no desktop file needed
- qalc seems to run ok, but I only played very briefly with it

Needswork:

- rpmlint not clean, mainly due to not splitting off header files etc. into a
  separate -devel subpackage
- BuildReqs need a bit of work (see below)
- ldconfig not run in %post/%postun
- package includes static libraries (deprecated in Fedora)
- http://qalculate.sourceforge.net/downloads.html suggests that libxml2 >=
  2.3.8 is a buildreq
- the program will include readline/ncurses support if available at build
  time, so readline-devel & ncurses-devel should be added as buildreqs
- explicit glib2 & libxml2 deps are redundant and should be removed
- the Makefile includes DESTDIR support, which I believe is preferred to
  using %makeinstall
- the README file just includes a URL to visit so isn't really worth including
- the AUTHORS and TODO files probably are worth including as %doc
- license text in COPYING should be included as %doc

Suggestions:

- perhaps split off a separate package for the text-mode qalc program itself
  (maybe called qalculate?)

I'll attach a patch addressing the Needswork issues.


I haven't reviewed a library before so anyone more experienced with those can
please take a look too...

Created attachment 119779 [details]
Patch addressing review issues

Updated spec file and .srpm here 
Spec Name or Url: ftp://czar.eas.yorku.ca/pub/qalculate/libqalculate.spec
SRPM Name or Url:
ftp://czar.eas.yorku.ca/pub/qalculate/libqalculate-1.8.1.2-2.src.rpm

I'm sure though if another package needs to be spin-off from this.

Thanks Paul for the review and the patch.

Deji

What I find odd is that the package is called "libqalculate", but that
the package description reads like it's an application
(quote: a "multi-purpose desktop calculator").

It is commonly considered bad taste to repeat the product name in
the "Summary:" line (better: "Summary: Multi-purpose calculator library"
or similar).

libcalculate-devel contains a pkg-config file which links -lgmp,
so the package should "Requires: gmp-devel".

(In reply to comment #5)
> What I find odd is that the package is called "libqalculate", but that
> the package description reads like it's an application
> (quote: a "multi-purpose desktop calculator").

I agree, which is why I suggested a separate (sub)package for qalc itself.

> It is commonly considered bad taste to repeat the product name in
> the "Summary:" line (better: "Summary: Multi-purpose calculator library"
> or similar).

Agreed.

> libcalculate-devel contains a pkg-config file which links -lgmp,
> so the package should "Requires: gmp-devel".

gmp-devel is a dep of cln-devel, which is already a dep of libqalculate-devel.

> gmp-devel is a dep of cln-devel

Ah, okay, never mind. Your intention is to optimise the dependency chain.
On the contrary, I don't mind if a direct dependency of a file in the
package is reflected in a direct "Requires". Matter of taste. No big
issue.

Created attachment 119800 [details]
Split off qalculate subpackage and update to 0.8.2

Attached patch updates spec for new 0.8.2 release and splits off the actual
calculator program into a separate package, with sensible descriptions for each
package.

I've implemented changes suggested by Michael and Paul; new spec file and srpm
available at;
ftp://czar.eas.yorku.ca/pub/qalculate/libqalculate.spec
ftp://czar.eas.yorku.ca/pub/qalculate/libqalculate-0.8.2-1.src.rpm

Thanks Michael and Paul.

I'm happy enough with this to approve it now. Any more comments on it Michael
(or anyone else)?

Are you already a Fedora Extras Contributor, or are you looking for a sponsor?

(In reply to comment #10)
> I'm happy enough with this to approve it now. Any more comments on it Michael
> (or anyone else)?
> 
> Are you already a Fedora Extras Contributor, or are you looking for a sponsor?

No, I'm not a Fedora Extras Contributor yet, and i'll like to have a sponsor.
Thanks.

Have you been through the CLA part of the Fedora Extras contributors system yet?
If not, do that. Then apply for membership of the cvsextras group and let me
know what your account name is and I'll sponsor you.

(In reply to comment #12)
> Have you been through the CLA part of the Fedora Extras contributors system yet?
> If not, do that. Then apply for membership of the cvsextras group and let me
> know what your account name is and I'll sponsor you.

I've just gone through the process now. My account name is 'deji'.
Thanks.