Bug 483364

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> - 1.04.02-1
- Adapt to Fedora guidelines

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

* Sat Jul 19 2008 Johan Vromans <jvromans> - 1.03.90
- Remove debian stuff
- Don't use unstable.
---------------------------------------------------------------

ping?

Stay tuned, I'll get back ASAP.

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?

(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"

Thanks for the feedback. Unless someone comes up with a more elegant solution I'll consider this approach for the next release.

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

(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.

(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

(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.

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

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.

Mamoru thanks for the input, Johan what Mamoru said :)

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

(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}".

Thanks for the clarification. I'll throw out the epoch related things.

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

(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

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:

cvs done.

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

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

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

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

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.

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.