[Bug 528782] Review Request: xcf-pixbuf-loader - XCF (GIMP) image loader for GTK+ applications

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=528782

Bastien Nocera <bnocera at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?(bnocera at redhat.co |
                   |m)                          |

--- Comment #5 from Bastien Nocera <bnocera at redhat.com> 2010-02-17 10:47:11 EST ---
(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 at freebook SPECS]$ rpm  --eval %{_target_platform}
> x86_64-redhat-linux-gnu
> [rishi at freebook SPECS]$ rpm  --eval %{_host}
> x86_64-unknown-linux-gnu
> [rishi at freebook SPECS]$ ls /etc/gtk-2.0/
> gtkrc  x86_64-redhat-linux-gnu
> [rishi at 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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.