Bug 560071 (php-pecl-augeas)

Summary: Review Request: php-pecl-augeas - PHP bindings to the Augeas API
Product: [Fedora] Fedora Reporter: Pedro Padron <ppadron>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora, fedora-package-review, itamar, notting
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-06-15 15:58:42 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Pedro Padron 2010-01-29 14:17:39 EST
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.
Comment 1 Pedro Padron 2010-01-29 14:21:16 EST
Forgot to mention that I need a sponsor for this. I have submitted other 2 packages, but they were not reviewed yet.
Comment 2 Itamar Reis Peixoto 2010-01-29 14:25:54 EST
Can you post koji scratch build ?
Comment 3 Pedro Padron 2010-02-01 14:04:04 EST
Sure, here it is:
Comment 4 Pedro Padron 2010-02-01 14:19:12 EST
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
Comment 5 Remi Collet 2010-02-06 12:35:59 EST
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}
Requires: php-api = %{php_apiver}

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

%{_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)
Comment 6 Remi Collet 2010-02-06 12:44:25 EST
> - 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.
Comment 7 Pedro Padron 2010-02-08 09:45:36 EST
Remi, thanks for the comments!

Here's the changelog for the new release:

* Mon Feb 08 2010 Pedro Padron <ppadron@w3p.com.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:

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?

Comment 8 Remi Collet 2010-02-08 12:30:59 EST
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)

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)
Comment 9 Remi Collet 2010-02-10 11:40:25 EST
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)
Comment 10 Pedro Padron 2010-02-11 12:52:02 EST

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!
Comment 12 Remi Collet 2010-02-12 12:49:06 EST
Ok, version 0.6.1 fix apache segfault.

+ 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)
+ 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.
Comment 13 Jason Tibbitts 2010-11-16 18:06:41 EST
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.