Bug 233070 - gnome-web-photo: HTML pages thumbnailer
gnome-web-photo: HTML pages thumbnailer
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-20 08:06 EDT by Bastien Nocera
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-10 11:57:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mclasen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Bastien Nocera 2007-03-20 08:06:59 EDT
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 08:38:57 EDT
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 01:43:33 EDT
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 09:57:54 EDT
(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 01:50:35 EDT
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 10:12:38 EDT
(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 12:18:40 EDT
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 06:51:18 EDT
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 18:50:19 EDT
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 11:57:27 EDT
(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.