Bug 488185 - Review Request: php-pecl-selinux - SELinux binding for PHP scripts
Summary: Review Request: php-pecl-selinux - SELinux binding for PHP scripts
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL: http://pecl.php.net/package/selinux
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-03-03 02:55 UTC by KaiGai Kohei
Modified: 2009-03-10 02:21 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-03-10 02:21:12 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description KaiGai Kohei 2009-03-03 02:55:11 UTC
Spec URL: http://sepgsql.googlecode.com/files/php-pecl-selinux.spec
SRPM URL: http://sepgsql.googlecode.com/files/php-selinux-0.1.2-devel.fc10.src.rpm
Description:
This package provides a set of interfaces to communicate between
SELinux and PHP scripts via libselinux.
It enables PHP scripts the following stuffs.
- get/set a security context of processes and other resources
- get/set system booleans
- make a query for in-kernel security server
- translate form of security context between 'raw' and 'translated'

It shows the list of APIs:
http://code.google.com/p/sepgsql/wiki/Memo_PHP_SELinux

Comment 1 KaiGai Kohei 2009-03-04 02:55:59 UTC
The result of rpmlint:

[kaigai@saba ~]$ rpmlint /home/kaigai/RPMS/SRPMS/php-pecl-selinux-0.1.2-devel.fc10.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[kaigai@saba ~]$ rpmlint /home/kaigai/RPMS/RPMS/i386/php-pecl-selinux-0.1.2-devel.fc10.i386.rpm
php-pecl-selinux.i386: W: incoherent-version-in-changelog 0.1.2 ['0.1.2-devel.fc10', '0.1.2-devel']
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

It claims "0.1.2" is noted on %changelog, although it is "0.1.2-devel.fc10".
IIRC, it can be an acceptable warnings, isn't it?

Comment 2 Mamoru TASAKA 2009-03-04 19:54:09 UTC
(Removing NEEDSPONSOR)

Well, I am familiar with neither php nor selinux, however
some comments

* rpm name
  - Please make Name consistent first.
    - I guess this rpm should be named as "php-pecl-selinx" as
      the spec file suggests.
    - However currently Name uses "php-selinux".

* Versioning
  - If this is the pre-release of formal 0.1.2 release,
    please follow
    https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
    (Anyway using "devel" as Release seems strange)

* %__pecl
  - To build this package on koji,
------------------------------------------------------
%{!?__pecl:     %{expand: %%global __pecl     %{_bindir}/pecl}}
------------------------------------------------------
    cannot be removed because
    - When buildroot is initialized, no PHP related rpms
      are installed yet, so %__pecl is not defined at this stage.
    - Then mock tries "rpm -bs --nodeps foo.spec".
      Then rpm complains like
------------------------------------------------------
error: line 14: Dependency tokens must begin with alpha-numeric, '_' or '/': Requires(post): %{__pecl}
------------------------------------------------------

* %if %{?php_zend_api}0
  - Well, actually Fedora guideline actually suggests so, however
    generally this should be "if 0%{?php_zend_api}" (no deference
    for this case, however this is usual usage)

* BR (BuildRequires)
  - Would you check if the following message in build.log ignored?
------------------------------------------------------
    81  checking for re2c... no
    82  configure: WARNING: You will need re2c 0.13.4 or later if you want to regenerate PHP parsers.
------------------------------------------------------

* %post scriptlet
------------------------------------------------------
%post
%{pecl_install} %{pecl_xmldir}/%{name}.xml >/dev/null || :
%endif
------------------------------------------------------
  - However %{pecl_xmldir}/%{name}.xml does not seem to be
    installed.

