Bug 624179 - Review Request: zathura - A lightweight PDF viewer
Summary: Review Request: zathura - A lightweight PDF viewer
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-08-13 22:37 UTC by François Cami
Modified: 2012-11-29 22:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-08-24 03:14:06 UTC
Type: ---
Embargoed:
kevin: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description François Cami 2010-08-13 22:37:25 UTC
Spec URL: http://fcami.fedorapeople.org/srpms/zathura.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/zathura-0.0.8.1-1.fc13.src.rpm
Description:

Zathura is a highly configurable and functional PDF viewer based on the Poppler
rendering library and the GTK+ toolkit. The idea behind zathura is an
application that provides a minimalist and space saving interface as well as
an easy usage that mainly focuses on keyboard interaction.

Please note that I need a sponsor as I am not a packager yet.

Comment 1 François Cami 2010-08-13 22:39:17 UTC
Adding FE-NEEDSPONSOR.

Zathura is on the Package Wishlist:
http://fedoraproject.org/wiki/PackageMaintainers/WishList#X-Z

Comment 2 François Cami 2010-08-14 00:44:53 UTC
Closing until licensing issues can be sorted out.

Comment 3 d. johnson 2010-08-14 00:47:32 UTC
Not an official reviewer.

% rpmlint zathura-*
zathura-debuginfo.i686: E: empty-debuginfo-package
2 packages and 0 specfiles checked; 1 errors, 0 warnings.

- Package meets naming and packaging guidelines
- Spec file matches base package name.
- Spec has consistent macro usage.
- Meets Packaging Guidelines.
- License - BSD-like, but does not match
http://en.wikipedia.org/wiki/BSD_licenses
- License field in spec matches (BSD)
- License file included in package
- Spec in American English
- Spec is legible.
- Sources match upstream md5sum:

- BuildRequires correct
- Package has %defattr and permissions on files is good.
- Package has a correct %clean section.
- Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- Package is code or permissible content.
- Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- .la files are removed.

- Package compiles and builds on at least one arch.
- Package has no duplicate files in %files.
- Package doesn't own any directories other packages own.
- Package owns all the directories it creates.
- No rpmlint output. (see above)
- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =;
rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and
anything provided by glibc.)

libatk-1.0.so.0  
libcairo.so.2  
libfontconfig.so.1  
libfreetype.so.6  
libgdk-x11-2.0.so.0  
libgdk_pixbuf-2.0.so.0  
libgio-2.0.so.0  
libglib-2.0.so.0  
libgmodule-2.0.so.0  
libgobject-2.0.so.0  
libgthread-2.0.so.0  
libgtk-x11-2.0.so.0  
libm.so.6  
libm.so.6(GLIBC_2.0)  
libpango-1.0.so.0  
libpangocairo-1.0.so.0  
libpangoft2-1.0.so.0  
libpoppler-glib.so.4

SHOULD Items:

- Should build in mock. (Does)
- Should build on all supported archs (Does)
- Should function as described.
- Should have sane scriptlets. (None)
- Should have subpackages require base package with fully versioned depend.
(None)
- Should have dist tag (Does)
- Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. License needs verification from author.

Comment 4 François Cami 2010-08-14 08:19:47 UTC
Thank you Dennis for the review.
The license appears to be zlib, so I've updated the spec and uploaded a new src.rpm:

Spec URL: http://fcami.fedorapeople.org/srpms/zathura.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/zathura-0.0.8.1-2.fc13.src.rpm

I seem unable to switch the bug status from CLOSED to NEW.

Comment 5 François Cami 2010-08-14 08:20:11 UTC
Switching to NEW. Takes two updates.

Comment 6 François Cami 2010-08-14 09:43:45 UTC
The build fix is now in upstream git.

Comment 7 Kevin Fenzi 2010-08-14 18:47:02 UTC
I'll go ahead and look at reviewing this. Look for a full review in a bit. ;)

