Bug 314871 - Review Request: gimp-lqr-plugin - GIMP LiquidRescale Plug-In
Summary: Review Request: gimp-lqr-plugin - GIMP LiquidRescale Plug-In
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 428916 (view as bug list)
Depends On: 429202
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-10-02 00:36 UTC by Alexandru Ciobanu
Modified: 2008-03-19 17:35 UTC (History)
9 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-03-03 22:16:19 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Alexandru Ciobanu 2007-10-02 00:36:41 UTC
Spec URL: http://tvtransilvania.ro/ics/Fedora/8/gimp-lqr-plugin.spec
SRPM URL: http://tvtransilvania.ro/ics/Fedora/8/gimp-lqr-plugin-0.2.1-1.fc8.src.rpm
Description: 
This package is a plug-in for GIMP 2.2 and newer. It implements the algorithm
described in the paper "Seam Carving for Content-Aware Image Resizing"
by Shai Avidan and Ariel Shamir, which can be found at
http://www.faculty.idc.ac.il/arik/imret.pdf

This is my first package and I am seeking a sponsor.

Comment 1 Marc Bradshaw 2007-10-15 02:21:53 UTC
Hi Alexandru,

As I am unable to sponsor you this is an informal (and incomplete) review but
does hi-light some issues which need to be fixed.


?? -  Meets Packaging Guidelines.
?? -  Package named correctly
NA -  Patches named correctly
OK -  Spec file named correctly to match base
OK -  License is valid
OK -  Licence field matches package
OK -  Licence file installed if supplied
OK -  Spec file in American English
?? -  Source matches upstream (md5)
NO *  Locales use %find_lang
OK -  %clean is present and correct
OK -  Package has correct buildroot.
OK -  Specfile Legible
NO *  Builds in Mock
NA -  %post/%postun calls ldconfig for sh libs
OK -  Owns directories it creates
OK -  No duplicate files
NO *  Has %defattr and has correct permissions
NO *  Macros used consistently
OK -  %doc does not affect runtime
NA -  Headers/static libs in -devel
NA -  .pc files in -devel
NA -  .so files in -devel
NA -  -devel requires base
OK -  Contains no .la libtool archive files
OK -  Does not own others files
NA -  .desktop files installed correctly
NO *  BuildRequires correct.
?? -  Package is code or permissible content.
OK -  Package has rm -rf %{buildroot} at top of %install.
      $RPM_BUILD_ROOT used instead
?? -  Package compiles and builds on at least one arch.
?? -  rpmlint output.
NA -  documentation in -doc package
?? -  final provides and requires are sane.
OK -  should have dist tag
?? -  should package latest version

Comments and Suggestions. 

Summary Field should not be in camel caps, in my opinion it would be better as
"Liquid rescale plug-in for the GIMP"
or something more descriptive like "Content-aware resizing plug-in for the GIMP"

The description does not state what the plugin actually does.

Please use the ${name} macrom in the Source0: URL:

%defattr MUST be at top of %files section

Group: would be better as Applications/Multimedia

There is a typo in the URL field.

Package fails to build in mock.
Please see http://marcbradshaw.co.uk/packages/review/gimp-lqr-plugin/ for build logs

Please see the Locale section of
http://fedoraproject.org/wiki/Packaging/Guidelines


-Marc

Comment 2 Jason Tibbitts 2007-11-21 21:16:28 UTC
Was there any response to this?  It's been over a month, and the above
commentary needs to be addressed.

Comment 3 Alexandru Ciobanu 2007-11-22 18:21:49 UTC
Hi Marc,

I've followed your pointers to resolve the issues. I'm very new when it comes to
mock, so I don't have a fully buildsys right now (I'm still studying) and didn't
test if it builds in mock.

Spec URL: http://tvtransilvania.ro/ics/Fedora/8/gimp-lqr-plugin.spec
SRPM URL: http://tvtransilvania.ro/ics/Fedora/8/gimp-lqr-plugin-0.3.0-1.fc8.src.rpm



Comment 4 Alexandru Ciobanu 2007-11-22 19:30:18 UTC
I've just successfully completed a mock build too.

Comment 6 Mamoru TASAKA 2007-12-09 13:33:05 UTC
ping?

Comment 7 Alexandru Ciobanu 2007-12-10 14:13:41 UTC
It builds OK on my rawhide machine, i386. Am I doing something wrong?

http://tvtransilvania.ro/ics/Fedora/devel/build.log
http://tvtransilvania.ro/ics/Fedora/devel/gimp-lqr-plugin-0.3.0-1.fc9.src.rpm

Comment 8 Mamoru TASAKA 2007-12-10 16:22:36 UTC
(In reply to comment #7)
> It builds OK on my rawhide machine, i386. Am I doing something wrong?
  You actually modified your spec file :)
---------------------------------------------------
@@ -11,7 +8,7 @@
 Source0:       
http://liquidrescale.wikidot.com/local--files/en:download-page/%{name}-%{version}.tar.gz
 BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-%(%{__id_u} -n)
 
