Bug 483364 - Review Request: EekBoek - Bookkeeping software for small and medium-size businesses
Review Request: EekBoek - Bookkeeping software for small and medium-size busi...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-30 18:23 EST by Johan Vromans
Modified: 2009-05-22 12:48 EDT (History)
4 users (show)

See Also:
Fixed In Version: 1.04.03-3.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-15 19:27:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
hdegoede: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Johan Vromans 2009-01-30 18:23:30 EST
Spec URL: http://www.squirrel.nl/pub/xfer/EekBoek.spec
SRPM URL: http://www.squirrel.nl/pub/xfer/EekBoek-1.04.02-1.fc11.src.rpm
Description:
EekBoek is a bookkeeping package for small and medium-size businesses.
Unlike other accounting software, EekBoek has both a command-line
interface (CLI) and a graphical user-interface (GUI, currently under
development). Furthermore, it has a complete Perl API to create your
own custom applications. EekBoek is designed for the Dutch/European
market and currently available in Dutch only. An English translation
is in the works (help appreciated).

The spec file is non-trivial but well commented.
The spec file and rpms are rpmlint free with the exception of two files that are not utf8. This is intentional, these files need to be latin1.

Results of last koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1094851

This is the third package I submit for review, and I'm looking for a sponsor.
Comment 1 Mamoru TASAKA 2009-03-13 13:07:40 EDT
Some random notes:

* Licensing
  - First of all, from what file(s) can we judge that this
    software is under Artistic 2.0? 

* Requires/Provides
  - What package provides "%{name}-db"? (are the rebuilt binary
    rpms installable for you?)

  - By the way, if you make -db-{sqlite,postgresql} subpackages
    have "Provides: %{name}-db = %{version}", AFAIK
    "yum install EekBoek" will always install EekBoek and
    EekBoek-db-sqlite because yum usually gives higher priority
    to shorter name.
    Is this what you expect? Usually I don't recommend to add
    the virtual Requires on the "main" package like this (because
    with this yum will always choose one) and suggest to write
    a note which tells that admins have to install either of
    the packages providing database backends afterwards by
    themselves.

* BuildRequires
  - "BuildRequires: glibc-common" is redundant.

* Duplicate directories
  - Since both database backend subpackages have "Requires: %name",
    listing the following directories in the subpackages is
    not needed. Just move to the main package.
---------------------------------------------------------------
%dir %{ebshare}
%dir %{ebshare}/lib
%dir %{ebshare}/lib/EB
%dir %{ebshare}/lib/EB/DB
---------------------------------------------------------------

* %changelog
  - As it is useful when using Fedora CVS, I recommend to put
    one line between %changlog entries like:
---------------------------------------------------------------
%changelog
* Fri Jan 30 2009 Johan Vromans <jvromans@squirrel.nl> - 1.04.02-1
- Adapt to Fedora guidelines

* Sun Jan 26 2009 Johan Vromans <jvromans@squirrel.nl> - 1.04.02
- Remove QUICKSTART.

* Sat Jul 19 2008 Johan Vromans <jvromans@squirrel.nl> - 1.03.90
- Remove debian stuff
- Don't use unstable.
---------------------------------------------------------------
Comment 2 Mamoru TASAKA 2009-03-19 12:46:56 EDT
ping?
Comment 3 Johan Vromans 2009-03-19 13:15:52 EDT
Stay tuned, I'll get back ASAP.
Comment 4 Johan Vromans 2009-03-19 14:21:48 EDT
Thanks for your kind and constructive feedback. Most items are trivially fixed, which I will do in a next release.

The requires/provides "%{name}-db" seems to be confusing, which may be an indication that the intention is not clear.  Or wrong.

As you may have guessed, EekBoek supports both SQLite and PostgreSQL as database backend (and probably more to come). It is tempting to bundle the SQLite backend with the main package (that is what Debian does) but I personally do not like having to install a package that one doesn't need.

