Spec URL: http://rpms.damian.net/SPECS/php-pdepend-PHP-Depend.spec SRPM URL: http://rpms.damian.net/SRPMS/php-pdepend-PHP-Depend-0.9.9-1.fc12.src.rpm Description: PHP_Depend design quality metrics for PHP package
First notes (before review): - According to package.xml and "pci" command result, it requires php-xml (dom + simpleXML) - directory /usr/share/pear/PHP is not owned - should use %{channel} in %postun to have consistent use of macros Optional deps : it's your choice to include this, or not > PHP_Depend-0.9.9 can optionally use package "pecl/imagick" (version >= 2.2.0b2) As this package is available, you could add Requires: php-pecl(imagick). A comment about this as "optionnal" seems usefull. If you don't want to add it, please add a note in the package description. Why do you remove test from RPM ? Adding %{pear_testdir}/%{pear_name} seems usefull (nothing on the Guidelines about this, but most of pear packages which provides tests keep it bundled) Do you know how to run the test suite providesd? I've try (but most fails) $ cd /usr/share/pear/test/PHP_Depend/tests/PHP/Depend/ $ phpunit AllTests (it's also a good idea, if you couldn't run it during build to add a comment about how to run it in your spec)
(In reply to comment #1) > First notes (before review): > > - According to package.xml and "pci" command result, it requires php-xml (dom + > simpleXML) OK > - directory /usr/share/pear/PHP is not owned I was not sure. I thought these common directories shouldn't be owned. > - should use %{channel} in %postun to have consistent use of macros OK > Optional deps : it's your choice to include this, or not > > PHP_Depend-0.9.9 can optionally use package "pecl/imagick" (version >= 2.2.0b2) > > As this package is available, you could add > Requires: php-pecl(imagick). > A comment about this as "optionnal" seems usefull. > If you don't want to add it, please add a note in the package description. I have added it, it makes sense as it is available in Fedora anyway. > Why do you remove test from RPM ? > > Adding %{pear_testdir}/%{pear_name} seems usefull (nothing on the Guidelines > about this, but most of pear packages which provides tests keep it bundled) I had a look at other packages and some seem to remove them and some keep them. I planned to add them again anyway after seeing some other packages doing it. > Do you know how to run the test suite providesd? > I've try (but most fails) > $ cd /usr/share/pear/test/PHP_Depend/tests/PHP/Depend/ > $ phpunit AllTests > (it's also a good idea, if you couldn't run it during build to add a comment > about how to run it in your spec) To run them you have to create the /usr/share/pear/test/PHP_Depend/tests/PHP/Depend/_run directory and I think the tree has to be writeable. Once you do that you get: OK, but incomplete or skipped tests! Tests: 1145, Assertions: 3601, Skipped: 3. If it isn't writeable you get: FAILURES! Tests: 1145, Assertions: 3478, Failures: 6, Errors: 32, Skipped: 1. I don't think that they are made to be run after install. I made a new version: Spec URL: http://rpms.damian.net/SPECS/php-pdepend-PHP-Depend.spec SRPM URL: http://rpms.damian.net/SRPMS/php-pdepend-PHP-Depend-0.9.9-2.fc12.src.rpm
Thanks for the _run solution. With php-pecl-imagick installed : ==> >ERROR: meta.c (179): wmf_header_read: this isn't a wmf file FAILURES! Tests: 1145, Assertions: 3762, Errors: 2, Skipped: 3. It seems I have an issue with imagick extension, as php -i crash. imagick php: magick/semaphore.c:385: RelinquishSemaphoreInfo: Assertion `semaphore_info->signature == 0xabacadabUL' failed. Abandon (core dumped) I will try to investigate a little on this issue (not related to this package) and end the review ASAP.
Also seems the PHP_Depend_Util_ImageConvert::hasImagickConvert() is broken... WARNING: Cannot generate image of type 'png'. This feature needs either the pecl/imagick extension or the ImageMagick cli tool 'convert'. using : // $proc = proc_open('convert', $desc, $pipes); $proc = proc_open('convert -version', $desc, $pipes); if (is_resource($proc)) { // fwrite($pipes[0], '-version'); fclose($pipes[0]); seems working. This should probably be reported upstream, even if, when should not need it if we have php-pecl-imagick installed...
About imagick crash, see #559675 (not related to this package, so not blocking the review) My idea - as imagick conflicts with gmagick and is only optional for this package - as pdepend can generate .svg (without any other additionnal lib) - as pdepend can generate other format with "convert" (see previous comment) You probably could remove php-pecl(imagick) dependency. This is YOUR choice (not affecting the review)
REVIEW: + rpmlint is ok php-pdepend-PHP-Depend.src: I: checking php-pdepend-PHP-Depend.noarch: I: checking php-pdepend-PHP-Depend.noarch: W: no-documentation 2 packages and 1 specfiles checked; 0 errors, 1 warnings. + package name ok + spec file name ok + package meet the PHP Guidelines + License ok : BSD + License is upstream + spec in english and legible + license file not provided + sources match the upstream sources d1e686b555f62f39cfce830a40d5d675 PHP_Depend-0.9.9.tgz + Source URL ok + build on F12.x86_64 + BuildRequires (php-pear >= 1:1.6.0, php-channel(pear.pdepend.org)) ok + no locale + no .so + own all directories that it creates + no duplicate file + %defattr ok + %clean section + use macros consistently + contain code + small documentation not required to run + no devel + no pkgconfig + no sub-package + no GUI + don't own files or directories already owned by other packages + %install start with rm -rf + valid UTF-8 + test suite ok (can't be run during build) + scriptlets ok + Final Requires ok /usr/bin/pear /usr/bin/php php-channel(pear.pdepend.org) php-common >= 5.2.2 php-pecl(imagick) >= 2.2.0b2 php-xml + Final Provides ok php-pear(pear.pdepend.org/PHP_Depend) = 0.9.9 php-pdepend-PHP-Depend = 0.9.9-2.fc8 + Build in koji http://koji.fedoraproject.org/koji/taskinfo?taskID=1953005 You could remplace Requires: php-common >= 5.2.2 Requires: php-xml With a single Requires: php-xml >= 5.2.2 =========== APPROVED ===========
Thanks for the review. New Package CVS Request ======================= Package Name: php-pdepend-PHP-Depend Short Description: PHP_Depend design quality metrics for PHP package Owners: cdamian Branches: F-11 F-12 InitialCC:
CVS done (by process-cvs-requests.py).
php-pdepend-PHP-Depend-0.9.9-2.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/php-pdepend-PHP-Depend-0.9.9-2.fc11
php-pdepend-PHP-Depend-0.9.9-2.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/php-pdepend-PHP-Depend-0.9.9-2.fc12
php-pdepend-PHP-Depend-0.9.9-2.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report.
php-pdepend-PHP-Depend-0.9.9-2.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report.
Package Change Request ====================== Package Name: php-pdepend-PHP-Depend New Branches: EL-6 Owners: cdamian
Package Change Request ====================== Package Name: php-pdepend-PHP-Depend New Branches: EL-7 Owners: llaumgui
Git done (by process-git-requests).
Can you please check, it seems epel7 branch doesn't exist.
Package Change Request ====================== Package Name: php-pdepend-PHP-Depend New Branches: epel7 Owners: llaumgui