Review for this package:-
      Mock Build Results for i386 
    Some unused variables warnings otherwise package built successfully for i386.
MUST Items:
     - MUST: rpmlint shows error 
rpmlint php-apc-5.1.4_3.0.10-1.src.rpm
W: php-apc invalid-license PHP License
W: php-apc strange-permission php-apc.spec 0666
     - MUST: The package is named according to the Package Naming Guidelines.
     - MUST: The spec file name matching the base package php-apc, in the format
php-apc.spec
      - MUST: This package meets the Packaging Guidelines.
      - MUST: The package is licensed with an open-source compatible license GPL.
      - MUST: The License field in the package php-apc.spec file matches the
actual license file LICENSE in tarball.
      - MUST: The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct.
      - MUST: This package owns all directories that it creates. 
      - MUST: This package did not contain any duplicate files in the %files
listing.
      - MUST: This package  have a %clean section, which contains %{__rm} -rf
%{buildroot}.
      - MUST: This package used macros.
      - MUST: Document files are included like TECHNOTES.txt NOTICE.

You must change upstream package name APC to php-apc-3.0.10

Hi Panemade, thanks for picking this up!

I've changed the mod of the spec file to 644, so that reduces the rpmlint errors
to just the licence warning (which is a valid licence, same as php's).

Unfortunatly i can't easely change the APC tgz name, its pecl's choice about the
naming, so not much i can do about it.

I've uploaded the new src.rpm with the chmod'd spec file to the same location as
before:
Spec URL: http://develop.intermax.nl/php-apc/php-apc.spec
SRPM URL: http://develop.intermax.nl/php-apc/php-apc-5.1.4_3.0.10-1.src.rpm

>its pecl's choice about the naming, so not much i can do about it.
  what do you mean by that?

You asked me:
"You must change upstream package name APC to php-apc-3.0.10"

But i am unable to do so, its pecl.php.net's naming standard, plus not my call
to make

I could rename the included tarball to php-apc-3.0.10 if you insist, though this
would be breaking consistency with upstream's naming

ok package Approved

Imported, and build for devel:
http://buildsys.fedoraproject.org/logs/fedora-development-extras/11210-php-apc-5.1.4_3.0.10-1.fc6/

Closing bug & thanks for the review!

I don't think Parag was entitled to approve packages actually; the review
guidelines say "The primary Reviewer can be any current package owner", and
Parag is not yet sponsored. I've encouraged Parag to review packages as part of
the sponsorship process, but this shouldn't include approving packages.

Paul,
     Then do that mean i am still not having rights to chnage FE-REVIEW to
FE-ACCEPT. If so then can I chnage back FE-ACCEPT to FE-REVIEW from all other
bugs that i am resolving since last fewe hours.

(In reply to comment #9)
> Paul,
>      Then do that mean i am still not having rights to chnage FE-REVIEW to
> FE-ACCEPT. If so then can I chnage back FE-ACCEPT to FE-REVIEW from all other
> bugs that i am resolving since last fewe hours.

Yes, that's correct. You'll be able to be the primary reviewer and approve
packages once you're sponsored, What other people in your position tend to do is
to start their reviews with "Not an official review as I'm not yet sponsored" or
something similar, and then add their review comments. This process is useful as
it helps the package submitter get any issues sorted out before the package is
formally reviewed, and it demonstrates your understanding of the packaging
guidelines to your own potential sponsors.

Ok I got it Thanks

Paul though i appreciate the situation, i didn't think to check first if the 
person reviewing this package was infact already a contributer, i kind of 
assumed everyone knew the rules :-)

That leaves this package in the rather odd position that its already been 
imported and build and part of extras devel, without it actually being 
approved..

Who's Parag's sponsor, and does he/she have the time to do a formal review of 
this package? :-) Else i'll have to ask the extra's/cvs maintainers to remove 
the package from extra's untill its been given a formal review, would be a 
shame

(In reply to comment #12)
> Paul though i appreciate the situation, i didn't think to check first if the 
> person reviewing this package was infact already a contributer, i kind of 
> assumed everyone knew the rules :-)
> 
> That leaves this package in the rather odd position that its already been 
> imported and build and part of extras devel, without it actually being 
> approved..

I'll try to find the time to re-review it later today.

> Who's Parag's sponsor, and does he/she have the time to do a formal review of 
> this package? :-) Else i'll have to ask the extra's/cvs maintainers to remove 
> the package from extra's untill its been given a formal review, would be a 
> shame

Parag doesn't yet have sponsor (which is the core of the problem here). It is
likely that I will be that sponsor eventually though, when I'm convinced that
Parag understands the processes...

First thing I've noticed from a preliminary look at this is that the package
naming doesn't line up with the proposed naming convention for PHP PECL packages
at: http://www.fedoraproject.org/wiki/Packaging/PHP

Whilst these guidelines havn't made it into the top-level guidelines yet, it'd
probably be best to go along with them, or make a case for staying with the
current name if you really want to do that.

Other things I noticed:
BuildReqs of autoconf and automake should be pulled in by the existing buildreq
of php-devel; if libtool really is a requirement of phpize, perhaps a bug should
be raised on php to that effect? Similarly, the buildreq of php is pulled in by
php-devel and _could_ be removed (I'm not saying it has to be though).

Hi Paul, thanks for taking the time to look at the package in this situation

I've made some changes to the spec file based on your feedback:

Removed Buildrequires for php, auto*, libtool .. mock build says your absolutely
right and there was no need for them :-)

I've added "Provides: php-pecl(apc)" to it, based on the sugestions in the PHP
packaging proposal.

