Bug 560071 (php-pecl-augeas)

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.