Bug 841662 - Review Request: php-pecl-cairo - PHP cairo extension module
Review Request: php-pecl-cairo - PHP cairo extension module
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
: Reopened
Depends On:
  Show dependency treegraph
Reported: 2012-07-19 15:08 EDT by Nathanael Noblet
Modified: 2014-11-04 02:57 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2014-11-04 02:57:22 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
limburgher: fedora‑cvs+

Attachments (Terms of Use)
php-pecl-cairo-review.txt (8.59 KB, text/plain)
2012-08-09 08:53 EDT, Remi Collet
no flags Details

  None (edit)
Description Nathanael Noblet 2012-07-19 15:08:48 EDT
Spec URL: http://www.noblet.ca/php-pecl-cairo.spec
SRPM URL: http://www.noblet.ca/php-pecl-cairo-0.3.2-0.1.fc17.src.rpm
 PECL cairo is the official Cairo Graphics Library binding. It provides an
object oriented and procedural interface for the cairo library. Support is
currently provided for 1.4 to 1.8 versions of the library
(1.10 is partially supported).

Fedora Account System Username: gnat
Comment 1 Nathanael Noblet 2012-08-02 01:49:52 EDT
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
Comment 2 Remi Collet 2012-08-05 03:03:42 EDT
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

> 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 \
%{_bindir}/php run-tests.php \
    -n -q \
    -d extension_dir=modules \
    -d extension=%{pecl_name}.so
Comment 3 Nathanael Noblet 2012-08-07 13:45:27 EDT
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.
Comment 4 Remi Collet 2012-08-08 02:51:26 EDT
(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:

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.
Comment 5 Remi Collet 2012-08-08 03:12:29 EDT
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.
Comment 6 Nathanael Noblet 2012-08-08 13:54:14 EDT
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

- 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
Comment 7 Nathanael Noblet 2012-08-08 14:34:37 EDT
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:

Comment 8 Nathanael Noblet 2012-08-08 14:41:07 EDT
Sorry bad spec.. new scratch build with patch:

Comment 9 Vincent Danen 2012-08-08 17:01:24 EDT
Is there a reason this was flagged as security-sensitive?  And does this bug need to be private?
Comment 10 Nathanael Noblet 2012-08-08 17:58:28 EDT
@Vincent Danen - No to both. I never intentionally set either, and actually can't seem to clear the flag
Comment 11 Vincent Danen 2012-08-09 00:16:48 EDT
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!
Comment 12 Remi Collet 2012-08-09 01:22:17 EDT
It seems  http://www.noblet.ca is currently down...
Comment 13 Nathanael Noblet 2012-08-09 03:48:31 EDT
we were migrating to a new server... back up.
Comment 14 Remi Collet 2012-08-09 08:53:03 EDT
Created attachment 603254 [details]

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

It will be better to have failed tests mark as "skipped"
Should be reported to upstream.
Comment 15 Remi Collet 2012-08-09 08:57:28 EDT
No blocker.

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

=== Approved ===
Comment 16 Nathanael Noblet 2012-08-09 11:30:40 EDT
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
Comment 17 Gwyn Ciesla 2012-08-09 12:17:31 EDT
Git done (by process-git-requests).

Don't request rawhide, devel is automatic.
Comment 18 Remi Collet 2012-11-13 11:05:59 EST
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)
Comment 19 Nathanael Noblet 2014-09-02 13:55:18 EDT
Package Change Request
Package Name: php-pecl-cairo
New Branches: epel7
Owners: gnat
Comment 20 Gwyn Ciesla 2014-09-02 15:18:08 EDT
Git done (by process-git-requests).
Comment 21 Remi Collet 2014-11-04 02:57:22 EST
Should have be closed for a while...

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