Bug 505917

Summary: Review Request: libpuzzle - Library to quickly find visually similar images
Product: [Fedora] Fedora Reporter: Andrew Colin Kissa <andrew>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, notting, steve.traylen
Target Milestone: ---Flags: mtasaka: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.11-5.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-06-23 16:27:57 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Andrew Colin Kissa 2009-06-14 17:22:27 UTC
Spec URL: http://www.topdog-software.com/oss/SRPMS/fedora/libpuzzle/libpuzzle.spec
SRPM URL: http://www.topdog-software.com/oss/SRPMS/fedora/libpuzzle/libpuzzle-0.11-1.fc11.src.rpm
Description: 
The Puzzle library is designed to quickly find visually similar images
(gif, png, jpg), even if they have been resized, recompressed,
recolored or slightly modified. The library is free, lightweight yet
very fast, configurable, easy to use and it has been designed with
security in mind.

This spec file includes, a PHP extension rpm sub package as well as the devel subpackage.

I do need someone to sponsor the package if they can please.

Comment 1 Steve Traylen 2009-06-16 04:13:47 UTC
Hi Andrew, 

I am not able to sponsor your package but I can hopefully offer some useful
help.

For me the compile won't pass an rpath check as shown below.

rpmbuild -ba libpuzzle-0.11-1.fc11.src.rpm

runs a check since I have in my ~/.rpmmacros

%__arch_install_post   /usr/lib/rpm/check-rpaths   /usr/lib/rpm/check-buildroot

as put there by "rpmdev-setuptree"

Steve



+ /usr/lib/rpm/check-rpaths /usr/lib/rpm/check-buildroot
*******************************************************************************
*
* WARNING: 'check-rpaths' detected a broken RPATH and will cause 'rpmbuild'
*          to fail. To ignore these errors, you can set the '$QA_RPATHS'
*          environment variable which is a bitmask allowing the values
*          below. The current value of QA_RPATHS is 0x0000.
*
*    0x0001 ... standard RPATHs (e.g. /usr/lib); such RPATHs are a minor
*               issue but are introducing redundant searchpaths without
*               providing a benefit. They can also cause errors in multilib
*               environments.
*    0x0002 ... invalid RPATHs; these are RPATHs which are neither absolute
*               nor relative filenames and can therefore be a SECURITY risk
*    0x0004 ... insecure RPATHs; these are relative RPATHs which are a
*               SECURITY risk
*    0x0008 ... the special '$ORIGIN' RPATHs are appearing after other
*               RPATHs; this is just a minor issue but usually unwanted
*    0x0010 ... the RPATH is empty; there is no reason for such RPATHs
*               and they cause unneeded work while loading libraries
*    0x0020 ... an RPATH references '..' of an absolute path; this will break
*               the functionality when the path before '..' is a symlink
*          
*
* Examples:
* - to ignore standard and empty RPATHs, execute 'rpmbuild' like
*   $ QA_RPATHS=$[ 0x0001|0x0010 ] rpmbuild my-package.src.rpm
* - to check existing files, set $RPM_BUILD_ROOT and execute check-rpaths like
*   $ RPM_BUILD_ROOT=<top-dir> /usr/lib/rpm/check-rpaths
*  
*******************************************************************************
ERROR   0001: file '/usr/bin/puzzle-diff' contains a standard rpath '/usr/lib64' in [/usr/lib64]
error: Bad exit status from /var/tmp/rpm-tmp.O9FHYw (%install)

Comment 2 Andrew Colin Kissa 2009-06-16 07:48:51 UTC
Hi Steve,

Thanks for taking time to review my rpm, its very weird that happens when you build, i have tested using koji as well as locally and could not reproduce your error. I have how ever added a fix to the spec, please test again and see if it builds.

Thanks again.

Andrew

Comment 3 Steve Traylen 2009-06-16 08:20:41 UTC
Hi, It builds now.

There some rpmlint errors that need fixing.

$ rpmlint libpuzzle-0.11-1.fc11.x86_64.rpm 
libpuzzle.x86_64: E: standard-dir-owned-by-package /usr/share/man/man3
libpuzzle.x86_64: E: standard-dir-owned-by-package /usr/share/man/man8
libpuzzle.x86_64: E: zero-length /usr/share/doc/libpuzzle-0.11/ChangeLog
libpuzzle.x86_64: E: zero-length /usr/share/doc/libpuzzle-0.11/NEWS

