Bug 429202 - Review Request: liblqr-1 - LiquidRescale library
Summary: Review Request: liblqr-1 - LiquidRescale library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: i386
OS: Linux
low
low
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 314871
TreeView+ depends on / blocked
 
Reported: 2008-01-17 21:59 UTC by Alexandru Ciobanu
Modified: 2008-03-02 16:34 UTC (History)
5 users (show)

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


Attachments (Terms of Use)
Patch for pkgconfig .pc file to remove unneeded directory (372 bytes, patch)
2008-02-25 18:57 UTC, Mamoru TASAKA
no flags Details | Diff

Description Alexandru Ciobanu 2008-01-17 21:59:48 UTC
Spec URL: http://tvtransilvania.ro/ics/Fedora/9/liblqr.spec
SRPM URL: http://tvtransilvania.ro/ics/Fedora/9/liblqr-1-0.1.0-1.fc9.i386.rpm

The LiquidRescale (lqr) library provides a C/C++ API for
performing non-uniform resizing of images by the seam-carving
technique.

This package is a part of the LiquidRescale plugin for The GIMP.

Comment 1 Jason Tibbitts 2008-01-17 22:05:52 UTC
The SRPM URL does not point to an srpm.

Comment 2 Alexandru Ciobanu 2008-01-17 22:09:28 UTC
Pardon me.
SRPM:http://tvtransilvania.ro/ics/Fedora/9/liblqr-1-0.1.0-1.fc9.src.rpm




Comment 3 Mamoru TASAKA 2008-01-18 16:21:13 UTC
The license of this software is actually GPLv3. It is okay
for this software, however this causes a problem on gimp-lqr-plugin
(see bug 314871)

Comment 4 Mamoru TASAKA 2008-01-18 16:28:21 UTC
Actually I want to know why the upstream released this as GPLv3
before checking this package.

Comment 5 Mamoru TASAKA 2008-01-31 14:59:56 UTC
Once the license issue on bug 314871 is resolved, I will try to
start to check this package.

Comment 7 Mamoru TASAKA 2008-02-20 17:47:50 UTC
For general packaging guidelines you can refer to:
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines

For 0.1.0-2:

* spec file name
  - must coincide with the name of main package, i.e.
    spec file must have the name "liblqr-1.spec".

* Group tag
  - Usually the library related rpm has 
    "Group: System Environment/Libraries" and its -devel
    subpackage usually has
    "Group: Development/Libraries".

* Requires/BuildRequires
  - "BuildRequires: glib" is not needed. GIMP uses glib2,
    liblqr-1 also uses glib2 not glib (GLib 1).
    Also liblqr-1 has "BuildRequies: gimp-devel", which 
    also pulls glib2 automatically.

  - "Requires: pkgconfig" must be removed from main package.
    The main package is not for development and pkgconfig must
    not be needed.

  - Also "Requires: gimp" is not needed for main package.
    This library does not need gimp itself, only gimp-libs
    is needed and such dependency are automatically treated
    by rpmbuild.

  - -devel subpackage does not need "Requires: gimp-devel".
    From installed pkgconfig .pc file and header files,
    only "glib2-devel" (and also pkgconfig) is needed as
    Requires for -devel subpackage

  - Also -devel subpackage must have
    "Requires: %{name} = %{version}-%{release}"

* License
  - For this package there is no reason that -devel
    subpackage should have different License tag.

* macros usage consistency
  - If you want to use %__rm , please also use %__make
    %__install for consistency.

* symlink for linkage
  - %{_libdir}/libXXXX.so should be in -devel package
    (%{_libdir}/libXXXX.so.X symlink should be in main package).
    This type of errors can be detected by using rpmlint
    commands (in rpmlint package). 
-------------------------------------------------------------
[tasaka1@localhost liblqr-1]$ rpmlint *0.1.0-2*rpm
liblqr-1.i386: W: devel-file-in-non-devel-package /usr/lib/liblqr-1.so
.......
-------------------------------------------------------------

* Directory ownership issue
  - Please make it sure that all directories which are created
    when installing a rpm are owned by the rpm.
    For this package the directory %{_includedir}/lqr-1/,
   %{_includedir}/lpr must be owned by -devel subpackage.

  ! By the way,
    when you write:
-------------------------------------------------------------
%files devel
%{_includedir}/lqr-1/
-------------------------------------------------------------
    This contains the directory %{_includedir}/lqr-1 and
    all files/directories/etc under %{_includedir}/lqr-1.

Comment 9 Mamoru TASAKA 2008-02-21 17:23:34 UTC
For 0.1.0-3:

* Description
  - The %description of -devel package is wrong.
--------------------------------------------------------------
%description devel
The libqr-devel package contains the static libraries and header files
                                     ^^^^^^^^^^^^^^^^
--------------------------------------------------------------

