Bug 477338 - Review Request: php-pecl-imagick - Provides a wrapper to the ImageMagick library
Summary: Review Request: php-pecl-imagick - Provides a wrapper to the ImageMagick library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-12-20 17:48 UTC by Pavel Alexeev
Modified: 2009-03-23 15:56 UTC (History)
3 users (show)

Fixed In Version: 2.2.2-1.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-02-24 20:42:58 UTC
Type: ---
Embargoed:
fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Pavel Alexeev 2008-12-20 17:48:35 UTC
Spec URL: http://hubbitus.net.ru/rpm/Fedora9/php-pecl-imagick/php-pecl-imagick.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora9/php-pecl-imagick/php-pecl-imagick-2.2.1-1.fc9.src.rpm
Description:
php-pecl-imagick is a native php extension to create and modify images using the ImageMagick API.

Rpmlint produces only this errors:
php-pecl-imagick.athlon: E: zero-length /usr/share/doc/php-pecl-imagick-2.2.1/INSTALL
php-pecl-imagick.athlon: E: zero-length /usr/share/doc/php-pecl-imagick-2.2.1/TODO

But this files has zero length in upstream, so I thought this may be safely ignored.

Koji build are successful: http://koji.fedoraproject.org/koji/taskinfo?taskID=1012208

Comment 1 Remi Collet 2008-12-28 16:46:21 UTC
A few notes :

- License is PHP, not BSD (according to pecl.php.net)
- must use %setup -q -c (to not have package.xml outside the build tree)
- missing require for ABI check : php(zend-abi)
- should use %{pecl_install} and  %{pecl_uninstall} when exists
- why PEAR in sumnary ?
- should add example directory in %doc (rather than each files)

Read : http://fedoraproject.org/wiki/Packaging/PHP

Comment 2 Remi Collet 2008-12-28 16:53:53 UTC
Also 
- use %{php_extdir} instead of %{_libdir}/php/modules
- use %{pecl_xmldir} instead of your %{xmldir}

As the package is available in my little testing repo, you can have a look to
http://rpms.famillecollet.com/SPEC/php-pecl-imagick.spec

Comment 3 Pavel Alexeev 2009-01-04 20:43:01 UTC
(In reply to comment #1)
> A few notes :
> 
> - License is PHP, not BSD (according to pecl.php.net)
Off course. It has initialy 'PHP License' which is not correct, and I erroneously wasn't find "PHP" in list...

> - must use %setup -q -c (to not have package.xml outside the build tree)
Ok, I add -c flag
> - missing require for ABI check : php(zend-abi)
Hm... I fully borrow %if...%endif statement for that from your spec-file...

Updated later: I found this in doc by link provided by you.

> - should use %{pecl_install} and  %{pecl_uninstall} when exists
Ok. This is good note, thank you.
> - why PEAR in sumnary ?
Because description from it.
Right, removed.

> - should add example directory in %doc (rather than each files)
Ok.

> Read : http://fedoraproject.org/wiki/Packaging/PHP
Thanks a lot.

(In reply to comment #2)
> Also 
> - use %{php_extdir} instead of %{_libdir}/php/modules
> - use %{pecl_xmldir} instead of your %{xmldir}
Done.

> As the package is available in my little testing repo, you can have a look to
> http://rpms.famillecollet.com/SPEC/php-pecl-imagick.spec
Yes, I have seen this new in your blog. Thank you for help.
Your spec-file was very useful , but it also has some shortcomings (such as undocumented options documented only what its is undocumented :) in config, created from SPEC...)


http://hubbitus.net.ru/rpm/Fedora9/php-pecl-imagick/php-pecl-imagick-2.2.1-2.fc9.src.rpm

Comment 4 Remi Collet 2009-01-11 07:52:35 UTC
REVIEW:

+ rpmlint (see comment)
php-pecl-imagick.i386: I: checking
php-pecl-imagick.i386: E: zero-length /usr/share/doc/php-pecl-imagick-2.2.1/INSTALL
php-pecl-imagick.i386: E: zero-length /usr/share/doc/php-pecl-imagick-2.2.1/TODO
php-pecl-imagick.src: I: checking
php-pecl-imagick-debuginfo.i386: I: checking
3 packages and 0 specfiles checked; 2 errors, 0 warnings.
+ package name ok
+ specfile name ok
+ license Ok (PHP)
+ spec in english
+ source ok
c205ff5e38ca88aad01e74ea8d0e3816  imagick-2.2.1.tgz
+ source URL ok
+ latest stable version
+ build ok (i386, x86_64, ppc and ppc64)
+ build ok in mock (F9, F10, devel)
+ BR ok for fedora
+ no locale
+ own all directories ok
+ perm ok
+ clean ok
+ contain code ok
+ doc small enough
+ doc not required to run
+ no devel
+ no gui
+ install start with rm buildroot
+ Final Requires 
/bin/sh  
/usr/bin/pecl  
config(php-pecl-imagick) = 2.2.1-2.fc11
libMagickCore.so.1  
libMagickWand.so.1  
libc.so.6  
php(api) = 20041225
php(zend-abi) = 20060613
- Final Provides not ok (see MUST)
config(php-pecl-imagick) = 2.2.1-2.fc11
imagick.so  
php-pecl-imagick = 2.2.1-2.fc11
+ package.xml is V2 (registration ok)


SHOULD:
- TODO is provided empty by upstream, so i think it's ok (could be not empty in another version)

- INSTALL is also empty but generally provides information about "building from source" which is not usefull with RPM. You should probably remove it (some package keep it, some remove it)

- add version in BR (ok for all fedora but EPEL 4 don't have requirement)
BuildRequires: php-devel >= 5.1.3, php-pear, ImageMagick-devel >= 6.2.4

- setup the -n option is needless when -c used

- add conditional (recommended in PHP Guidelines)
  %post => %if 0%{?pecl_install:1}
  %postun => %if 0%{?pecl_uninstall:1}
Without, you package couldn't be imported in EPEL-5 (macro not defined in old php-pear, but even "pecl install" don't work => no extension registration in this case)

MUST:
- add the missing virtual provides (from PHP guidelines)
Provides:     php-pecl(%peclName)


Only the "MUST" must be fixed for approval, but SHOULD need your attention.

Do you want to maintain thi package in EPEL ?

Comment 5 Remi Collet 2009-01-11 07:58:52 UTC
For MUST, of course i mean
Provides:     php-pecl(%peclName) = %{version}

Comment 6 Pavel Alexeev 2009-01-11 13:31:22 UTC
Firstly - thank you for review.

(In reply to comment #4)
> REVIEW:

> SHOULD:
> - TODO is provided empty by upstream, so i think it's ok (could be not empty in
> another version)
> 
> - INSTALL is also empty but generally provides information about "building from
> source" which is not usefull with RPM. You should probably remove it (some
> package keep it, some remove it)
As mentioned initialy in first post I also thik what it is not errors and I want to stay it here because it comes from upstream.

