Bug 195836
Summary: | Review Request: php-pecl-apc | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Chris Chabot <chabotc> |
Component: | Package Review | Assignee: | Paul Howarth <paul> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | paul, rpm |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | kevin:
fedora-cvs+
|
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-06-19 17:18:16 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Chris Chabot
2006-06-18 14:04:26 UTC
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. |