Bug 455226 - Review Request: php-pecl-runkit - PHP Opcode Analyser
Review Request: php-pecl-runkit - PHP Opcode Analyser
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-14 04:26 EDT by Pavel Alexeev
Modified: 2009-06-14 12:57 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-14 12:57:42 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pavel Alexeev 2008-07-14 04:26:36 EDT
Spec URL: http://hubbitus.net.ru/rpm/Fedora9/php-pecl-runkit/php-pecl-runkit.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora9/php-pecl-runkit/php-pecl-runkit-0.9-0.CVS20080512.fc8.Hu.5.src.rpm

Description: Provides a userspace interpretation of the opcodes generated by the Zend engine compiler built into PHP.
This extension is meant for development and debug purposes only and contains some code which is potentially non-threadsafe.

Small note: I'm read naming guide and understand it, but in my own rpm-repository (http://hubbitus.net.ru/rpm) all my packages with changes made have portion of my release like ".Hu.<number>". This addon of release made to differ version from upstream packages.

This is my 2nd (in Fedora package review, not in packaging history at all) package and I am looking for sponsor.
Comment 1 Pavel Alexeev 2008-07-14 04:36:14 EDT
I am very, very apologize. I mix with the description php-pecl-parsekit.
Correct are:
Short description: php-pecl-runkit - Extension to mangle with user defined
functions and classes

Long description:
Replace, rename, and remove user defined functions and classes.
Define customized superglobal variables for general purpose use.
Execute code in restricted environment (sandboxing).
Comment 2 Mamoru TASAKA 2008-08-25 03:46:19 EDT
(Removing NEEDSPONSOR: bug 455067)
Comment 3 Remi Collet 2008-11-23 03:35:07 EST
Small question :

Do you think it is a good idea to import a very old package which doesn't seem maintained upstream for quite a long time (june 2006).


Small notes :

Version 0.9 is available, so you cannot use a pre-release version number (0.9-0.xxx) but a post release (Official 0.9 will have a 0.9-1 number schema)

Please add in comment the "cvs export" command with the revision or date retrieved.

Also indicate where the patch come from. If they are needed to build against latest php version, you should file a bug upstream with your patch attached (but I see in CSV php 5.3.0 compatibility have been fixed for a few months).

+
Comment 4 Remi Collet 2008-11-23 03:46:34 EST
Also read the PHP Guidelines (and/or read other pecl spec file)
http://fedoraproject.org/wiki/Packaging/PHP

You must have the requires for ABI version

.hu should be removed from release.
Comment 5 Pavel Alexeev 2008-11-23 05:50:28 EST
Firstly - thank you for comment.