-BuildRequires:  gimp, gimp-devel >= 2.2.13, perl-XML-Parser
+BuildRequires:  gimp, gimp-devel >= 2.2.13, perl-XML-Parser, gettext
 Requires:       gimp >= 2.2.13, perl-XML-Parser
 
 %description
---------------------------------------------------
  Now your srpm builds.
  http://koji.fedoraproject.org/koji/taskinfo?taskID=285397

! Note
  - From next time please change the release number of your
    spec file every time you modify your spec file.

For 0.3.0-1.fc9:
* Perl modules dependency
  c.f. http://fedoraproject.org/wiki/Packaging/Perl
  - When writing perl modules dependency, please write the
    module name as dependency, not writing the rpm name
    directly, for example:
---------------------------------------------------
BuildRequires: perl(XML::Parser)

* Tarball, versioning
  - The written Source0 tarball could not found. Instead I found
   
http://liquidrescale.wikidot.com/local--files/en:download-page/gimp-lqr-plugin-0.3.0-6.tar.gz

    Umm... In this case, we regard the version as "0.3.0.6" as
    rpm "Version" does not accept hyphen.

* CFLAGS
----------------------------------------------------
make %{?_smp_mflags} CFLAGS="%{optflags}"
----------------------------------------------------
  - Please check if this CFLAGS is really needed.
    %configure sets CFLAGS as environ (check what %configure actually
    does by `rpm --eval %configure`) and most recent Makefiles
    treats this properly.

* Timestamps
  - Please check if the following method keeps timestamps on
    installed files
----------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
----------------------------------------------------
    While sometimes this does not work, this usually works
    on recent Makefiles.

* Documents
  - Please add the following documents to %doc.
----------------------------------------------------
AUTHORS
NEWS
----------------------------------------------------

* Directory ownership issue:
  - Please consider the ownership issue of directories.
----------------------------------------------------
%files
%{_datadir}/gimp-lqr-plugin/help/en/gimp-help.xml
%{_datadir}/gimp-lqr-plugin/help/en/index.html
%{_datadir}/gimp-lqr-plugin/help/images/dialog1.png
%{_datadir}/gimp-lqr-plugin/help/images/dialog2.png
.......
----------------------------------------------------

   This causes the following problem, for example.
----------------------------------------------------
[tasaka1@localhost ~]$ rpm -qf /usr/share/gimp-lqr-plugin/help/en/index.html 
gimp-lqr-plugin-0.3.0-1.fc9
[tasaka1@localhost ~]$ rpm -qf /usr/share/gimp-lqr-plugin/help/en/           
file /usr/share/gimp-lqr-plugin/help/en is not owned by any package
[tasaka1@localhost ~]$ rpm -qf /usr/share/gimp-lqr-plugin/help/   
file /usr/share/gimp-lqr-plugin/help is not owned by any package
[tasaka1@localhost ~]$ rpm -qf /usr/share/gimp-lqr-plugin/     
file /usr/share/gimp-lqr-plugin is not owned by any package
----------------------------------------------------
    Here
    - Installing gimp-lpr-plugin binary rpm creates the directories
      /usr/share/gimp-lpr-plugin, .../help, .../help/en to install
      index.html under the last directory
    - But these directories left unowned by any package.
    This autually causes some problems.

    ! Note
    - When we write
------------------------------------------------------
%files
%{_datadir}/gimp-lqr-plugin/
------------------------------------------------------
      This contains the directory %_datadir/gimp-lpr-plugin itself
      _and_ all files/directories/etc under %_datadir/gimp-lpr-plugin,
      where
------------------------------------------------------
%files
%dir %{_datadir}/gimp-lqr-plugin/
------------------------------------------------------
      contains the directory %_datadir/gimp-lqr-plugin only.


Comment 9 Mamoru TASAKA 2007-12-18 16:59:20 UTC
ping?

Comment 10 Mamoru TASAKA 2007-12-25 18:25:53 UTC
ping again?

Comment 11 Mamoru TASAKA 2008-01-06 15:27:18 UTC
I will close this bug as NOTABUG if no response from the reporter is
received by 2008-01-17.

Comment 12 Ewan Mac Mahon 2008-01-16 00:57:29 UTC
I've had a completely independent (and slightly different) package of this 
plugin sat around for a while pending some tidy ups. I've got it into a 
presentable state and submitted a review request as bug #428916; if the OP of 
this package doesn't reappear it may be worth considering that as an 
alternative.

Comment 13 Alexandru Ciobanu 2008-01-16 12:41:56 UTC
The OP just finished his vacation and will build the new lqr plugin tonight. 
Alex

Comment 14 Alexandru Ciobanu 2008-01-17 22:02:48 UTC
Spec URL: http://tvtransilvania.ro/ics/Fedora/9/gimp-lqr-plugin.spec
SRPM URL: http://tvtransilvania.ro/ics/Fedora/9/gimp-lqr-plugin-0.4.0-1.fc9.src.rpm

I have also commited a review request for the lqr library: BZ #429202

