Bug 563510 (php-xcache)

Summary: Review Request: php-xcache - yet another php cacher
Product: [Fedora] Fedora Reporter: Timon <timosha>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora, fedora-package-review, itamar, mrunge, notting, opensource, pahan, pdx
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-01-10 18:37:50 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449    
Attachments:
Description Flags
spec file
none
srpm
none
php-xcache.spec
none
php-xcache-1.3.0-2.fc12.src.rpm
none
php-xcache.spec
none
php-xcache-1.3.0-3.fc12.src.rpm
none
php-xcache.spec
none
php-xcache-1.3.0-4.fc12.src.rpm none

Description Timon 2010-02-10 08:33:10 EST
Description: XCache is a fast, stable PHP opcode cacher that has been tested and is now running on production servers under high load.
Comment 1 Timon 2010-02-10 08:34:21 EST
Created attachment 389992 [details]
spec file
Comment 2 Timon 2010-02-10 08:35:02 EST
Created attachment 389994 [details]
srpm
Comment 4 Itamar Reis Peixoto 2010-02-10 08:41:49 EST
is this your first package ?

Can you add a scratch build here ?
Comment 5 Matthias Runge 2010-02-10 15:09:06 EST
As I'm not approved, I could only provide an unofficial review:

rpmlint ../RPMS/i686/php-xcache-1.3.0-1.fc12.i686.rpm ../RPMS/i686/php-xcache-debuginfo-1.3.0-1.fc12.i686.rpm php-xcache.spec 
2 packages and 1 specfiles checked; 0 errors, 0 warnings. (ok)

* name meets naming convention (ok)

