[Bug 488185] Review Request: php-pecl-selinux - SELinux binding for PHP scripts

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=488185





--- Comment #4 from KaiGai Kohei <kaigai at kaigai.gr.jp>  2009-03-05 04:55:07 EDT ---
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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.