207802 – Review Request: libpaper - Library and tools for handling papersize

Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 207802 - Review Request: libpaper - Library and tools for handling papersize

Summary: Review Request: libpaper - Library and tools for handling papersize

Keywords:
Status:	CLOSED RAWHIDE
Alias:	None
Product:	Fedora
Classification:	Fedora
Component:	Package Review
Sub Component:
Version:	rawhide
Hardware:	All
OS:	Linux
Priority:	medium
Severity:	medium
Target Milestone:	---
Assignee:	Dan Horák
QA Contact:	Fedora Package Reviews List
Docs Contact:
URL:
Whiteboard:
Depends On:
Blocks:	FE-ACCEPT 207761
TreeView+	depends on / blocked

Reported:	2006-09-23 13:53 UTC by Tom "spot" Callaway
Modified:	2007-11-30 22:11 UTC (History)
CC List:	1 user (show)
Fixed In Version:
Doc Type:	Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:	2006-09-23 16:38:33 UTC
Type:	---
Embargoed:
Dependent Products:

Attachments	(Terms of Use)

Description Tom "spot" Callaway 2006-09-23 13:53:04 UTC

Spec URL: http://www.auroralinux.org/people/spot/review/libpaper.spec
SRPM URL: http://www.auroralinux.org/people/spot/review/libpaper-1.1.20-1.fc6.src.rpm
Description: 
The paper library and accompanying files are intended to provide a
simple way for applications to take actions based on a system- or
user-specified paper size. This release is quite minimal, its purpose
being to provide really basic functions (obtaining the system paper name
and getting the height and width of a given kind of paper) that
applications can immediately integrate.

Comment 1 Dan Horák 2006-09-23 14:49:01 UTC

rpmlint from Rawhide gives
	W: libpaper mixed-use-of-spaces-and-tabs (on the src.rpm)

Could the static library be excluded?

When running paperconf, it could not find /etc/papersize.

Comment 2 Tom "spot" Callaway 2006-09-23 15:03:52 UTC

/etc/papersize is (re)generated with /usr/sbin/paperconfig, but the package
should own it. I set letter as the default.

New SRPM:
http://www.auroralinux.org/people/spot/review/libpaper-1.1.20-2.fc6.src.rpm
New SPEC:
http://www.auroralinux.org/people/spot/review/libpaper.spec

Comment 3 Dan Horák 2006-09-23 16:09:28 UTC

Review:
- no rpmlint output on any package
- package name OK
- spec file name OK, is in English and is legible
- package meets the Packaging Guidelines
- license OK (GPL) and is included
- source matches upstream
	7075f580606a84e63b7d6d9fa3124c31  libpaper_1.1.20.tar.gz
	7075f580606a84e63b7d6d9fa3124c31  libpaper_1.1.20.tar.gz.1
- compiles and builds at least on i386 (devel)
- BuildRequires are correct
- contains no localized files
- has shared lib with appropriate ldconfig calls
- does not create any directory
- no duplicates files, permissions are set properly, uses %defattr
- has %clean section
- consistent use of macros
- contains code
- no large docs, %doc is not required during runtime
- devel subpackage required and present, contains header
- no .la libtool archives
- not a GUI application
- it works

APPROVED

The static library can be taken out also with %configure --disable-static.

Comment 4 Tom "spot" Callaway 2006-09-23 16:38:33 UTC

Thanks for the fast review, built in FC-5 and devel.

Comment 5 Patrice Dumas 2006-09-23 17:20:40 UTC

I have spotted some more or less minor issues:

* the man pages for functions should be in the devel 
  package

%{_mandir}/man3/*

* removing unneded files
rm -rf $RPM_BUILD_ROOT%{_libdir}/*.la
rm -rf $RPM_BUILD_ROOT%{_libdir}/*.a
should be without -r and even maybe without f

* I think that there is no need to set letter as a default, it 
  is allready the default according to the man paperconf. In my 
  opinion, what should be better is an empty file or a file with a 
  comment along 
echo '# Simply write the paper name. See papersize(5) for possible values' > 
$RPM_BUILD_ROOT%{_sysconfdir}/papersize

* in man paperconfig there is a reference to /etc/libpaper.d

* The file NEWS in debian/ should certainly be in %doc

Comment 6 Dan Horák 2006-09-23 17:56:08 UTC

Yes, I really missed the man3 directory. The rest is minor, but reasonable.

Also in debian/po subdirectory there are hidden localized files. It would be
nice to have them installed.

Comment 7 Tom "spot" Callaway 2006-09-24 01:09:37 UTC

Thanks for the feedback. -3 should resolve all the above issues.

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