Bug 488185 - Review Request: php-pecl-selinux - SELinux binding for PHP scripts
Review Request: php-pecl-selinux - SELinux binding for PHP scripts
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
http://pecl.php.net/package/selinux
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-02 21:55 EST by KaiGai Kohei
Modified: 2009-03-09 22:21 EDT (History)
5 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description KaiGai Kohei 2009-03-02 21:55:11 EST
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-03 21:55:59 EST
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 14:54:09 EST
(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 15:03:57 EST
(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 04:55:07 EST
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 04:57:01 EST
(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 05:26:32 EST
Assigning.
Comment 7 Mamoru TASAKA 2009-03-05 11:58:01 EST
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-05 20:54:54 EST
> 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 09:09:46 EST
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 11:04:43 EDT
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@ak.jp.nec.com
Comment 11 Kevin Fenzi 2009-03-09 12:03:14 EDT
cvs done.
Comment 12 KaiGai Kohei 2009-03-09 22:21:12 EDT
Tasaka-san,

Thanks for your great helps!

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