> - add version in BR (ok for all fedora but EPEL 4 don't have requirement)
> BuildRequires: php-devel >= 5.1.3, php-pear, ImageMagick-devel >= 6.2.4
Ok.
But from what you get minimum requirement version of ImageMagick? I do not remember what versions was used, but as you can see by changelog, I maintain (for himself repository off course) this package notably long time, nad do not remember any problems with IM...

> - setup the -n option is needless when -c used
I wasn't known that. Fixed.

> - add conditional (recommended in PHP Guidelines)
>   %post => %if 0%{?pecl_install:1}
>   %postun => %if 0%{?pecl_uninstall:1}
Done.
> Without, you package couldn't be imported in EPEL-5 (macro not defined in old
> php-pear, but even "pecl install" don't work => no extension registration in
> this case)
So, EPEL-5 do not require such registration at all??

> 
> MUST:
> - add the missing virtual provides (from PHP guidelines)
> Provides:     php-pecl(%peclName)
Ok, added:
Provides:     php-pecl(%peclName) = %{version}

> 
> Do you want to maintain thi package in EPEL ?
I'm not use this distributions itself. But I can maintain it for EPEL5 on CentOS. It is acceptable? I compleatly do not want maintain it for EPEL4 (but I can import for that branch without testing with hope to the best). May be you want co-mantain it in EPEL4 and/or EPEL5?


http://hubbitus.net.ru/rpm/Fedora9/php-pecl-imagick/php-pecl-imagick-2.2.1-3.fc9.src.rpm

Comment 7 Remi Collet 2009-01-11 17:19:29 UTC
> But from what you get minimum requirement version of ImageMagick?

From upstream (and spec) description ;)

> So, EPEL-5 do not require such registration at all??

No, like in older fedora (pear < 1.7) it wasn't possible.

> But I can maintain it for EPEL5 on CentOS

Yes

> I compleatly do not want maintain it for EPEL4

Anyway, it is not possible as EL-4 only provides php 4.3.9

> May be you want co-mantain it in EPEL4 and/or EPEL5?

Yes, add my FAS (remi) to the CC and request the EL-5 branch in you CVS request.

Comment 8 Remi Collet 2009-01-11 17:20:06 UTC
All changes are OK.

Package APPROVED.

Comment 9 Pavel Alexeev 2009-01-20 20:25:57 UTC
Sory for dalay.
New Package CVS Request
=======================
Package Name: php-pecl-imagick
Short Description: Provides a wrapper to the ImageMagick library
Owners: Hubbitus
CC: remi
Branches: F9 F10
InitialCC:

Comment 10 Kevin Fenzi 2009-01-20 20:35:26 UTC
cvs done.

Comment 11 Pavel Alexeev 2009-01-20 21:31:50 UTC
I forgot EL-5

New Package CVS Request
=======================
Package Name: php-pecl-imagick
Short Description: Provides a wrapper to the ImageMagick library
Owners: Hubbitus
CC: remi
Branches: EL-5
InitialCC:

Comment 12 Kevin Fenzi 2009-01-23 23:18:39 UTC
cvs done.

Comment 13 Fedora Update System 2009-02-05 02:13:49 UTC
php-pecl-imagick-2.2.1-3.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update php-pecl-imagick'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-1275

Comment 14 Fedora Update System 2009-02-05 02:17:31 UTC
php-pecl-imagick-2.2.1-3.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update php-pecl-imagick'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-1312

Comment 15 Fedora Update System 2009-02-24 20:42:53 UTC
php-pecl-imagick-2.2.1-3.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2009-02-24 20:55:19 UTC
php-pecl-imagick-2.2.1-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2009-03-01 01:45:26 UTC
php-pecl-imagick-2.2.2-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/php-pecl-imagick-2.2.2-1.fc9

Comment 18 Fedora Update System 2009-03-01 01:46:33 UTC
php-pecl-imagick-2.2.2-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/php-pecl-imagick-2.2.2-1.fc10

Comment 19 Fedora Update System 2009-03-23 15:51:37 UTC
php-pecl-imagick-2.2.2-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2009-03-23 15:56:05 UTC
php-pecl-imagick-2.2.2-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.


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