Comment 15 Mamoru TASAKA 2008-01-18 16:27:16 UTC
Well, I also checked liblqr-1 and

* liblqr-1 (bug 429202) is actually licensed under GPLv3 (strict).
* This package gimp-lpr-plugin is licensed under GPLv2 (strict)
* gimp-lpr-plugin depends on liblqr-1
* GPLv2 (strict) and GPLv3 (strict) are incompatible....

Setting FE-Legal

It seems that the upstreams of the both softwares are same.

Comment 16 Tom "spot" Callaway 2008-01-18 17:23:33 UTC
Perhaps, given that the upstreams are identical, they could sync up the licenses
to match? :)

Comment 17 Mamoru TASAKA 2008-01-31 07:39:34 UTC
Are there any response from upstream?

Comment 18 Alexandru Ciobanu 2008-01-31 14:44:34 UTC
Carlo Baldassi wrote:
> Ok, I have changed the licensing of the plugin from "GPLv2 strict" to
> "GPLv2 or later", which I hope solves the issue.

I've been studying the licenses and as far as I can tell the two packages are
now fully compatible. I'll post SRPMS later today.

Regards,
Alex

Comment 19 Mamoru TASAKA 2008-02-12 12:04:49 UTC
ping again?

Comment 20 David Fraser 2008-02-19 08:24:31 UTC
This spec file needs to be adjusted to reflect the 0.4.0-4 tarball versioning
done by gimp-lqr-plugin. In addition it should include BuildRequires: lqr-1-devel

Adjusted spec file at http://davidf.sjsoft.com/files/gimp-lqr-plugin.spec

This builds installs and runs fine on my Fedora 8 i386 system.

These improvements are from https://bugzilla.redhat.com/show_bug.cgi?id=428916
which is a duplicate package request

Comment 21 Alexandru Ciobanu 2008-02-19 20:32:11 UTC
New packages with all the issues fixed(hopefully).
Spec URL: http://tvtransilvania.ro/ics/Fedora/devel/gimp-lqr-plugin.spec
SRPM URL:
http://tvtransilvania.ro/ics/Fedora/devel/gimp-lqr-plugin-0.4.0-3.fc9.src.rpm

Comment 22 Mamoru TASAKA 2008-02-20 05:41:37 UTC
Still gimp-lqr-plugin is GPLv2 strict and liblqr-1 (bug 429202)
is GPLv3. Note that we are not judging the license issues by the 
license tags in the spec files, but by actually checking the source
tarballs.

Would you ask the upstream to actually release the new version of
gimp-lqr-plugin tarball to fix the license conflict from liblqr-1?

Comment 23 Mamoru TASAKA 2008-02-20 05:48:52 UTC
By the way the reason we are saying that gimp-lqr-plugin is actually
licensed under strict GPLv2 is that the source codes under src directory
actually says they are licensed under GPLv2 only.

Comment 25 Mamoru TASAKA 2008-02-20 15:47:00 UTC
Okay, now gimp-lqr-plugin is under GPLv2+. Removing FE-Legal.

Comment 26 Mamoru TASAKA 2008-02-29 16:23:26 UTC
Once liblqr-1 is imported on devel branch, I will review this.

Comment 27 Mamoru TASAKA 2008-03-02 18:26:08 UTC
For 0.4.0.4-1:

* Requires
  - Requires: liblqr-1 >= 0.1.0 is not needed. Library dependencies
    like these are automatically checked and added by rpmbuild itself.

  - Requires: perl(XML::Parser) is not needed. This is used only
    for BuildRequires.

* BuildRequires
  - "BuildRequres: gimp" is not needed ("BuildRequires: gimp-devel"
    is needed)

* Directoy ownership issue
  - The directory %{_datadir}/gimp-lqr-plugin is not owned by
    any package.

* %changelog
  - It is preferable you write %changelog as:
---------------------------------------------------------
* Wed Feb 20 2008 Alexandru Ciobanu <alex> - 0.4.0.4-1
- Remode hyphen from versioning.

* Tue Feb 19 2008 Alexandru Ciobanu <alex> - 0.4.0-3
- Fixed licensing issue.

* Mon Feb 4 2008 Alexandru Ciobanu <alex> - 0.4.0-2
---------------------------------------------------------

Comment 29 Mamoru TASAKA 2008-03-03 14:25:32 UTC
Okay.

-----------------------------------------------------------------------
   This package (gimp-lqr-plugin) is APPROVED by me
-----------------------------------------------------------------------

Comment 30 Alexandru Ciobanu 2008-03-03 15:15:40 UTC
New Package CVS Request
=======================
Package Name: gimp-lqr-plugin
Short Description: Content-aware resizing plug-in for the GIMP
Owners: ics
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes

Comment 31 Kevin Fenzi 2008-03-03 20:00:34 UTC
cvs done.

Comment 32 Mamoru TASAKA 2008-03-19 17:35:23 UTC
*** Bug 428916 has been marked as a duplicate of this bug. ***


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