Bug 476829 - Review Request: foomatic-db - Database of printers and printer drivers
Review Request: foomatic-db - Database of printers and printer drivers
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 461234
  Show dependency treegraph
 
Reported: 2008-12-17 06:59 EST by Tim Waugh
Modified: 2009-08-18 04:57 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-18 04:57:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Tim Waugh 2008-12-17 06:59:31 EST
Spec URL: http://twaugh.fedorapeople.org/foomatic-db/foomatic-db.spec
SRPM URL: http://twaugh.fedorapeople.org/foomatic-db/foomatic-db-3.0.0.20081124-1.fc9.src.rpm
Description:
This is the database of printers, printer drivers, and driver options
for Foomatic.

The site http://www.openprinting.org/ is based on this database.


--

Note to reviewer:
This package has been split out of the main 'foomatic' package.  The reason is that the database itself should be a noarch package as it is architecture-independent, while the engine for accessing it is small but architecture-dependent.

Once this package has been approved I will adjust the main foomatic package so that it requires foomatic-db (and does not ship the database itself).
Comment 1 Dan Horák 2008-12-17 07:12:52 EST
Are you aware that rpmbuild since F-10 can create a noarch subpackage while the main package is "arch"? It was announced as one of the new features in rpm 4.6.
Comment 2 Tim Waugh 2008-12-17 10:49:31 EST
I am, although another reason for having a separate source package is so that bug fixes in the database engine can be distributed separately from the entire database which is quite large.
Comment 3 Dan Horák 2008-12-17 12:23:45 EST
Yes, that sounds completely reasonable.

Few notes before full review:
- you can drop setting the CFLAGS for the "make" commands in %build, there is no C/C++ code built, if I see it right
- are the INSTALLSITELIB and INSTALLSITEARCH really required for the "make" commands in %install? They looks like some pythonic paths.
- what is the upstream versioning scheme? Does the version in the included archive mean that it is a snapshot made 20081124 after the 3.0 release? Then you should apply "post-release package guideline" - https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Post-Release_packages (version=3.0, release=1.20081124)
- according to the README in foomatic-db, there should be files licensed under MIT too, please recheck and update the License tag to "GPLv2+ and MIT"
- new snapshot was released today :-)
Comment 4 Dan Horák 2008-12-19 05:23:32 EST
formal review is here, see the notes below:

source files match upstream:

BAD	package meets naming and versioning guidelines.
BAD	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
BAD?	license field matches the actual license.
OK*	license is open source-compatible. License text not included upstream.
BAD	latest version is being packaged.
OK	BuildRequires are proper.
N/A	compiler flags are appropriate.
BAD	%clean is present.
OK	package builds in mock (Rawhide/x86_64).
N/A	debuginfo package looks complete.
OK	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	no shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
OK	no scriptlets present.
OK	allowed content
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	no headers.
OK	no pkgconfig files.
OK	no libtool .la droppings.
OK	not a GUI app.

- see comment #3 for more detailed description of the issues
- versioning is wrong for a post-release snapshop
- AFAICT unused variable are passed in the make calls
- recheck the licenses for the files
- include the license(s) as %doc
- new snapshot is available
- using only "rm -rf %{buildroot}" is sufficient for the %clean section
- is the database useful for other purpose then for cups?
Comment 5 Tim Waugh 2009-08-03 07:39:01 EDT
(In reply to comment #3)
> - you can drop setting the CFLAGS for the "make" commands in %build, there is
> no C/C++ code built, if I see it right

Yep.

> - are the INSTALLSITELIB and INSTALLSITEARCH really required for the "make"
> commands in %install? They looks like some pythonic paths.

No, removed.

> - what is the upstream versioning scheme? Does the version in the included
> archive mean that it is a snapshot made 20081124 after the 3.0 release? Then
> you should apply "post-release package guideline" -
> https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Post-Release_packages
> (version=3.0, release=1.20081124)

It's daily snapshots and very infrequent version releases.  I've switch to the post-release scheme now.

> - according to the README in foomatic-db, there should be files licensed under
> MIT too, please recheck and update the License tag to "GPLv2+ and MIT"

Fixed (new sub-package carries MIT license).

> - using only "rm -rf %{buildroot}" is sufficient for the %clean section

Fixed.

> - is the database useful for other purpose then for cups?  

No.

I've updated the package to match the current devel foomatic package (newer snapshot, oki4linux obsoleted, etc), and also split out a 'ppds' sub-package for bug #461234.

Spec URL: http://twaugh.fedorapeople.org/foomatic-db/foomatic-db.spec
SRPM URL:
http://twaugh.fedorapeople.org/foomatic-db/foomatic-db-4.0-1.20090702.fc11.src.rpm
Comment 6 Tim Waugh 2009-08-03 13:39:17 EDT
Spec URL: http://twaugh.fedorapeople.org/foomatic-db/foomatic-db.spec
SRPM URL:
http://twaugh.fedorapeople.org/foomatic-db/foomatic-db-4.0-2.20090702.fc11.src.rpm

A few more changes:

* Mon Aug  3 2009 Tim Waugh <twaugh@redhat.com> 4.0-2.20090702
- Move foomatic-db-ppds symlink to ppds sub-package.
- Use sed instead of perl in raster PPDs (bug #512739).
- Removed code to convert old-style printer IDs (there are none).
- Ship README file.
Comment 7 Tim Waugh 2009-08-12 12:06:56 EDT
Would be really useful to have this done for Fedora 12, as it is causing perl to be required on the Live CD.
Comment 8 Dan Horák 2009-08-17 07:01:14 EDT
I am back in work from vacations, so the review continues.

I have still found few issues:
- you should have the license as "GPLv2+ and MIT" for the ppd subpackage because at least PPDs for Brother, OKI and SHARP are licensed under GPLv2+
- add the COPYING file from foomatic-db-20090702 as %doc for the main package
- the question is who should own the /usr/share/foomatic/db/sources directory, because when only the "ppds" subpackage is installed then this dir (and all "upper dirs") will be unowned. If the "ppds" subpackage can be installed independently then creating "foomatic-db-filesystem" subpackage (containing the empty /usr/share/foomatic/db structure) looks like an option to me.

The rest looks good.
Comment 9 Tim Waugh 2009-08-17 07:53:53 EDT
Spec URL: http://twaugh.fedorapeople.org/foomatic-db/foomatic-db.spec
SRPM URL:
http://twaugh.fedorapeople.org/foomatic-db/foomatic-db-4.0-3.20090702.fc11.src.rpm

* Mon Aug 17 2009 Tim Waugh <twaugh@redhat.com> 4.0-3.20090702
- License for ppds sub-package should include GPLv2+.
- Ship COPYING file in main package.
- Added filesystem sub-package for directory ownership.
Comment 10 Dan Horák 2009-08-17 08:21:25 EDT
All issues are fixed, this package is APPROVED.
Comment 11 Tim Waugh 2009-08-17 08:31:03 EDT
Thank you!
Comment 13 Tim Waugh 2009-08-17 08:33:13 EDT
New Package CVS Request
=======================
Package Name: foomatic-db
Short Description: Database of printers and printer drivers
Owners: twaugh
Branches: devel
InitialCC:
Comment 14 Kevin Fenzi 2009-08-17 14:26:58 EDT
cvs done.

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