Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 560071 (php-pecl-augeas) - Review Request: php-pecl-augeas - PHP bindings to the Augeas API
Summary: Review Request: php-pecl-augeas - PHP bindings to the Augeas API
Keywords:
Status: CLOSED NOTABUG
Alias: php-pecl-augeas
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2010-01-29 19:17 UTC by Pedro Padron
Modified: 2012-06-15 19:58 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-06-15 19:58:42 UTC


Attachments (Terms of Use)

Description Pedro Padron 2010-01-29 19:17:39 UTC
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 19:21:16 UTC
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 19:25:54 UTC
Can you post koji scratch build ?

Comment 3 Pedro Padron 2010-02-01 19:04:04 UTC
Sure, here it is:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1957217

Comment 4 Pedro Padron 2010-02-01 19:19:12 UTC
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 17:35:59 UTC
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)

Comment 6 Remi Collet 2010-02-06 17:44:25 UTC
> - 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 14:45:36 UTC
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:
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!

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

Comment 9 Remi Collet 2010-02-10 16:40:25 UTC
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 17:52:02 UTC
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!

Comment 12 Remi Collet 2010-02-12 17:49:06 UTC
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.

Comment 13 Jason Tibbitts 2010-11-16 23:06:41 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.