* .so symlink location
(In reply to comment #7)
> * symlink for linkage
>   - %{_libdir}/libXXXX.so should be in -devel package
>     (%{_libdir}/libXXXX.so.X symlink should be in main package).
>     This type of errors can be detected by using rpmlint
>     commands (in rpmlint package). 
> -------------------------------------------------------------
> [tasaka1@localhost liblqr-1]$ rpmlint *0.1.0-2*rpm
> liblqr-1.i386: W: devel-file-in-non-devel-package /usr/lib/liblqr-1.so
> .......
> -------------------------------------------------------------

  - Please check your rpms by rpmlint to detect generic packaging errors.
    For newest rpm:
---------------------------------------------------------------
liblqr-1-devel.i386: E: library-without-ldconfig-postin /usr/lib/liblqr-1.so.0.0.0
liblqr-1-devel.i386: E: library-without-ldconfig-postun /usr/lib/liblqr-1.so.0.0.0
---------------------------------------------------------------
    What I said was:
    - %_libdir/liblqr-1.so.0.0.0 and %_libdir/liblqr-1.so.0 must be
       in main package
    - %_libdir/liblqr-1.so must be moved to -devel package.

* BuildRequires
  - Well, I rechecked BuildRequires and actually glib2-devel is
    sufficient for BuildRequires (gimp-devel is not needed for
    BuildRequires)

* Typo
  - Please fix below (by the way with this typo rebuild actually
    fails....)
---------------------------------------------------------------
find %buildroot -name \*.la -exec %rm -f {} \;
                                  ^^^^
---------------------------------------------------------------

Comment 10 Alexandru Ciobanu 2008-02-25 09:29:31 UTC
Thanks Mamoru for your time, here are the new files.
Spec URL: http://tvtransilvania.ro/ics/Fedora/devel/liblqr-1.spec
SRPM URL: http://tvtransilvania.ro/ics/Fedora/devel/liblqr-1-0.1.0-4.fc9.src.rpm

Comment 11 Mamoru TASAKA 2008-02-25 18:57:29 UTC
Created attachment 295827 [details]
Patch for pkgconfig .pc file to remove unneeded directory

For 0.1.0-4:

3 issues 

* undefined non-weak symbols
-----------------------------------------------------------------
[root@localhost liblqr-1]# rpmlint liblqr-1 liblqr-1-devel
liblqr-1.i386: W: undefined-non-weak-symbol /usr/lib/liblqr-1.so.0.0.0 g_free
liblqr-1.i386: W: undefined-non-weak-symbol /usr/lib/liblqr-1.so.0.0.0 sqrt
liblqr-1.i386: W: undefined-non-weak-symbol /usr/lib/liblqr-1.so.0.0.0 log
liblqr-1.i386: W: undefined-non-weak-symbol /usr/lib/liblqr-1.so.0.0.0 exp
liblqr-1.i386: W: undefined-non-weak-symbol /usr/lib/liblqr-1.so.0.0.0
g_try_malloc
liblqr-1.i386: W: undefined-non-weak-symbol /usr/lib/liblqr-1.so.0.0.0
g_try_malloc0
-----------------------------------------------------------------
  - liblqr-1.so contains undefined non-weak symbols.
    You can check this by $ ldd -r /usr/lib/liblqr-1.so.

    As this library is to be used for linkage from other packages,
    such symbols cannot be allowed because leaving such symbols
    causes linkage failure.

    The following fixes this issue:
------------------------------------------------------------------
%build
export LDFLAGS="`pkg-config --libs glib-2.0` -lm"
%configure
%{__make} %{?_smp_mflags}
------------------------------------------------------------------

* Unneeded libdir in .pc file
  - On i386, /usr/lib/pkgconfig/lqr-1.pc  contains the line:
------------------------------------------------------------------
    10	Libs: -L${libdir}/lqr-1 -llqr-1
------------------------------------------------------------------
    But no libraries are installed under %{_libdir}/lqr-1 so
    -L${libdir}/lqr-1 is unneed. Please apply the patch attached.

* %buildroot v.s. $RPM_BUILD_ROOT
  - Please choose one and don't use both.

Comment 12 Alexandru Ciobanu 2008-02-29 11:32:33 UTC
Spec URL: http://tvtransilvania.ro/ics/Fedora/devel/liblqr-1.spec
SRPM URL: http://tvtransilvania.ro/ics/Fedora/devel/liblqr-1-0.1.0-5.fc9.src.rpm

I have a clean rpmlint report now.

Comment 13 Mamoru TASAKA 2008-02-29 16:22:27 UTC
Please change the permission of files in srpm to 0644.

- This package (liblqr-1) is okay
- Your another review request (bug 314871) needs fixing on some points,
  however I hope it can be approved after some fixes.

-------------------------------------------------------------------------
    This package (liblqr-1) is APPROVED by me
-------------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor. At the stage, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 7/8, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.


Comment 14 Mamoru TASAKA 2008-03-01 13:43:43 UTC
Now I am sponsoring you. Please follow "Join" wiki again.

Comment 15 Alexandru Ciobanu 2008-03-01 14:24:55 UTC
New Package CVS Request
=======================
Package Name: liblqr-1
Short Description: LiquidRescale library
Owners: ics
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes

Comment 16 Kevin Fenzi 2008-03-01 18:34:17 UTC
cvs done.

Comment 17 Mamoru TASAKA 2008-03-02 16:34:13 UTC
Please don't forget to rebuild this package also on F-7/8.
Also, for F-7/8, please request the rebuilt packages to push to
stable on:
https://admin.fedoraproject.org/updates/


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