Bug 477338 - Review Request: php-pecl-imagick - Provides a wrapper to the ImageMagick library
Review Request: php-pecl-imagick - Provides a wrapper to the ImageMagick library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-20 12:48 EST by Pavel Alexeev
Modified: 2009-03-23 11:56 EDT (History)
3 users (show)

See Also:
Fixed In Version: 2.2.2-1.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-24 15:42:58 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pavel Alexeev 2008-12-20 12:48:35 EST
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 11:46:21 EST
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 11:53:53 EST
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 15:43:01 EST
(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 02:52:35 EST
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 02:58:52 EST
For MUST, of course i mean
Provides:     php-pecl(%peclName) = %{version}
Comment 6 Pavel Alexeev 2009-01-11 08:31:22 EST
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 12:19:29 EST
> 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 12:20:06 EST
All changes are OK.

Package APPROVED.
Comment 9 Pavel Alexeev 2009-01-20 15:25:57 EST
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 15:35:26 EST
cvs done.
Comment 11 Pavel Alexeev 2009-01-20 16:31:50 EST
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 18:18:39 EST
cvs done.
Comment 13 Fedora Update System 2009-02-04 21:13:49 EST
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-04 21:17:31 EST
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 15:42:53 EST
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 15:55:19 EST
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-02-28 20:45:26 EST
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-02-28 20:46:33 EST
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 11:51:37 EDT
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 11:56:05 EDT
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.