Bug 169971 - Review Request: libqalculate - Multi-purpose calculator library
Review Request: libqalculate - Multi-purpose calculator library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
David Lawrence
http://qalculate.sourceforge.net/
:
: 169974 (view as bug list)
Depends On:
Blocks: FE-ACCEPT 169972 170296
  Show dependency treegraph
 
Reported: 2005-10-05 19:52 EDT by Deji Akingunola
Modified: 2008-04-07 04:16 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-10-17 16:22:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Patch addressing review issues (2.73 KB, patch)
2005-10-10 13:18 EDT, Paul Howarth
no flags Details | Diff
Split off qalculate subpackage and update to 0.8.2 (2.42 KB, patch)
2005-10-11 08:28 EDT, Paul Howarth
no flags Details | Diff

  None (edit)
Description Deji Akingunola 2005-10-05 19:52:13 EDT
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-0.8.1.2-1.src.rpm
Description: Qalculate! is a multi-purpose desktop calculator for GNU/Linux. It is small and simple to use but with much power and versatility underneath. Features include customizable functions, units, arbitrary precision, plotting, and a user-friendly interface (KDE or GTK+).
Comment 1 Paul Howarth 2005-10-10 09:46:37 EDT
*** Bug 169974 has been marked as a duplicate of this bug. ***
Comment 2 Paul Howarth 2005-10-10 13:17:22 EDT
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...
Comment 3 Paul Howarth 2005-10-10 13:18:13 EDT
Created attachment 119779 [details]
Patch addressing review issues
Comment 4 Deji Akingunola 2005-10-10 14:17:59 EDT
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
Comment 5 Michael Schwendt 2005-10-10 17:56:49 EDT
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".
Comment 6 Paul Howarth 2005-10-11 03:02:24 EDT
(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.
Comment 7 Michael Schwendt 2005-10-11 05:51:58 EDT
> 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.
Comment 8 Paul Howarth 2005-10-11 08:28:35 EDT
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.
Comment 9 Deji Akingunola 2005-10-11 11:03:14 EDT
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.
Comment 10 Paul Howarth 2005-10-11 11:44:14 EDT
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?
Comment 11 Deji 2005-10-11 13:01:35 EDT
(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.
Comment 12 Paul Howarth 2005-10-14 10:52:38 EDT
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.
Comment 13 Deji Akingunola 2005-10-14 12:28:40 EDT
(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.

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