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
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Package Reviews List
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)
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:


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.