Comment 8 Kevin Fenzi 2010-08-14 19:12:30 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
OK - License (zlib)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
67351e5ab66cfdda9a71c9ce6c47a970  zathura-0.0.8.1.tar.gz
67351e5ab66cfdda9a71c9ce6c47a970  zathura-0.0.8.1.tar.gz.orig

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

See below - Package is a GUI app and has a .desktop file

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin

Issues: 

1. This is a GUI app, can you make a desktop file for it so it can be seen/used
from menus? (Might contribute anything you make upstream too)

2. It doesn't look like it's obeying the standard fedora CFLAGS here. 
This should be fixed because we use some of them to provide additional security. 

Something like a 'export CFLAGS=%{optflags}' before make might work. 

3. rpmlint says: 
zathura-debuginfo.x86_64: E: empty-debuginfo-package
3 packages and 0 specfiles checked; 1 errors, 0 warnings.

The debuginfo issue is likely due to wrong cflags or the Makefile calling strip.

Comment 9 François Cami 2010-08-14 22:33:00 UTC
Thanks Kevin.

1. zathura.desktop file added, will ask upstream to include it.

2. export CFLAGS="%{optflags}" added and apparently used.

3. we now use the "debug" build target, and avoid rebuilding the whole package at install time. The debuginfo package is now generated properly.

Spec URL: http://fcami.fedorapeople.org/srpms/zathura.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/zathura-0.0.8.1-3.fc13.src.rpm

Comment 10 Kevin Fenzi 2010-08-15 03:24:29 UTC
rpmlint now tells me: 

zathura.x86_64: E: invalid-desktopfile /usr/share/applications/zathura.desktop value "0.0.8.1" for key "Version" in group "Desktop Entry" is not a known version

The rest looks good. ;)

Comment 11 François Cami 2010-08-15 11:16:21 UTC
I should have caught this one... Thanks!

Spec URL: http://fcami.fedorapeople.org/srpms/zathura.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/zathura-0.0.8.1-4.fc13.src.rpm

Comment 12 Kevin Fenzi 2010-08-15 18:43:09 UTC
Right, the version in there is the version of the desktop file spec, not the application. :) 

I forgot to mention that you need to use desktop-file-install to install the file. :( 
See: https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

Also, I would consider using sed instead of perl in the spec, as sed is much smaller and you won't need a perl Build dependency here, but thats not a blocker, just a matter of taste.

Comment 13 François Cami 2010-08-15 19:01:41 UTC
Both valid concerns :)
Switched to sed, and ran desktop-file-validate on the .desktop file.

Spec URL: http://fcami.fedorapeople.org/srpms/zathura.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/zathura-0.0.8.1-5.fc13.src.rpm

Comment 14 Kevin Fenzi 2010-08-15 19:59:51 UTC
Looks good. All the blockers I see are solved, so this package is APPROVED. 

I will go ahead and sponsor you. 

Welcome to the packaging fun!

Comment 15 François Cami 2010-08-17 20:27:11 UTC
New Package SCM Request
=======================
Package Name: zathura
Short Description: A lightweight PDF viewer
Owners: fcami
Branches: f13 f14 
InitialCC: kevin

Comment 16 Kevin Fenzi 2010-08-17 21:02:27 UTC
Git done (by process-git-requests).

Comment 17 Kevin Fenzi 2010-08-24 03:14:06 UTC
This has been built and pushed to testing, so we can go ahead and close. 

Note that you can also add this bug to the list of bugs fixed in the update and it would have autoclosed it when it went to stable. ;)

Comment 18 François Cami 2012-11-28 21:23:27 UTC
Package Change Request
======================
Package Name: zathura
New Branches: el6

Comment 19 Gwyn Ciesla 2012-11-29 12:00:40 UTC
Misformatted request.

Comment 20 François Cami 2012-11-29 21:25:05 UTC
Sorry for the noise Jon.

Package Change Request
======================
Package Name: zathura
New Branches: el6
Owners: fcami kevin psabata

Comment 21 Gwyn Ciesla 2012-11-29 22:45:51 UTC
Git done (by process-git-requests).


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