Spec URL: http://ppadron.blog.br/rpm/specs/php-pecl-augeas.spec SRPM URL: http://ppadron.blog.br/rpm/srpm/php-pecl-augeas-0.5.1-1.src.rpm Description: This package provides PHP bindings to the Augeas API. Augeas is a configuration editing tool. It parses configuration files in their native formats and transforms them into a tree. Configuration changes are made by manipulating this tree and saving it back into native config files.
Forgot to mention that I need a sponsor for this. I have submitted other 2 packages, but they were not reviewed yet.
Can you post koji scratch build ?
Sure, here it is: http://koji.fedoraproject.org/koji/taskinfo?taskID=1957217
Update for release 0.6.0: Spec URL: http://ppadron.blog.br/rpm/specs/php-pecl-augeas.spec SRPM URL: http://ppadron.blog.br/rpm/srpms/php-pecl-augeas-0.6.0-1.src.rpm
Just some quick notes : - missing CREDITS files in %doc - rpmlint output a error E: explicit-lib-dependency augeas-libs You don't need to "requires: augeas-libs" as the binary package auto-requires "libaugeas.so.0" (provided by augeas-libs) - %define (line 2) must be replaced by %global - must not requires "php" (which brings a lot other unneeded dependencies, mainly httpd), but "php-common" - conditional requires not needed (as this requires php 5.2, which always provides php(zend-abi)) Instead of: Requires: php >= 5.2.0, augeas-libs %if 0%{?php_zend_api} Requires: php(zend-abi) = %{php_zend_api} Requires: php(api) = %{php_core_api} %else Requires: php-api = %{php_apiver} %endif Could simply use: Requires: php-common >= 5.2.0 Requires: php(zend-abi) = %{php_zend_api} Requires: php(api) = %{php_core_api} - you create a macro %{pecl_name} macro, but you don't use it where you could - your macro usage is not consistent. As you use sometime "%{__install}" and sometime "install" - no test provided, but you can add a simple load test %check %{_bindir}/php \ -n -q -d extension_dir=modules \ -d extension=%{pecl_name}.so \ --modules | grep %{pecl_name} What are your other reviews pending ? (if under my skills, I could enter in the process to sponsor you)
> - no test provided, In fact, a PHPUnit test suite provided. So, (only if you can't run it during the rpmbuild), simply add a comment in your spec on how to run it.
Remi, thanks for the comments! Here's the changelog for the new release: * Mon Feb 08 2010 Pedro Padron <ppadron.br> - 0.6.0-2 - Changes based on initial package review by Remi Collet - Added CREDITS file - Removed explicit "Requires: augeas-libs" - Changed "Requires: php" to "Requires: php-common >= 5.2.0" - Removed conditional Requires for php(api) - Added check section with a module load test - pecl_name and __install macros are now used properly About the tests, I added the load test you suggested. For the next release (0.7.0) I'll convert the PHPUnit tests to .phpt, which makes more sense when it comes to PECL extensions. I believe that the Wiki entry about PHP packaging could use an update based on your comments. I'll try to do that today (don't know if I can actually edit the page, though). http://fedoraproject.org/wiki/Packaging/PHP Here are the new links: SPEC URL: http://ppadron.blog.br/rpm/specs/php-pecl-augeas.spec SRPM URL: http://ppadron.blog.br/rpm/srpms/php-pecl-augeas-0.6.0-2.fc12.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=1968752 Just a question... Your rpmlint output differs from what I got here. Even for the first release, rpmlint did not complain about anything. Do you have additional filters? Thanks!
You cannot edit the PHP Guidelines. Changes must be reviewed / approved by FPC See: http://fedoraproject.org/wiki/Packaging/Committee But some changes are already written and approved (just waiting to be moved to official Guidelines) http://fedoraproject.org/wiki/PackagingDrafts/PHP What do you think is missing there ? > Your rpmlint output differs from what I .. No, standard rpmlint. > I'll convert the PHPUnit tests to .phpt Well, I think PHPUnit is really a good tool ;) probably a better one than .phpt. But that is only your (upstream) choice. I encounter an issue on my machine : httpd segfault on launch when augeas extension is enabled (no problem with php-cli). I need to check if this can be reproduce on another computer (I run PHP 5.3.2-dev)
Pedro, Have you test this extension under x86_64 ? Fedora 11 i386 + PHP 5.3.1 => ok Fedora 11 i386 + PHP 5.3.2RC1 => ok EL 5 x86_64 + PHP 5.3.1 => segfault Fedora 12 x86_64 + PḦP 5.3.2RC1 => segfault (build ok and php-cli ok, only apache issue)
Remi, I could reproduce the problem here. I have tested in x86_64 only with php-cli, running the phpUnit test suite, that's why I missed it =( I'm debugging the problem right now, which seems to be related to the AugeasException class. As soon as I fix it and push a new release, I'll update this ticket. Thanks for your help!
Apache segfault on x86_64 fixed! http://pecl.php.net/package/augeas/0.6.1 SRPM: http://ppadron.blog.br/rpm/srpms/php-pecl-augeas-0.6.1-1.fc12.src.rpm SPEC: http://ppadron.blog.br/rpm/specs/php-pecl-augeas.spec scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1979587
Ok, version 0.6.1 fix apache segfault. REVIEW : + rpmlint is ok php-pecl-augeas.src: I: checking php-pecl-augeas.x86_64: I: checking php-pecl-augeas-debuginfo.x86_64: I: checking 3 packages and 1 specfiles checked; 0 errors, 0 warnings. + package is named according to the Package Naming Guidelines . + spec file name + The package must meet the Packaging Guidelines and PHP Guidelines + license ok (PHP) and match upstream + license provided + spec file is legible + sources match upstream 44859bbe4da82b88d18f41489495c3a3 augeas-0.6.1.tgz + source URL ok + build on F12 x86_64 (php 5.3.2RC2) + build on mock/koji (F12 ref in previous post) + build on all arch (F11, i386, x86_64, ppc, and ppc64) http://koji.fedoraproject.org/koji/taskinfo?taskID=1980430 + BuildRequires + no locale + no shared library (extension are not lib.) + no system library + own all directories that it creates + not list a file more than once in the spec + Permissions on files are set properly. + %clean ok + consistently use macro + contain code + small doc, no sub package + doc not required to run + no -devel + no -static + no .pc + no .la + not own files or directories already owned by other packages + %install start with rm -rf $RPM_BUILD_ROOT + all files are UTF-8 + %check ok (only load test) + PHPUnit suite ok (after install) OK (14 tests, 19 assertions) + load correctly with httpd Should : ask upstream ;) if possible to split README into INSTALL + README, because INSTALL instruction could be removed from RPM (for the next release) *** APPROVED *** Of course, you need to wait for your "packager" status approval before asking for CVS. That's why I didn't put the review flag + now.
Pedro, have you done any work to obtain a sponsor? http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group has some pointers.