* can not verify license (BSD given in SPEC, couldn't find this in code, esp. COPYING does not mention BSD)

* Mixing $RPM_BUILD_ROOT and %{buildroot} should not be done (cf. Packaging guidelines in Fedora wiki)

* %global preferred over %define (used both)

* some code in SPEC is commented out, decreases readability. Why don't you remove those lines? Why is code for RHEL 4 and RHEL 5 included?

* Package compiles succesful into binary RPMS
Comment 6 Itamar Reis Peixoto 2010-02-10 15:24:53 EST
I recommend you to put this out of spec file.

%{__cat} > %{buildroot}%{_sysconfdir}/php.d/xcache.ini << 'EOF'
[xcache-common]
zend_extension = %{php_extdir}/xcache.so

[xcache.admin]
xcache.admin.enable_auth = On
xcache.admin.user = "mOo"
; xcache.admin.pass = md5($your_password)
xcache.admin.pass = ""

[xcache]
xcache.shm_scheme =        "mmap"
xcache.size  =               60M
xcache.count =                 1
xcache.slots =                8K
xcache.ttl   =              3600
xcache.gc_interval =         300

xcache.var_size  =            4M
xcache.var_count =             1
xcache.var_slots =            8K
xcache.var_ttl   =             0
xcache.var_maxttl   =          0
xcache.var_gc_interval =     300
EOF
Comment 7 Timon 2010-02-11 06:25:58 EST
Created attachment 390219 [details]
php-xcache.spec
Comment 8 Timon 2010-02-11 06:26:54 EST
Created attachment 390220 [details]
php-xcache-1.3.0-2.fc12.src.rpm
Comment 9 Timon 2010-02-11 06:27:59 EST
(In reply to comment #4)
> is this your first package ?
exactly, no. I'm not approved yet, but already have two review requests (#464141,#464117).

(In reply to comment #5)
> As I'm not approved, I could only provide an unofficial review:
thanks 

> * can not verify license (BSD given in SPEC, couldn't find this in code, esp.
> COPYING does not mention BSD)
http://xcache.lighttpd.net/changeset/45
I added COPYING to %doc dir.
 
> * Mixing $RPM_BUILD_ROOT and %{buildroot} should not be done (cf. Packaging
> guidelines in Fedora wiki)
fixed

> * some code in SPEC is commented out, decreases readability. Why don't you
> remove those lines? Why is code for RHEL 4 and RHEL 5 included?
fixed

(In reply to comment #6)
> I recommend you to put this out of spec file.
> 
> %{__cat} > %{buildroot}%{_sysconfdir}/php.d/xcache.ini << 'EOF'
> [xcache-common]
> zend_extension = %{php_extdir}/xcache.so
> 
> [xcache.admin]
> xcache.admin.enable_auth = On
> xcache.admin.user = "mOo"
> ; xcache.admin.pass = md5($your_password)
> xcache.admin.pass = ""
> 
> [xcache]
> xcache.shm_scheme =        "mmap"
> xcache.size  =               60M
> xcache.count =                 1
> xcache.slots =                8K
> xcache.ttl   =              3600
> xcache.gc_interval =         300
> 
> xcache.var_size  =            4M
> xcache.var_count =             1
> xcache.var_slots =            8K
> xcache.var_ttl   =             0
> xcache.var_maxttl   =          0
> xcache.var_gc_interval =     300
> EOF    
fixed

spec: https://bugzilla.redhat.com/attachment.cgi?id=390219
srpm: https://bugzilla.redhat.com/attachment.cgi?id=390220
Comment 10 Till Maas 2010-02-11 06:39:24 EST
(In reply to comment #9)
> (In reply to comment #4)

> > * some code in SPEC is commented out, decreases readability. Why don't you
> > remove those lines? Why is code for RHEL 4 and RHEL 5 included?
> fixed

It's good to remove the commented out lines, but if there is a chance that this package can be included in EPEL[0], then the rhel conditionals are helpful.

[0] https://fedoraproject.org/wiki/EPEL
Comment 11 Till Maas 2010-02-11 06:41:10 EST
Just noticed this:
Instead of using /usr/share, please use %{_datadir}.
Comment 12 Timon 2010-02-11 07:38:59 EST
Created attachment 390235 [details]
php-xcache.spec
Comment 13 Timon 2010-02-11 07:40:03 EST
Created attachment 390236 [details]
php-xcache-1.3.0-3.fc12.src.rpm
Comment 14 Timon 2010-02-11 07:40:21 EST
(In reply to comment #11)
> Just noticed this:
> Instead of using /usr/share, please use %{_datadir}.    

fixed
Comment 15 Remi Collet 2010-02-13 11:49:50 EST
Quick notes

- don't understand why you try to register this as a pecl extension (%post / %postun). 
This is not a pecl extension, and you don't have the package.xml file required for this.


- so, could also be removed
%global pecl_name xcache
Requires(post): %{__pecl}
Requires(postun): %{__pecl}
Provides:      php-pecl(%{pecl_name}) = %{version}


- xcache.ini 
zend_extension = /EXT_DIR/xcache.so
no need to give full path.


- %defattr(-, root, root, 0755)
(-,root,root,-) must be enough. php script should be 644 
This will remove a lot of rpmlint message
php-xcache.x86_64: E: script-without-shebang /usr/share/php-xcache/admin/...


- admin / coverager
You need to create a alias (httpd/conf.d) to give access to admin URL.
probably also need a writable dir for xcache.coveragedump_directory

If you don't want to make this available, don't install it, just add it in %doc

- INSTALL
don't need this file in RPM

- Buildroot
is acceptable, but read
http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot
Comment 16 Itamar Reis Peixoto 2010-02-15 07:38:05 EST
Can you post a updated spec file + src.rpm ?

also can you include a koji scratch build ?
Comment 17 Timon 2010-02-16 05:29:51 EST
Created attachment 394498 [details]
php-xcache.spec
Comment 18 Timon 2010-02-16 05:30:41 EST
Created attachment 394499 [details]
php-xcache-1.3.0-4.fc12.src.rpm
Comment 19 Timon 2010-02-16 05:34:02 EST
(In reply to comment #15)
> Quick notes
> 
> - don't understand why you try to register this as a pecl extension (%post /
> %postun). 
> This is not a pecl extension, and you don't have the package.xml file required
> for this.
> - so, could also be removed
> %global pecl_name xcache
> Requires(post): %{__pecl}
> Requires(postun): %{__pecl}
> Provides:      php-pecl(%{pecl_name}) = %{version}
I used php-pecl-apc as template.
fixed

> - xcache.ini 
> zend_extension = /EXT_DIR/xcache.so
> no need to give full path.
http://ru2.php.net/manual/en/ini.core.php#ini.zend-extension
Absolute path required.

> - %defattr(-, root, root, 0755)
> (-,root,root,-) must be enough. php script should be 644 
> This will remove a lot of rpmlint message
> php-xcache.x86_64: E: script-without-shebang /usr/share/php-xcache/admin/...
fixed

> - admin / coverager
> You need to create a alias (httpd/conf.d) to give access to admin URL.
> probably also need a writable dir for xcache.coveragedump_directory
> If you don't want to make this available, don't install it, just add it in %doc
Fixed. I move them to %doc.

> - INSTALL
> don't need this file in RPM
fixed

> - Buildroot
> is acceptable, but read
> http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot
fixed. 
BuildRoot:     %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)


(In reply to comment #16)
> Can you post a updated spec file + src.rpm ?
php-xcache.spec: https://bugzilla.redhat.com/attachment.cgi?id=394498 
php-xcache-1.3.0-4.fc12.src.rpm: https://bugzilla.redhat.com/attachment.cgi?id=394499

> also can you include a koji scratch build ?    
[timon@timon rpmbuild]$ koji build --scratch dist-f12 SRPMS/php-xcache-1.3.0-4.fc12.src.rpm Uploading srpm: SRPMS/php-xcache-1.3.0-4.fc12.src.rpm
[====================================] 100% 00:00:05 107.71 KiB  21.09 KiB/sec
Created task: 1990279
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=1990279
None
Watching tasks (this may be safely interrupted)...
1990279 build (dist-f12, php-xcache-1.3.0-4.fc12.src.rpm): open (x86-06.phx2.fedoraproject.org)
  1990280 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, ppc): free
  1990281 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, x86_64): free
  1990280 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, ppc): free -> open (ppc06.phx2.fedoraproject.org)
  1990281 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, x86_64): free -> open (xb-01.phx2.fedoraproject.org)
  1990283 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, i686): open (x86-05.phx2.fedoraproject.org)
  1990282 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, ppc64): open (ppc10.phx2.fedoraproject.org)
  1990283 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, i686): open (x86-05.phx2.fedoraproject.org) -> closed
  0 free  4 open  1 done  0 failed
  1990281 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, x86_64): open (xb-01.phx2.fedoraproject.org) -> closed
  0 free  3 open  2 done  0 failed
  1990282 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, ppc64): open (ppc10.phx2.fedoraproject.org) -> closed
  0 free  2 open  3 done  0 failed
  1990280 buildArch (php-xcache-1.3.0-4.fc12.src.rpm, ppc): open (ppc06.phx2.fedoraproject.org) -> closed
  0 free  1 open  4 done  0 failed
1990279 build (dist-f12, php-xcache-1.3.0-4.fc12.src.rpm): open (x86-06.phx2.fedoraproject.org) -> closed
  0 free  0 open  5 done  0 failed

1990279 build (dist-f12, php-xcache-1.3.0-4.fc12.src.rpm) completed successfully
Comment 20 Jason Tibbitts 2011-01-10 18:37:50 EST
No response for months in another of the submitter's tickets; closing them all.