Bug 314871 - Review Request: gimp-lqr-plugin - GIMP LiquidRescale Plug-In
Review Request: gimp-lqr-plugin - GIMP LiquidRescale Plug-In
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
: 428916 (view as bug list)
Depends On: 429202
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-01 20:36 EDT by Alexandru Ciobanu
Modified: 2008-03-19 13:35 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-03 17:16:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Alexandru Ciobanu 2007-10-01 20:36:41 EDT
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-14 22:21:53 EDT
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 16:16:28 EST
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 13:21:49 EST
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 14:30:18 EST
I've just successfully completed a mock build too.
Comment 6 Mamoru TASAKA 2007-12-09 08:33:05 EST
ping?
Comment 7 Alexandru Ciobanu 2007-12-10 09:13:41 EST
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 11:22:36 EST
(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 11:59:20 EST
ping?
Comment 10 Mamoru TASAKA 2007-12-25 13:25:53 EST
ping again?
Comment 11 Mamoru TASAKA 2008-01-06 10:27:18 EST
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-15 19:57:29 EST
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 07:41:56 EST
The OP just finished his vacation and will build the new lqr plugin tonight. 
Alex
Comment 14 Alexandru Ciobanu 2008-01-17 17:02:48 EST
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 11:27:16 EST
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 12:23:33 EST
Perhaps, given that the upstreams are identical, they could sync up the licenses
to match? :)
Comment 17 Mamoru TASAKA 2008-01-31 02:39:34 EST
Are there any response from upstream?
Comment 18 Alexandru Ciobanu 2008-01-31 09:44:34 EST
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 07:04:49 EST
ping again?
Comment 20 David Fraser 2008-02-19 03:24:31 EST
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 15:32:11 EST
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 00:41:37 EST
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 00:48:52 EST
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 10:47:00 EST
Okay, now gimp-lqr-plugin is under GPLv2+. Removing FE-Legal.
Comment 26 Mamoru TASAKA 2008-02-29 11:23:26 EST
Once liblqr-1 is imported on devel branch, I will review this.
Comment 27 Mamoru TASAKA 2008-03-02 13:26:08 EST
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@tvtransilvania.ro> - 0.4.0.4-1
- Remode hyphen from versioning.

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

* Mon Feb 4 2008 Alexandru Ciobanu <alex@tvtransilvania.ro> - 0.4.0-2
---------------------------------------------------------
Comment 29 Mamoru TASAKA 2008-03-03 09:25:32 EST
Okay.

-----------------------------------------------------------------------
   This package (gimp-lqr-plugin) is APPROVED by me
-----------------------------------------------------------------------
Comment 30 Alexandru Ciobanu 2008-03-03 10:15:40 EST
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 15:00:34 EST
cvs done.
Comment 32 Mamoru TASAKA 2008-03-19 13:35:23 EDT
*** 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.