Bug 528782 - Review Request: xcf-pixbuf-loader - XCF (GIMP) image loader for GTK+ applications
Summary: Review Request: xcf-pixbuf-loader - XCF (GIMP) image loader for GTK+ applicat...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Benjamin Otte
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-10-13 17:33 UTC by Bastien Nocera
Modified: 2016-08-14 15:54 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-08-14 15:54:46 UTC
Type: ---
Embargoed:
otte: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Bastien Nocera 2009-10-13 17:33:24 UTC
Spec URL: http://people.fedoraproject.org/~hadess/xcf-pixbuf-loader/xcf-pixbuf-loader.spec
SRPM URL: http://people.fedoraproject.org/~hadess/xcf-pixbuf-loader/xcf-pixbuf-loader-0.0.1-1.e5ce761.fc11.src.rpm
Description: 
xcf-pixbuf-loader contains a plugin to load XCF images, as created by
the GIMP, in GTK+ applications.

Comment 1 Debarshi Ray 2009-12-05 14:47:52 UTC
I think it should be %{_target_platform} instead of %{_host} because on Fedora 11 x86_64:

[rishi@freebook SPECS]$ rpm  --eval %{_target_platform}
x86_64-redhat-linux-gnu
[rishi@freebook SPECS]$ rpm  --eval %{_host}
x86_64-unknown-linux-gnu
[rishi@freebook SPECS]$ ls /etc/gtk-2.0/
gtkrc  x86_64-redhat-linux-gnu
[rishi@freebook SPECS]$

Comment 2 Debarshi Ray 2009-12-05 23:10:43 UTC
I think your %postun scriptlet should be:
if [ $1 -eq 0 ]; then

See: http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax

Comment 3 Benjamin Otte 2010-01-25 21:57:57 UTC
I'll review this one.

Some comments:
- I believe you should run update-gdk-pixbuf-loaders unconditionally in %postun. Only gtk shouldn't run it after uninstall, all other pixbuf loader providing packages do.
- Is there a reason for not just running make install with the patch you included?
- If you updated to git master, that patch would even be included already (hint hint)
- I suppose Stephane doesn't want to make releases for this but rather wants to try to get it into gtk proper? So there's no use waiting for a release?

I'll try to actually build it and run the checklist tomorrow

Comment 4 Benjamin Otte 2010-01-28 12:06:52 UTC
Reviewed this. So the problems are:

- postun should run update-gdk-pixbuf-loaders unconditionally

- rpmlint:
$ rpmlint xcf-pixbuf-loader.spec 
xcf-pixbuf-loader.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 19, tab: line 1)
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

- License says "LGPL" but should be "LGPLv2+"

- The correct static URL for the tarball is http://gitorious.org/xcf-pixbuf-loader/mainline/archive-tarball/e5ce7614 (The tarball is then generated and you need to redownload a minute or so later)

The rest is fine.

Comment 5 Bastien Nocera 2010-02-17 15:47:11 UTC
(In reply to comment #4)
> Reviewed this. So the problems are:
> 
> - postun should run update-gdk-pixbuf-loaders unconditionally

Done.

> - rpmlint:
> $ rpmlint xcf-pixbuf-loader.spec 
> xcf-pixbuf-loader.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 19, tab:
> line 1)
> 0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Fixed

> - License says "LGPL" but should be "LGPLv2+"

Fixed

> - The correct static URL for the tarball is
> http://gitorious.org/xcf-pixbuf-loader/mainline/archive-tarball/e5ce7614 (The
> tarball is then generated and you need to redownload a minute or so later)
> 
> The rest is fine.    

Pointed to that now.

(In reply to comment #3)
> I'll review this one.
> 
> Some comments:
> - I believe you should run update-gdk-pixbuf-loaders unconditionally in
> %postun. Only gtk shouldn't run it after uninstall, all other pixbuf loader
> providing packages do.

Done.

> - Is there a reason for not just running make install with the patch you
> included?

Because it would install in %{_libdir}, not the subdir. There's no easy ways to detect where it should be installed.

> - If you updated to git master, that patch would even be included already (hint
> hint)

Ya. Done.

> - I suppose Stephane doesn't want to make releases for this but rather wants to
> try to get it into gtk proper? So there's no use waiting for a release?

Nope, no real point.

> I'll try to actually build it and run the checklist tomorrow    

(In reply to comment #1)
> I think it should be %{_target_platform} instead of %{_host} because on Fedora
> 11 x86_64:
> 
> [rishi@freebook SPECS]$ rpm  --eval %{_target_platform}
> x86_64-redhat-linux-gnu
> [rishi@freebook SPECS]$ rpm  --eval %{_host}
> x86_64-unknown-linux-gnu
> [rishi@freebook SPECS]$ ls /etc/gtk-2.0/
> gtkrc  x86_64-redhat-linux-gnu
> [rishi@freebook SPECS]$    

GTK+ uses %{_host}, so should we.

Updated here:
http://people.fedoraproject.org/~hadess/xcf-pixbuf-loader/xcf-pixbuf-loader.spec
http://people.fedoraproject.org/~hadess/xcf-pixbuf-loader/xcf-pixbuf-loader-0.0.1-2.8af913d1.fc12.src.rpm

Comment 6 Benjamin Otte 2010-02-18 10:17:13 UTC
Looks good.

Comment 7 Bastien Nocera 2010-02-18 10:31:26 UTC
New Package CVS Request
=======================
Package Name: xcf-pixbuf-loader
Short Description: XCF (GIMP) image loader for GTK+ applications
Owners: hadess
Branches: F-12
InitialCC:

Comment 8 Jason Tibbitts 2010-02-19 18:57:30 UTC
CVS done (by process-cvs-requests.py).

Also added an F-13 branch.


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