see 

$ rpmlint -I standard-dir-owned-by-package

to get details on the individual messages. In particular for the first one
see the first paragraph of:

https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership

There are few trivial ones as well in the php package.

$ rpmlint php-libpuzzle-0.11-1.fc11.x86_64.rpm 
php-libpuzzle.x86_64: E: zero-length /usr/share/doc/php-libpuzzle-0.11/EXPERIMENTAL
php-libpuzzle.x86_64: W: file-not-utf8 /usr/share/doc/php-libpuzzle-0.11/similar.php
php-libpuzzle.x86_64: W: non-conffile-in-etc /etc/php.d/libpuzzle.ini

Also  I don't think you need or shoudl have the 
Requires: gd 

since this is picked up automatically anyway. Check for php bits as
well but am less sure. See:

https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

Comment 4 Andrew Colin Kissa 2009-06-16 09:01:36 UTC
Hi Steve,

Fixed the issues raised, the rpmlint now comes out clean on my side. If you can give it a run again.

I would appreciate it if you can critic my other submission https://bugzilla.redhat.com/show_bug.cgi?id=505848

Thanks.

Comment 5 Steve Traylen 2009-06-16 10:17:54 UTC
$ rpmlint ../RPMS/x86_64/php-libpuzzle-0.11-1.fc11.x86_64.rpm 
php-libpuzzle.x86_64: W: file-not-utf8 /usr/share/doc/php-libpuzzle-0.11/similar.php
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

still present.

Agreed the libpuzzle-* ones pass just fine.

Reading the INSTALL file since there is "make check" is sensible
or possible to call this after the build?

Maybe the .ini file should be created in the %build stage before 
being installed in the %install stage.

Genrally manipulations of the BUILD area should be in build.
i.e 

%{__rm} -rf php
%{__mv} php-plain php

should move up to %build if possible?

Will look at the php bit later.

Comment 6 Andrew Colin Kissa 2009-06-16 11:16:27 UTC
I have made the changes rpmlint is now clean on all the 3 rpm's.

Comment 7 Mamoru TASAKA 2009-06-18 17:41:48 UTC
Would you post the URLs of your latest spec/srpm?
Note that please change the release number of your spec/srpm every
time you modify your spec file to avoid confusion.

Comment 9 Mamoru TASAKA 2009-06-20 16:24:39 UTC
Some notes:

* chrpath
  - We usually recommend to avoid to use chrpath as the means to
    delete rpath (and to add "chrpath" as BR(BuildRequires)) and 
    instead recommend to try to remove rpath by modifying configure/libtool.
    For this packge:
-----------------------------------------------------------------
%build
%{__cat} <<'EOF' >libpuzzle.ini
extension=libpuzzle.so
EOF
%configure --disable-static
sed -i.rpath -e 's|^\(hardcode_libdir_flag_spec=\).*|\1""|' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool
make %{?_smp_mflags}
.......
.......
cd php/libpuzzle
phpize
%ifarch x86_64 ppc64
LDFLAGS="$LDFLAGS -L%{_builddir}/prelim2/usr/lib64"; export LDFLAGS
%endif

sed -i.rpath -e 's|\$ld_runpath_switch\$ai_p||g' configure
%configure --with-libpuzzle=%{_builddir}/prelim2/usr
sed -i.rpath -e 's|^\(hardcode_libdir_flag_spec=\).*|\1""|' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool
make %{?_smp_mflags}
........
........
-----------------------------------------------------------------
    will remove rpath:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1426380
    Ref:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath

* Requires
  - php-%{name} should have "Requires: php-common" for directory ownership
    issue

* Usage of %_builddir
-----------------------------------------------------------------
make DESTDIR=%{_builddir}/prelim2 install
-----------------------------------------------------------------
  - This is problematic is some other srpm use prelim2 directory.
    Please move this temporary directory to under 
    %_builddir/%{name}-%{version}
    (and using "rm -rf %{_builddir}/prelim2" at clean is not desired)

