Spec URL: http://www.noblet.ca/php-pecl-cairo.spec
SRPM URL: http://www.noblet.ca/php-pecl-cairo-0.3.2-0.2.fc17.src.rpm

Updated with output of fedora-review issues

quick notes

1/ from http://fedoraproject.org/wiki/Packaging/PHP
> The source archive contains a package.xml outside any directory,
> so you have to use use 
> %setup -q -c

2/ You must own %{_includedir}/php/ext/cairo

3/
> BuildRequires: php-devel >= 5.1.0
> Requires:      php-devel%{?_isa} >= 5.4.0
Why 5.1.0 and 5.4.0 ? (upstream says 5.1.6)

4/ No need to requires php-common (abi check is enough)

5/ Do you really to plan EPEL-5 ?
AFAIK, this package requires cairo >= 1.4 (so should BR cairo-devel >= 1.4) which is not available in EL-5.

Dropping EL-5 target will allow a lot of cleanups.

6/ Please run provided test suite in %check, something like

TEST_PHP_EXECUTABLE=%{_bindir}/php \
REPORT_EXIT_STATUS=1 \
NO_INTERACTION=1 \
%{_bindir}/php run-tests.php \
    -n -q \
    -d extension_dir=modules \
    -d extension=%{pecl_name}.so

Spec URL: http://www.noblet.ca/php-pecl-cairo.spec
SRPM URL: http://www.noblet.ca/php-pecl-cairo-0.3.2-0.3.fc17.src.rpm

Updated with changes based on reviewer feedback

I am curious about some of the changes I needed to make to have the %setup -q -c work. Because of that line I had to do cd Cairo-%{version} in basically every section including the %files for documentation. I looked at the %setup macros but didn't find anything that would what was needed. I hope this is okay the way it is. Not really sure.

(In reply to comment #3)
> I am curious about some of the changes I needed to make to have the %setup
> -q -c work. Because of that line I had to do cd Cairo-%{version} in
> basically every section including the %files for documentation. I looked at
> the %setup macros but didn't find anything that would what was needed. I
> hope this is okay the way it is. Not really sure.

Yes, this is ok
(the only solution I know, and used by others pear/pecl extensions, as documented in the Guidelines)

You don't need to use pre-version release (0.3). Version 0.3.2 is the final published version (even if state is "beta", but this is about code stability, not about versionning). See xdebug for pre-version example http://pecl.php.net/package/xdebug.

=> Release 1%{?dist}

I would prefer you to create to cairo.ini in %prep and install it in %install
This is cleaner, and make simpler if you need it twice (if one day, you choose to also build the zts extension). Just a cosmetic change.

%defattr(-,root,root,-) is EL-5 only, should be removed

%global php_apiver .. is not used, should be removed

> - Own php/etc/cairo dir
=> typo : /usr/include/php/ext/cairo

But this dir must be owned by -devel. So, this is enough:
%{_includedir}/php/ext/%{pecl_name}

Have you test build under various version / arch ?
When I first build this extension, few month ago, I notice some test failure ?

Will do the formal review soon.

Looking at config.m4 : this package requires php 5.2.0 (despite web site says 5.1.6) and freetype library, so need

BuildRequires: php-devel >= 5.2.0
BuildRequires: freetype-devel

Freetype is optionnal, but as available in fedora repository, I think it will be a mistake to not provide all the feature.

Spec URL: http://www.noblet.ca/php-pecl-cairo.spec
SRPM URL: http://www.noblet.ca/php-pecl-cairo-0.3.2-0.4.fc17.src.rpm

Changes:
- Moved ownership of include/php/ext/cairo to -devel 
- php BR 5.2
- added freetype-devel BR
- moved the creation of cairo.ini to %prep
- removed defattr & php_apiver

I didn't mark the releasever as 1. My first package ever the reviewer had me use n<0 during review until approved. At which point I change the releasever to 1. I was planning on doing the same here so that the package starts at 1. No issue either way to me but it made sense back in the day.

I've only built under x86_64. Scratch build here: http://koji.fedoraproject.org/koji/taskinfo?taskID=4369959

So you were correct and the tests failed for i386 arches. I poked around and found a patch here: https://bugs.php.net/bug.php?id=61882 that seems to fix it. I've added it to the spec above without bumping the releasever or what not. New scratch builds here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=4370068

Sorry bad spec.. new scratch build with patch:


http://koji.fedoraproject.org/koji/taskinfo?taskID=4370073

Is there a reason this was flagged as security-sensitive?  And does this bug need to be private?

@Vincent Danen - No to both. I never intentionally set either, and actually can't seem to clear the flag

No, you wouldn't be able to.  I'll do that; I just wanted to make sure I wasn't missing something obvious first.  Thanks!

It seems  http://www.noblet.ca is currently down...

we were migrating to a new server... back up.

Created attachment 603254 [details]
php-pecl-cairo-review.txt

Generated by fedora-review 0.2.0 (53cc903) last change: 2012-07-09

I suspect EL6 cairo version (1.8) too old for the test suite.

A temporary acceptable solution:
%if 0%{?fedora} > 13
REPORT_EXIT_STATUS=1 \
%else
REPORT_EXIT_STATUS=0 \
%endif

It will be better to have failed tests mark as "skipped"
Should be reported to upstream.

No blocker.

Don't forget to set Release to 1 after import.

=== Approved ===

New Package SCM Request
=======================
Package Name: php-pecl-cairo
Short Description:  PECL package for drawing using cairo via PHP scripts
Owners: gnat
Branches: rawhide f18 f17 f16 el6
InitialCC:

Git done (by process-git-requests).

Don't request rawhide, devel is automatic.

As the package is now imported into the repository, don't forget to close this review (should probably have be linked to the "new package" update in bodhi)

Package Change Request
======================
Package Name: php-pecl-cairo
New Branches: epel7
Owners: gnat

Git done (by process-git-requests).

Should have be closed for a while...