Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 233070 - gnome-web-photo: HTML pages thumbnailer
Summary: gnome-web-photo: HTML pages thumbnailer
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-03-20 12:06 UTC by Bastien Nocera
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-09-10 15:57:27 UTC
mclasen: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Bastien Nocera 2007-03-20 12:06:59 UTC
gnome-web-photo contains a thumbnailer that will be used by GNOME applications,
including the file manager, to generate screenshots of web pages.


Packages and spec at:
http://www.gnome.org/~hadess/gnome-web-photo/gnome-web-photo-0.3-1.src.rpm
http://www.gnome.org/~hadess/gnome-web-photo/gnome-web-photo.spec

Comment 1 Bastien Nocera 2007-03-20 12:38:57 UTC
Note that the .spec file changes the filename of "thumbnailer.schemas" to
"gnome-web-photo.schemas". This should probably be done upstream as well.

Comment 2 Peter Gordon 2007-03-21 05:43:33 UTC
Not a full review, but some comments from looking over the spec file:

* For your URL tag, Is there a specific reason for choosing the ftp.acc.umu.se
mirror rather than the round-robin ftp.gnome.org hostname?

* Your Source0 tag should be a fully-qualified URL to the tarball; or if it's a
Fedora-hosted project (wherein this packaging *is* the upstream source), you
should add a comment to note it as such.

* Does it really need the gettext development environment, or does it just use
the gettext utilities for its translation merging while building? If it just
uses the utilities, then please change the "BuildRequires: gettext-devel" to
simply "BuildRequires: gettext"

* You don't need a build-time dependency on libpng-devel, since it's a
dependency of gtk2-devel. Similarly, gnome-vfs2-devel has a depenency on
GConf2-devel, so you should remove libpng-devel and GConf2-devel from your
BuildRequires since they are duplicated entries.

Comment 3 Bastien Nocera 2007-09-05 13:57:54 UTC
(In reply to comment #2)
> Not a full review, but some comments from looking over the spec file:
> 
> * For your URL tag, Is there a specific reason for choosing the ftp.acc.umu.se
> mirror rather than the round-robin ftp.gnome.org hostname?

Fixed

> * Your Source0 tag should be a fully-qualified URL to the tarball; or if it's a
> Fedora-hosted project (wherein this packaging *is* the upstream source), you
> should add a comment to note it as such.

Fixed.

> * Does it really need the gettext development environment, or does it just use
> the gettext utilities for its translation merging while building? If it just
> uses the utilities, then please change the "BuildRequires: gettext-devel" to
> simply "BuildRequires: gettext"

gettext should be enough.

> * You don't need a build-time dependency on libpng-devel, since it's a
> dependency of gtk2-devel. Similarly, gnome-vfs2-devel has a depenency on
> GConf2-devel, so you should remove libpng-devel and GConf2-devel from your
> BuildRequires since they are duplicated entries.

Done.

Fixed packages at:
http://www.gnome.org/~hadess/gnome-web-photo/gnome-web-photo-0.3-2.src.rpm
http://www.gnome.org/~hadess/gnome-web-photo/gnome-web-photo.spec


Comment 4 Matthias Clasen 2007-09-06 05:50:35 UTC
formal review:

rpmlint: 
gnome-web-photo.i386: W: non-conffile-in-etc
/etc/gconf/schemas/gnome-web-photo.schemas

this follows existing practise, thus is ok


gnome-web-photo.i386: W: incoherent-version-in-changelog 0.3.2 0.3-2.fc8

should be fixed


gnome-web-photo.i386: W: invalid-license GPL

must be fixed (see below)


gnome-web-photo.src: W: mixed-use-of-spaces-and-tabs (spaces: line 38, tab: line 38)

unimportant


package name: ok
spec file name: ok  
packaging guidelines: 
  - I don't think there is any reason to use %{__rm}
  - You need a %postun for gconf schemas, I believe
  - The preferred form of the requires is perl(XML::Parser)
license: unclear, COPYING is GPLv2, but the sources all say LGPLv2+, 
  should be clarified upstream
license field: should be updated to match the result of aforementioned   
  clarification
license file: ok
spec file language: ok
spec file legibility: ok
upstream sources: ok
build: fails, see below
excludearch: n/a
build reqs: misses libjpeg-devel
%find_lang: ok
shared library symlinks: n/a
relocatable: n/a
directory ownership: ok
duplicate files: ok
file permissions: ok
%clean: ok
macro use: ok
permissible content: ok
large docs: n/a
%doc content: ok
header files: n/a
static libs: n/a
pc files: n/a
shared libs: n/a
devel package: n/a
libtool archives: n/a
gui apps: ok
directory ownership: ok
%install: ok
utf8 filenames: ok

Comment 5 Bastien Nocera 2007-09-06 14:12:38 UTC
(In reply to comment #4)
> formal review:
<snip>
> gnome-web-photo.i386: W: incoherent-version-in-changelog 0.3.2 0.3-2.fc8
> 
> should be fixed

Typo, fixed.

> gnome-web-photo.i386: W: invalid-license GPL
> 
> must be fixed (see below)

LGPLv2.1+ as per COPYING.README. I believe GPL refers to some of the build
tools/scripts.

> gnome-web-photo.src: W: mixed-use-of-spaces-and-tabs (spaces: line 38, tab:
line 38)
> 
> unimportant

Fixed anyway.

> package name: ok
> spec file name: ok  
> packaging guidelines: 
>   - I don't think there is any reason to use %{__rm}

Fixed.

>   - You need a %postun for gconf schemas, I believe

The schemas is removed in %preun, nothing to do in %postun, as the schemas's
already gone.

>   - The preferred form of the requires is perl(XML::Parser)
> license: unclear, COPYING is GPLv2, but the sources all say LGPLv2+, 
>   should be clarified upstream

See above.

> license field: should be updated to match the result of aforementioned   
>   clarification

Done.

> license file: ok

I put just the COPYING.README, should I also package the COPYING.LGPL?

> build reqs: misses libjpeg-devel

Fixed.

http://www.gnome.org/~hadess/gnome-web-photo/gnome-web-photo-0.3-3.src.rpm
http://www.gnome.org/~hadess/gnome-web-photo/gnome-web-photo.spec

Comment 6 Matthias Clasen 2007-09-06 16:18:40 UTC
For the license: yes, please include the license file that applies to the
source, ie COPYING.LGPL, if it is in the upstream tarball.

The rest looks fine now. Approved.





Comment 7 Bastien Nocera 2007-09-08 10:51:18 UTC
New Package CVS Request
=======================
Package Name: gnome-web-photo
Short Description: HTML pages thumbnailer
Owners: hadess
Branches: 
InitialCC: 
Cvsextras Commits: yes

Comment 8 Kevin Fenzi 2007-09-09 22:50:19 UTC
From the Licensing page, the tag here should be "LGPLv2+" (Thats used for both 2
and 2.1). 

cvs done.

Comment 9 Bastien Nocera 2007-09-10 15:57:27 UTC
(In reply to comment #8)
> From the Licensing page, the tag here should be "LGPLv2+" (Thats used for both 2
> and 2.1). 

Fixed, thanks!

Uploaded as gnome-web-photo-0.3-4.fc8



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