(In reply to comment #3)
> Small question :
> 
> Do you think it is a good idea to import a very old package which doesn't seem
> maintained upstream for quite a long time (june 2006).
This is work now and have not major bugs which must be fixed by upstream. Furthermore, I do not known any alternatives by functionality yet. So, answer is yes, I think is it is a very good idea to import this package.


> Small notes :
> 
> Version 0.9 is available, so you cannot use a pre-release version number
> (0.9-0.xxx) but a post release (Official 0.9 will have a 0.9-1 number schema)
Hm. It is build from CVS. How I should number its versions - 0.9-1.CVS20080512, 0.9-2.CVS20080512, 0.9-3.CVS20080512? Or 0.9-1.1.CVS20080512, 0.9-1-2.CVS20080512, 0.9-1-3.CVS20080512?

> Please add in comment the "cvs export" command with the revision or date
> retrieved.
Excuse me, I do not understand. Date of CVS retrieving present in version and in defined macros
%define	CVS	20080512
in spec file. For what and where more I should comment it?

> Also indicate where the patch come from.
This is my patches. And it is reflected in spec changelog.

> If they are needed to build against
> latest php version, you should file a bug upstream with your patch attached
> (but I see in CSV php 5.3.0 compatibility have been fixed for a few months).
Patch not for PHP, patch for this extension to make it compatible with new PHP ABI.
So, if we speak about file bug to upstream of this pecl extension - this have not sense, because you said before what it is old and unmaintained.
Comment 6 Remi Collet 2009-01-22 13:18:33 EST
> How I should number its versions

0.9-1.1.CVS20080512 seems ok.

> For what and where more I should comment it?

I'd like to see the exact commands use to build the tarball, just above the %source., p.e. (export greater than checkout) :

# cvs -d :pserver:cvsread@cvs.php.net/repository export -D 2009-01-22 pecl/runkit
# tar cjf runkit-CVS20090122.tar.bz2 -C pecl runkit
%Source0:		%{peclName}-CVS%{CVS}.tar.bz2

Don't use an URL which doesn't exists

I have well understood than patches are for PHP ABI.

Even if this package is unmaintained, please report the bug and post your patch(es), it will be usefull for everyone, and probably commited (last commit is only 5 weeks old on runkit.c).

I don't think restarting apache for each extension is a good idea. This should probably be removed (Have to search about this in the Guidelines).

Please :
- clean release (remove .Hu... and probably not usefull #*Hu comments)
- update to a recent CVS snapshot 
- register the extension (package2.xml is included)
- add PHP ABI check (see PHP Guidelines)
- add upstream bug reference above %patch
- use Fedora macros %pecl_xmldir, %php_extdir, %pecl_install, ...
- clean $Revision and $Log cvs status lines (spec is quite obfuscated)
- clean changelog (mainly % not acceptable)
Comment 7 Pavel Alexeev 2009-02-15 15:20:24 EST
(In reply to comment #6)
> > How I should number its versions
> 
> 0.9-1.1.CVS20080512 seems ok.
No, duedlines say what CVS, not released versions must start from 0.

I change it to 0.9-0.1.CVS20080512 enumeration.

> I'd like to see the exact commands use to build the tarball, just above the
> %source., p.e. (export greater than checkout) :
> 
> # cvs -d :pserver:cvsread@cvs.php.net/repository export -D 2009-01-22
> pecl/runkit
> # tar cjf runkit-CVS20090122.tar.bz2 -C pecl runkit
> %Source0:  %{peclName}-CVS%{CVS}.tar.bz2
Thank you, its done.


> I have well understood than patches are for PHP ABI.
> 
> Even if this package is unmaintained, please report the bug and post your
> patch(es), it will be usefull for everyone, and probably commited (last commit
> is only 5 weeks old on runkit.c).
Ok - http://pecl.php.net/bugs/bug.php?id=15969


> I don't think restarting apache for each extension is a good idea. This should
> probably be removed (Have to search about this in the Guidelines).
I agree. This comes as legacy. Restart is removed.


> Please :
> - clean release (remove .Hu... and probably not usefull #*Hu comments)
Done in release. In comments i think it is not necessary?

> - update to a recent CVS snapshot
Done.
> - register the extension (package2.xml is included)
Done.
> - add PHP ABI check (see PHP Guidelines)
Done.
> - add upstream bug reference above %patch
Done - http://pecl.php.net/bugs/bug.php?id=15969
> - use Fedora macros %pecl_xmldir, %php_extdir, %pecl_install, ...
Done.
> - clean $Revision and $Log cvs status lines (spec is quite obfuscated)
Done.
> - clean changelog (mainly % not acceptable)
Done.

http://hubbitus.net.ru/rpm/Fedora9/php-pecl-runkit/php-pecl-runkit-0.9-0.6.CVS20090215.fc9.src.rpm
Comment 8 Pavel Alexeev 2009-02-15 15:21:07 EST
Please, excuse me for long delay.
Comment 9 Remi Collet 2009-02-22 03:25:42 EST
> Please, excuse me for long delay.

Not a problem, I'm also actually very busy

> No, duedlines say what CVS, not released versions must start from 0.

Definitely, I can't agree.
Version 0.9 have been published on 2006-06-06

So this is a post-release version
Read : http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

I prefer using %{name}.xml rather than %{peclName}.xml (or %{peclName}2.xml) (see recent approved changes in PHP Guidelines, this is to avoid possible conflicts between pecl, pear and other channel extensions).

I need to search if %verify(not md5 mtime size) is acceptable in the Guidelines...

Is the spec encoding ok ? It seems there is UTF-8 (ru sumnary) and ISO (pl sumnary) which make my text editor crazy ?

$ file php-pecl-runkit.spec 
php-pecl-runkit.spec: Non-ISO extended-ASCII text, with LF, NEL line terminators
Comment 10 Pavel Alexeev 2009-02-22 18:06:30 EST
(In reply to comment #9)
> > No, duedlines say what CVS, not released versions must start from 0.
> Definitely, I can't agree.
> Version 0.9 have been published on 2006-06-06
> So this is a post-release version
> Read :
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages
Ok, you are right. Version release enumaration changed.

> I prefer using %{name}.xml rather than %{peclName}.xml (or %{peclName}2.xml)
> (see recent approved changes in PHP Guidelines, this is to avoid possible
> conflicts between pecl, pear and other channel extensions).
Ok, let it be so.

> I need to search if %verify(not md5 mtime size) is acceptable in the
> Guidelines...
> 
> Is the spec encoding ok ? It seems there is UTF-8 (ru sumnary) and ISO (pl
> sumnary) which make my text editor crazy ?
Polish summary and description recoded from iso8859-2 to UTF-8. 

> $ file php-pecl-runkit.spec 
> php-pecl-runkit.spec: Non-ISO extended-ASCII text, with LF, NEL line
> terminators
Now:
$ file php-pecl-runkit.spec
php-pecl-runkit.spec: UTF-8 Unicode text


http://hubbitus.net.ru/rpm/Fedora9/php-pecl-runkit/php-pecl-runkit-0.9-7.CVS20090215.fc9.src.rpm
Comment 11 Remi Collet 2009-02-24 11:13:36 EST
Why do you use 
    install modules/%{peclName}.so %{buildroot}%{php_extdir}

Rather than (which also create the destination dir)
    make install INSTALL_ROOT=$RPM_BUILD_ROOT


Doesn't build for me :
- %doc %{peclName}-%{version}/README
+ %doc %{peclName}/README

rpmlint : 
php-pecl-runkit.src: I: checking
php-pecl-runkit.src:39: W: unversioned-explicit-obsoletes php-pear-%{peclName}
php-pecl-runkit.src: W: summary-not-capitalized runkit - mangle with user defined functions and classes
php-pecl-runkit.src: W: non-standard-group Development/Languages/PHP
php-pecl-runkit.x86_64: I: checking
php-pecl-runkit.x86_64: W: summary-not-capitalized runkit - mangle with user defined functions and classes
php-pecl-runkit.x86_64: W: non-standard-group Development/Languages/PHP
php-pecl-runkit.x86_64: W: obsolete-not-provided php-pear-runkit
php-pecl-runkit-debuginfo.x86_64: I: checking
php-pecl-runkit.spec:39: W: unversioned-explicit-obsoletes php-pear-%{peclName}
3 packages and 1 specfiles checked; 0 errors, 7 warnings.

Most of them can be fixed easily

- stuff php-pear-%{peclName} is not usefull in Fedora (php-pear-runkit don't exists AFAIK)
- %{peclName} could be removed from desc and first word capitalized.
Comment 12 Pavel Alexeev 2009-02-26 02:28:33 EST
(In reply to comment #11)
> Why do you use 
>     install modules/%{peclName}.so %{buildroot}%{php_extdir}
> 
> Rather than (which also create the destination dir)
>     make install INSTALL_ROOT=$RPM_BUILD_ROOT
I do not remember now. It have any sense?

> Doesn't build for me :
> - %doc %{peclName}-%{version}/README
> + %doc %{peclName}/README
Thank you. Fixed.

> rpmlint : 
> php-pecl-runkit.src: I: checking
> php-pecl-runkit.src:39: W: unversioned-explicit-obsoletes php-pear-%{peclName}
> php-pecl-runkit.src: W: summary-not-capitalized runkit - mangle with user
> defined functions and classes
Name deleteed from all summary and capitalized.

> php-pecl-runkit.src: W: non-standard-group Development/Languages/PHP
Changed to Development/Libraries
> php-pecl-runkit.x86_64: I: checking
> php-pecl-runkit.x86_64: W: summary-not-capitalized runkit - mangle with user
> defined functions and classes
Fixed, see before.

> - stuff php-pear-%{peclName} is not usefull in Fedora (php-pear-runkit don't
> exists AFAIK)
Excuse me, I think do not understand you. You speak about unnecessary Obsoletes: php-pear-%{peclName}? I think you are rigth, it may be safely removed in this case. I do it.

rpmlint silent now. I am sorry that is not checked immediately.

http://hubbitus.net.ru/rpm/Fedora9/php-pecl-runkit/php-pecl-runkit-0.9-8.CVS20090215.fc9.src.rpm
Comment 13 Pavel Alexeev 2009-02-26 02:33:48 EST
Hm, It build successful on my box, but failed unexpected in koji http://koji.fedoraproject.org/koji/getfile?taskID=1183646&name=build.log

As seen in log, macros %{pecl_xmldir} not replaced by it own value. Strange. More strange in this situation what php-pecl-imagick package also uses it macros and therefor built correctly...

Until now I don't known what to do...
Comment 15 Remi Collet 2009-03-14 04:21:39 EDT
+ rpmlint is ok
php-pecl-runkit.i386: I: checking
php-pecl-runkit.src: I: checking
php-pecl-runkit-debuginfo.i386: I: checking
3 packages and 1 specfiles checked; 0 errors, 0 warnings.
+ package name
+ spec file name 
+ package meet the PHP Guidelines (except runkit.xml/php-pecl-runkit.xml)
+ License ok : PHP
+ License is upstream 
+ spec in english (+ru +pl) and legible
+ license file in sources is not provided
+ sources match the upstream sources from CVS 20090215
+ Source URL : CVS snapshot with comment
+ build  on F10.x86_64 (php 5.3)
+ BuildRequires ok
+ no locale
+ no .so
+ own all directories that it creates
+ no duplicate file
- %defattr (644,root,root,755)
+ %clean section
+ use macros consistently
+ contain code
+ small documentation
+ no devel
+ no pkgconfig
+ no sub-package
+ no GUI
+ don't own files or directories already owned by other packages
+ %install start with rm -rf 
+ valid UTF-8
+ build in mock (F10 i386 / php 5.2.9)
+ build in koji (rawhide / php 5.2.9)
- test suite doesn't run
- scriptlets register/unregister ok but not silent
+ Final Requires ok
/bin/sh  
/usr/bin/pecl  
php(api) = 20041225
php(zend-abi) = 20060613
+ Final Provides ok
php-pecl(runkit) = 0.9

Don't understand why you switch back to runkit.xml (rather then php-pecl-runkit.xml). Not a must (new Guidelines approved by FPC but not yet inline)
http://fedoraproject.org/wiki/PackagingDrafts/PHP

I can't run the test suite, probably because I use PHP 5.3, what are the result under PHP 5.2 ?

MUST : 
make the %post/postun scriptlet silent
use the default %defattr(-,root,root,-)
Comment 16 Remi Collet 2009-03-14 04:23:01 EDT
+ no .so

=> Of course I mean no shared lib which need to be in a -devel subpackage, only a PHP extension. So OK.
Comment 17 Pavel Alexeev 2009-03-17 06:43:53 EDT
Ok, ok, I rename xml-file again into php-pecl-runkit.xml.

I can run test on php even 5.3 (from your repository ;) ), but I think included suite is very outdated, and not seen any passed tests. So, it is a main reason why I do not include in spec test phase.

Set %defattr(-,root,root,-)

(In reply to comment #15)
> MUST : 
> make the %post/postun scriptlet silent
It is done. BUT why??? I prefer see errors if it is present. It is also guarantee to fast bug-report if something will wrong on user system...
In provided before link to Guidelines such scripts marked only as recommended, not mandatory.

http://hubbitus.net.ru/rpm/Fedora9/php-pecl-runkit/php-pecl-runkit-0.9-10.CVS20090215.fc9.src.rpm
Comment 18 Remi Collet 2009-03-22 13:35:12 EDT
> I prefer see errors if it is present.

Redirection of standard output (what you've done) doesn't suppress output of error ;) but only unuseful information about (un)registration.

All MUST (and should) are fixed, so

APPROVED
Comment 19 Pavel Alexeev 2009-03-22 13:49:45 EDT
Remi Collet, thank you for review.

New Package CVS Request
=======================
Package Name: php-pecl-runkit
Short Description: PHP Opcode Analyser
Owners: Hubbitus
Branches: F9 F10 EL5
InitialCC:
Comment 20 Kevin Fenzi 2009-03-24 13:15:31 EDT
cvs done.
Comment 21 Fedora Update System 2009-04-19 07:19:54 EDT
php-pecl-runkit-0.9-10.CVS20090215.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/php-pecl-runkit-0.9-10.CVS20090215.fc9
Comment 22 Fedora Update System 2009-04-19 07:21:00 EDT
php-pecl-runkit-0.9-10.CVS20090215.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/php-pecl-runkit-0.9-10.CVS20090215.fc10
Comment 23 Pavel Alexeev 2009-04-19 08:35:46 EDT
New branch please.

Package Change Request
======================
Package Name: php-pecl-runkit
New Branches: F-11
Owners: hubbitus
Comment 24 Kevin Fenzi 2009-04-21 15:42:01 EDT
There already is a F-11 branch. 

Note that you may have to do a 'cvs update -d' to see it locally.
Comment 25 Pavel Alexeev 2009-04-22 08:24:40 EDT
It is, but from times when it be "rawhide" if you understand me.
Now, after sucessful build, I try make update and got error:
php-pecl-runkit-0.9-10.CVS20090215.fc11 not tagged as an update candidate

Where I wrong?
Comment 26 Remi Collet 2009-04-22 10:32:04 EDT
You probably forget to "cvs update" to get the new branch.

In devel, package should be tagged F12.

+
Comment 27 Pavel Alexeev 2009-04-22 10:52:17 EDT
No, "cvs ud -d" was runned few times.

And oof course, I known what now devel is F-12, it is a reason why I think what F-11 should be added separately.

But, when I try tag it, I get error what this tag applied on a different branch:

$ make tag
cvs tag  -c php-pecl-runkit-0_9-10_CVS20090215_fc11
ERROR: The tag php-pecl-runkit-0_9-10_CVS20090215_fc11 is already applied on a different branch
ERROR: You can not forcibly move tags between branches
php-pecl-runkit-0_9-10_CVS20090215_fc11:devel:hubbitus:1237926729
php-pecl-runkit-0_9-10_CVS20090215_fc9:F-9:hubbitus:1237929134
php-pecl-runkit-0_9-10_CVS20090215_fc10:F-10:hubbitus:1237929597
php-pecl-runkit-0_9-10_CVS20090215_el5:EL-5:hubbitus:1237933086
php-pecl-runkit-0_9-11_CVS20090215_el5:EL-5:hubbitus:1240139384
cvs tag: Pre-tag check failed
cvs [tag aborted]: correct the above errors first!
make: *** [tag] error 1

Can I move tags? Or what I should do now?
Comment 28 Fedora Update System 2009-05-08 23:54:16 EDT
php-pecl-runkit-0.9-10.CVS20090215.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 29 Fedora Update System 2009-05-09 00:17:32 EDT
php-pecl-runkit-0.9-10.CVS20090215.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 30 Mamoru TASAKA 2009-06-14 12:57:42 EDT
This package is already imported.

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