Comment 3 Mamoru TASAKA 2009-03-04 20:03:57 UTC
(In reply to comment #2)
> * BR (BuildRequires)
>   - Would you check if the following message in build.log ignored?

if the following messages can be ignored?

Comment 4 KaiGai Kohei 2009-03-05 09:55:07 UTC
Tasaka-san,
Thanks for your reviewing.

I uploaded the revised version:
Spec: http://sepgsql.googlecode.com/files/php-pecl-selinux.spec.20090305
SRPM: http://sepgsql.googlecode.com/files/php-pecl-selinux-0.1.2-1.fc10.src.rpm

> * rpm name
>  - Please make Name consistent first.
>    - I guess this rpm should be named as "php-pecl-selinx" as
>      the spec file suggests.
>    - However currently Name uses "php-selinux".

Sorry, it was my misoperation.
The newer package uses "php-pecl-selinuc".

> * Versioning
>  - If this is the pre-release of formal 0.1.2 release,
>    please follow
>    https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
>    (Anyway using "devel" as Release seems strange)

Fixed. The "devel" was just a copy of PECL library.

> * %__pecl
>   - To build this package on koji,
> ------------------------------------------------------
> %{!?__pecl:     %{expand: %%global __pecl     %{_bindir}/pecl}}
> ------------------------------------------------------
>     cannot be removed because
>     - When buildroot is initialized, no PHP related rpms
>       are installed yet, so %__pecl is not defined at this stage.
>     - Then mock tries "rpm -bs --nodeps foo.spec".
>       Then rpm complains like
> ------------------------------------------------------
> error: line 14: Dependency tokens must begin with alpha-numeric, '_' or '/':
> Requires(post): %{__pecl}
> ------------------------------------------------------

Fixed, I added the definition at the head of specfile.

> * %if %{?php_zend_api}0
>   - Well, actually Fedora guideline actually suggests so, however
>     generally this should be "if 0%{?php_zend_api}" (no deference
>     for this case, however this is usual usage)

Fixed.

> * BR (BuildRequires)
>   - Would you check if the following message in build.log ignored?
> ------------------------------------------------------
>     81  checking for re2c... no
>     82  configure: WARNING: You will need re2c 0.13.4 or later if you want to
regenerate PHP parsers.
> ------------------------------------------------------

The "re2c" is a parser engine, so this package has no relations.
Now I asks for PHP experts to confirm whether my understanding is correct, or not.
  http://marc.info/?l=pecl-dev&m=123621647005625&w=2

> * %post scriptlet
> ------------------------------------------------------
> %post
> %{pecl_install} %{pecl_xmldir}/%{name}.xml >/dev/null || :
> %endif
> ------------------------------------------------------
>   - However %{pecl_xmldir}/%{name}.xml does not seem to be
>     installed.

I added to install package.xml as %{pecl_xmldir}/%{name}.xml

Comment 5 KaiGai Kohei 2009-03-05 09:57:01 UTC
(In reply to comment #4)
> The newer package uses "php-pecl-selinuc".

s/php-pecl-selinuc/php-pecl-selinux/g

Comment 6 Mamoru TASAKA 2009-03-05 10:26:32 UTC
Assigning.

Comment 7 Mamoru TASAKA 2009-03-05 16:58:01 UTC
Okay, two issues/questions

* Source tarball
  - source tarball in your srpm differs from what I could download
    from the URL written in your spec file.
    Does this mean that the source tarball used is the pre-release
    of 0.1.2? If so, please follow "Pre-release package" naming guideline.

* %changelog
  - EVR (Epoch-Version-Release) information in %changelog differs
    from the actual EVR of this rpm. Please fix it.

Comment 8 KaiGai Kohei 2009-03-06 01:54:54 UTC
> The "re2c" is a parser engine, so this package has no relations.
> Now I asks for PHP experts to confirm whether my understanding is correct,
> or not.
>   http://marc.info/?l=pecl-dev&m=123621647005625&w=2

PHP expert also agreed to ignore this warning in this package.
  http://marc.info/?l=pecl-dev&m=123627059603922&w=2

(In reply to comment #7)
> Okay, two issues/questions
> 
> * Source tarball
>   - source tarball in your srpm differs from what I could download
>     from the URL written in your spec file.
>     Does this mean that the source tarball used is the pre-release
>     of 0.1.2? If so, please follow "Pre-release package" naming guideline.

Sorry, it was the regenerated tarball from CVS repos in same version by my hand.
The correct tarball is the one uploaded at:
  http://pecl.php.net/selinux

It was fixed on updated SRPM.

> * %changelog
>   - EVR (Epoch-Version-Release) information in %changelog differs
>     from the actual EVR of this rpm. Please fix it.  

Oops, "x.y.z" was "x.z.y".
Fixed.

The updated packages are here:
Spec: http://sepgsql.googlecode.com/files/php-pecl-selinux.spec.20090306
SRPM: http://sepgsql.googlecode.com/files/php-pecl-selinux-0.1.2-1.fc10.src.rpm

Thanks,

Comment 9 Mamoru TASAKA 2009-03-06 14:09:46 UTC
Okay:

----------------------------------------------------------------
     This package (php-pecl-selinux) is APPROVED by mtasaka
----------------------------------------------------------------

Please follow
http://fedoraproject.org/wiki/New_package_process_for_existing_contributors
from Step 7.

Comment 10 KaiGai Kohei 2009-03-08 15:04:43 UTC
Thanks for your reviewing!

New Package CVS Request
=======================
Package Name: php-pecl-selinux
Short Description: SELinux binding for PHP scripting language
Owners: kaigai
Branches: F-9 F-10
InitialCC: kaigai.nec.com

Comment 11 Kevin Fenzi 2009-03-09 16:03:14 UTC
cvs done.

Comment 12 KaiGai Kohei 2009-03-10 02:21:12 UTC
Tasaka-san,

Thanks for your great helps!


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