Renaming the package to php-pecl-APC might be less desireable though, for a
number of reasons. Firstly out of conveniance (its already imported and build as
php-apc), but more importantly out of consitency with php-json, php-idn,
php-eaccelerator, etc ... only mailparse seems to follow this standard at this
time, however if your of the opinion that it would be much preferable to follow
the php-pecl-apc naming, i'd be willing to send out the revoke mails and rename
the package too :-)

Oh new locations of spec/srpm:
http://develop.intermax.nl/php-apc/php-apc.spec
http://develop.intermax.nl/php-apc/php-apc-5.1.4_3.0.10-2.src.rpm

(In reply to comment #15)
> Hi Paul, thanks for taking the time to look at the package in this situation
> 
> I've made some changes to the spec file based on your feedback:
> 
> Removed Buildrequires for php, auto*, libtool .. mock build says your absolutely
> right and there was no need for them :-)
> 
> I've added "Provides: php-pecl(apc)" to it, based on the sugestions in the PHP
> packaging proposal.
> 
> Renaming the package to php-pecl-APC might be less desireable though, for a
> number of reasons. Firstly out of conveniance (its already imported and build as
> php-apc), but more importantly out of consitency with php-json, php-idn,
> php-eaccelerator, etc ... only mailparse seems to follow this standard at this
> time, however if your of the opinion that it would be much preferable to follow
> the php-pecl-apc naming, i'd be willing to send out the revoke mails and rename
> the package too :-)

My preference would be for php-pecl-apc or php-pecl-APC; it's a hassle I know
but it helps to provide the right precedent for future packages (mailparse won't
be alone...). The new guidelines also have:

Where there is no naming conflict, a package named "foo" SHOULD do:
 * Provides: php-foo = %{version}-%{release}

so a "yum install php-apc" would still work if you had that.

Ok ok php-pecl-apc it is :-0

Renamed and uploaded to:
http://develop.intermax.nl/php-apc/php-pecl-apc-5.1.4_3.0.10-3.src.rpm
http://develop.intermax.nl/php-apc/php-pecl-apc.spec

I've added the Provides: php-apc to the spec file, and i've also added
'obsoletes: php-apc' to it, so it will replace the package with the old name and
to prevent extra's repo from being a mess :-)

(In reply to comment #18)
> Ok ok php-pecl-apc it is :-0
> 
> Renamed and uploaded to:
> http://develop.intermax.nl/php-apc/php-pecl-apc-5.1.4_3.0.10-3.src.rpm
> http://develop.intermax.nl/php-apc/php-pecl-apc.spec

Is it really necessary to have the PHP version number included in this package's
version? I'd have thought that the php-abi dep would take care of the PHP
compatibility issue that that's there for.

It can be usefull when there's multiple php packages, ie not every system is
always updated to the latest php packages, so having a package per php version
can be usefull. However it doesn't seem to be standard (anymore) so i removed it:

http://develop.intermax.nl/php-apc/php-pecl-apc-3.0.10-3.src.rpm
http://develop.intermax.nl/php-apc/php-pecl-apc.spec

Review:

- rpmlint output:
W: php-pecl-apc incoherent-version-in-changelog 5.1.4_3.0.10-3 3.0.10-3.fc5
W: php-pecl-apc invalid-license PHP License
W: php-pecl-apc invalid-license PHP License
W: php-pecl-apc-debuginfo invalid-license PHP License

  The License is actually fine, and standard for PHP packages.
  The changelog entry needs fixing.

- package meets proposed PHP naming guidelines
- spec file name is correct
- license is PHP license, matches spec, text included
- package meets guidelines
- spec file written in English and is legible
- sources match upstream
- package builds ok in mock (FC-5 i386)
- BR's OK
- no locales, libraries, pkgconfigs, or subpackages to worry about
- not relocatable
- no directory ownership or permissions problems
- no duplicate files
- %clean section present and correct
- macro usage is consistent
- code, not content
- no large docs
- docs don't affect runtime
- no desktop file needed
- no scriptlets

Needswork:

- Fix version number in changelog entry, which will shut rpmlint up

Queries:

- Why such a low value for apc.shm_size? This is the only value you've changed
  from the suggested values in the INSTALL file.
- What is "Provides: php-zend_extension" for?

Suggestions:

- Include INSTALL as %doc; it includes useful end-user configuration info

Needswork:
- Fixed changelog versions

Queries:
- Changed shm_size to 32m, donno was a judgement call to make, for most apps 16
megs of cache is more then enough, only on servers with lots of scripts could
you posibly break out of the 16m limit. However seeing 32m is the default
adviced size, i've changed the default config to it.

- zend_extention is a leftover from following php-eaccelerator, it included it
to so i followed its example :-) Seeing no reason to keep it included i've
removed it from the spec file though

Sugestions:
- Added install to the %doc section, used to it not having to be included in
most packages, but your right, it has a lot of configuration info so should be
included.

Put updated files in usual location:
http://develop.intermax.nl/php-apc/php-pecl-apc-3.0.10-3.src.rpm
http://develop.intermax.nl/php-apc/php-pecl-apc.spec

Approved. Again :-)

Don't forget to change the owners.list entry to reflect the package name change.

Thanks for jumping in and helping out Paul, its a better package for it in the
end :-)

Closing bug (again) with NEXTRELEASE, its been imported and build (
http://buildsys.fedoraproject.org/logs/fedora-development-extras/11244-php-pecl-apc-3.0.10-3.fc6/
), owners file has been updated to reflect the new package name too ofcourse :-)

Package Change Request
======================
Package Name: php-pecl-apc
New Branches: EL-4 EL-5
Updated Fedora CC: timj
Updated EPEL Owners: timj
Updated EPEL CC: chabotc

This has been agreed by the Fedora package owner chabotc.

cvs done.