* Timestamps
  - Add the option 'INSTALL="install -p"' to 'make install' to keep timestamps
    on installed files. This method usually works for Makefiles generated
    by recent autotools.

* Documents
  - There is no need to include the same document files to %doc of
    both libpuzzle and -devel packages.
  - Usually section 3 man pages are to explain functions, api or so
    and should belong to -devel subpackage.

* %changelog
  - It is useful when importing this package into Fedora CVS if you put
    one line between each %changelog entry like:
------------------------------------------------------------------
* Thu Jun 18 2009 Andrew Colin Kissa <andrew.net> - 0.11-2
- Fix rpmlint issues

* Sun May 14 2009 Andrew Colin Kissa <andrew.net> - 0.11-1
- Initial release
------------------------------------------------------------------

Comment 10 Andrew Colin Kissa 2009-06-21 09:46:11 UTC
Hi

Thanks for the review, I have addressed all the points that you raised.

The updated spec and source rpm can be found at.

http://www.topdog-software.com/oss/SRPMS/fedora/libpuzzle/libpuzzle.spec
http://www.topdog-software.com/oss/SRPMS/fedora/libpuzzle/libpuzzle-0.11-3.fc11.src.rpm

Comment 11 Mamoru TASAKA 2009-06-21 19:34:05 UTC
For -3:

* Macros consistency
  - Well, if you want to use %{__cat} or %{__sed} style, please
    use %{__make}, %{__rm} for consistency.

* Timestamps
  - Would you consider to use
---------------------------------------------------------------
make install DESTDIR=%{buildroot} INSTALL="install -p"
---------------------------------------------------------------
    for example to keep timestamps on installed files?

* Misc issue
---------------------------------------------------------------
%files devel
%defattr(-,root,root,-)
%doc 
---------------------------------------------------------------
  - Empty %doc line is unneeded.

Comment 12 Andrew Colin Kissa 2009-06-21 20:08:28 UTC
* Macros consistency - Fixed (__command style used)

* Timestamps - Fixed (install -p used)

* Misc - Fixed (doc line removed from devel package)

Updated Spec: http://www.topdog-software.com/oss/SRPMS/fedora/libpuzzle/libpuzzle.spec
Updated SRPM: http://www.topdog-software.com/oss/SRPMS/fedora/libpuzzle/libpuzzle-0.11-4.fc11.src.rpm

Comment 13 Mamoru TASAKA 2009-06-22 17:36:28 UTC
Okay.
+ This package itself is now okay
+ You have other review requests, which seem fine from very quick
  glance at them

-------------------------------------------------------------------
    This package (libpuzzle) is APPROVED by mtasaka
-------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 10/11, 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.

Removing NEEDSPONSOR.

Comment 14 Andrew Colin Kissa 2009-06-22 18:43:20 UTC
New Package CVS Request
=======================
Package Name: libpuzzle
Short Description: Library to quickly find visually similar images (gif, png,jpg)
Owners: topdog
Branches: F-10 F-11
InitialCC: mtasaka

Comment 15 Kevin Fenzi 2009-06-23 02:24:32 UTC
cvs done.

Comment 16 Fedora Update System 2009-06-23 09:47:33 UTC
libpuzzle-0.11-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/libpuzzle-0.11-4.fc10

Comment 17 Fedora Update System 2009-06-23 09:47:39 UTC
libpuzzle-0.11-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/libpuzzle-0.11-4.fc11

Comment 18 Mamoru TASAKA 2009-06-23 16:27:57 UTC
Now closing.

Comment 19 Andrew Colin Kissa 2009-07-15 11:41:23 UTC
Package Name: libpuzzle
New Branches: EL-5
Owners: topdog

Comment 20 Kevin Fenzi 2009-07-16 05:57:26 UTC
cvs done.

Comment 21 Fedora Update System 2009-07-16 08:43:58 UTC
libpuzzle-0.11-5.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/libpuzzle-0.11-5.el5

Comment 22 Fedora Update System 2009-07-19 10:08:53 UTC
libpuzzle-0.11-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2009-07-19 10:35:56 UTC
libpuzzle-0.11-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2009-08-04 02:27:50 UTC
libpuzzle-0.11-5.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.