What would be a better approach for this?
Comment 5 Mamoru TASAKA 2009-03-19 14:36:17 EDT
(In reply to comment #4)
> Thanks for your kind and constructive feedback. Most items are trivially fixed,
> which I will do in a next release.
> 
> The requires/provides "%{name}-db" seems to be confusing, which may be an
> indication that the intention is not clear.  Or wrong.
> 
> As you may have guessed, EekBoek supports both SQLite and PostgreSQL as
> database backend (and probably more to come). It is tempting to bundle the
> SQLite backend with the main package (that is what Debian does) but I
> personally do not like having to install a package that one doesn't need.
> 
> What would be a better approach for this? 

I would say that bundling SQLite backend to main package is
undesired solution.

In such case I recommend
- not to use virtual "Provodes/Requires: %{name}-db"
- write some text named "README.Fedora", for example to inform
  that
  - BekBook needs one of the database backends
  - Currently Fedora supports SQLite and PostgreSQL
  - The admin has to choose either of them and install
    the choosed one by "yum install BekBook-db-<choosed backend>"
    by him/herself
  and add to the "%description" of BekBook package some sentence
  like "Please read README.Fedora"
Comment 6 Johan Vromans 2009-03-19 14:43:26 EDT
Thanks for the feedback. Unless someone comes up with a more elegant solution I'll consider this approach for the next release.
Comment 7 Hans de Goede 2009-04-03 08:53:51 EDT
As discussed already, I'll review this and your other 2 perl package submissions,
and once they are all approved, I'll sponsor you.

I've done a full review of this package and I fully agree with Mamoru, please
fix the points he has addressed and then I'll aprove this.

As for the license tag, I've checked the various .pm sources, and the correct
license tag would be:
License: GPL+ or Artistic
Comment 8 Johan Vromans 2009-04-16 06:15:08 EDT
(In reply to comment #5)
> I would say that bundling SQLite backend to main package is
> undesired solution.

However, in https://bugzilla.redhat.com/show_bug.cgi?id=483286#c6, Hans de Goede writes:

> I know this is optional but in Fedora we always try to enable as much
> features in packaes as possible, even if this drags in a few additional
> dependencies.

Following this practice, it would be preferred to include the SQLite driver in the main package, especially since SQLite is relatively small and already a core part of many packages.

This would result in a working installation for people that do not read READMEs.
Comment 9 Hans de Goede 2009-04-16 06:34:05 EDT
(In reply to comment #8)
> (In reply to comment #5)
> > I would say that bundling SQLite backend to main package is
> > undesired solution.
> 
> However, in https://bugzilla.redhat.com/show_bug.cgi?id=483286#c6, Hans de
> Goede writes:
> 
> > I know this is optional but in Fedora we always try to enable as much
> > features in packaes as possible, even if this drags in a few additional
> > dependencies.
> 
> Following this practice, it would be preferred to include the SQLite driver in
> the main package, especially since SQLite is relatively small and already a
> core part of many packages.
> 
> This would result in a working installation for people that do not read
> READMEs.  

+1

Regards,

Hans
Comment 10 Mamoru TASAKA 2009-04-16 11:50:48 EDT
(In reply to comment #8)
> (In reply to comment #5)
> > I would say that bundling SQLite backend to main package is
> > undesired solution.
> 
> However, in https://bugzilla.redhat.com/show_bug.cgi?id=483286#c6, Hans de
> Goede writes:
> 
> > I know this is optional but in Fedora we always try to enable as much
> > features in packaes as possible, even if this drags in a few additional
> > dependencies.
> 
> Following this practice, it would be preferred to include the SQLite driver in
> the main package, especially since SQLite is relatively small and already a
> core part of many packages.
> 
> This would result in a working installation for people that do not read
> READMEs.  

Actually SQLite is rather small and I agree with you.
Comment 11 Johan Vromans 2009-04-17 06:06:59 EDT
Spec URL: http://www.squirrel.nl/pub/xfer/EekBoek.spec
SRPM URL: http://www.squirrel.nl/pub/xfer/EekBoek-1.04.03-1.fc11.src.rpm
Description:
EekBoek is a bookkeeping package for small and medium-size businesses.
Unlike other accounting software, EekBoek has both a command-line
interface (CLI) and a graphical user-interface (GUI, currently under
development). Furthermore, it has a complete Perl API to create your
own custom applications. EekBoek is designed for the Dutch/European
market and currently available in Dutch only. An English translation
is in the works (help appreciated).

The spec file is non-trivial but well commented.
The spec file and rpms are rpmlint free with the exception of two files that
are not utf8. This is intentional, these files need to be latin1.

All comments from the reviews are applied. SQLite is now included as default database backend.

Source is upgraded to new upstream version.

Results of last koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1303621
Comment 12 Mamoru TASAKA 2009-04-20 15:01:22 EDT
Some notes:

* %define -> %global
  - For some reasons (especially due to rpm's "unexpected" behaviour
    with nested macros), Fedora now suggests to use %global instead
    of %define:
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

* Including additional documents as Source
  - When including additional documents, please add them as Sources
    instead of making patch, like:
-----------------------------------------------------------------
Source1: README.postgres
.....
.....
%prep
%setup -q
%patch0 -p0 -b .script
cp -p %SOURCE1 .
-----------------------------------------------------------------
    (Also see the below comments about timestamps)

* Dependency between subpackages
  - Usually the dependency between packages rebuilt from the same
    srpm is strict EVR (Epoch-Version-Release) specific (not only
    version specific):
    https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package

! %setup
  - "-n %{name}-%{version}" is the default of %setup, so
    "%setup -q" is enough here.

* Timestamps
  - When using "cp" or "install" commands, use "-p" option to
    keep timestamps on installed files:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

* Permission
-----------------------------------------------------------------
%{__find} blib/lib ! -type d -printf "%{__install} -m 0444 %p %{buildroot}%{ebshare}/lib/%%P\n" | sh -x
-----------------------------------------------------------------
  - Usually these files should have 0644 permission.

* %files
-----------------------------------------------------------------
# Collect the list of files. Basically we include all files except
# the EB/DB (database specific) modules.
( cd %{buildroot};
  %{__find} ./%{ebshare} -type d -printf "%%%%dir %{ebshare}/%%P\n";
  %{__find} ./%{ebshare} -type f -printf "%{ebshare}/%%P\\n" | %{__grep} -v "lib/EB/DB/.*pm"
) > .files

# Include the SQLite driver.
echo '%{ebshare}/lib/EB/DB/Sqlite.pm' >> .files
-----------------------------------------------------------------
   - The following is much simpler:
-----------------------------------------------------------------
%files
%defattr(-,root,root,-)
%doc .......
%dir %{_sysconfdir}/%{lcname}
%config(noreplace) %{_sysconfdir}/%{lcname}/%{lcname}.conf
%{ebshare}/
%exclude %{ebshare}/lib/EB/DB/Postgres.pm
%{_bindir}/ebshell
%{_mandir}/man1/*

%files db-postgresql
%defattr(-,root,root,-)
%doc README.postgres
%{ebshare}/lib/EB/DB/Postgres.pm
-----------------------------------------------------------------

* Documents
  - Usually the file "INSTALL" is for people who want to install the
    package by themselves and not needed for rpm users.
Comment 13 Hans de Goede 2009-04-20 15:08:29 EDT
Mamoru thanks for the input, Johan what Mamoru said :)
Comment 14 Johan Vromans 2009-04-20 18:25:09 EDT
Spec URL: http://www.squirrel.nl/pub/xfer/EekBoek.spec
SRPM URL: http://www.squirrel.nl/pub/xfer/EekBoek-1.04.03-2.fc11.src.rpm
Description:
EekBoek is a bookkeeping package for small and medium-size businesses.
Unlike other accounting software, EekBoek has both a command-line
interface (CLI) and a graphical user-interface (GUI, currently under
development). Furthermore, it has a complete Perl API to create your
own custom applications. EekBoek is designed for the Dutch/European
market and currently available in Dutch only. An English translation
is in the works (help appreciated).

The spec file is non-trivial but well commented.
The spec file and rpms are rpmlint free with the exception of two files that
are not utf8. This is intentional, these files need to be latin1.

All comments from review #12 are applied, although I must remark that I do not see why an Epoch is needed. The link is all but helpful, and some further reading reveals that 'Epoch should be avoided if possible'...

Results of last koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1310184
Comment 15 Mamoru TASAKA 2009-04-21 01:55:03 EDT
(In reply to comment #14)
> All comments from review #12 are applied, although I must remark that I do not
> see why an Epoch is needed. The link is all but helpful, and some further
> reading reveals that 'Epoch should be avoided if possible'...

Ah, I didn't say Epoch must be introduced, what I said is that
-db-postgresql subpackage should have "%{name} = %{version}-%{release}" which
is exact EVR specific, not just "%{name} = %{version}".
Comment 16 Johan Vromans 2009-04-21 03:43:59 EDT
Thanks for the clarification. I'll throw out the epoch related things.
Comment 17 Johan Vromans 2009-04-22 02:13:08 EDT
Spec URL: http://www.squirrel.nl/pub/xfer/EekBoek.spec
SRPM URL: http://www.squirrel.nl/pub/xfer/EekBoek-1.04.03-3.fc11.src.rpm
Description:
EekBoek is a bookkeeping package for small and medium-size businesses.
Unlike other accounting software, EekBoek has both a command-line
interface (CLI) and a graphical user-interface (GUI, currently under
development). Furthermore, it has a complete Perl API to create your
own custom applications. EekBoek is designed for the Dutch/European
market and currently available in Dutch only. An English translation
is in the works (help appreciated).

The spec file is non-trivial but well commented.
The spec file and rpms are rpmlint free with the exception of two files that
are not utf8. This is intentional, these files need to be latin1.

Same as 1.04.03-2, with Epoch: removed.

Results of last koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1313959
Comment 18 Hans de Goede 2009-04-22 04:16:20 EDT
(In reply to comment #17)
> Spec URL: http://www.squirrel.nl/pub/xfer/EekBoek.spec
> SRPM URL: http://www.squirrel.nl/pub/xfer/EekBoek-1.04.03-3.fc11.src.rpm

Looks fine to me now, approved!

Johan, can you please apply for packager group member ship in the account system
(if you haven't done that already), and tell me your FAS username, then I'll sponsor you.

Regards,

Hans
Comment 19 Johan Vromans 2009-04-22 07:44:06 EDT
New Package CVS Request
=======================
Package Name: EekBoek
Short Description: Bookkeeping package for small and medium-size businesses
Owners: sciurius
Branches: F-10 F-11
InitialCC:
Comment 20 Kevin Fenzi 2009-04-23 12:26:12 EDT
cvs done.
Comment 21 Fedora Update System 2009-04-26 18:08:27 EDT
EekBoek-1.04.03-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/EekBoek-1.04.03-3.fc11
Comment 22 Fedora Update System 2009-04-26 18:08:34 EDT
EekBoek-1.04.03-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/EekBoek-1.04.03-3.fc10
Comment 23 Fedora Update System 2009-04-27 17:28:15 EDT
EekBoek-1.04.03-3.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update EekBoek'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-3972
Comment 24 Fedora Update System 2009-05-08 23:57:56 EDT
EekBoek-1.04.03-3.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update EekBoek'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-4342
Comment 25 Fedora Update System 2009-05-15 19:27:33 EDT
EekBoek-1.04.03-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 26 Fedora Update System 2009-05-15 19:34:33 EDT
EekBoek-1.04.03-3.fc11 has been pushed to the Fedora 11 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.