[Bug 181445] Review Request: php-shout

[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 report.

Summary: Review Request: php-shout


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





------- Additional Comments From holbrookbw at users.sourceforge.net  2006-03-30 02:41 EST -------
http://prdownloads.sourceforge.net/phpshout/php-shout.spec?download
http://prdownloads.sourceforge.net/phpshout/php-shout-0.3a-1.src.rpm?download

Thanks Matthias for your offer for sponsorship, see my new packages above and my
comments below...

(In reply to comment #20)
> I cannot sponsor you, but as I have some skills in php packaging I do some
> pre-review work here.
> 
> Remarks/nitpicks:
> - It seems that included tarball contains a lot of extra data (it is about 3.6Mb
> before "phpize --clean" and just 360kb after it). Maybe better to ship just
> pre-phpize tarball as a source.
> 

Done.  version 0.3a now ships clean, without a 'phpize', SRPM is now ~260K

> - .spec file must not contain version/release, rename it to "php-shout.spec"

This was done because SourceForge does not allow you publish files with the same
filename, even across separate releases.  Upon approval, I would of course
upload php-shout.spec to CVS.  Nonetheless, this most recent spec file has been
renamed as well.

> - remove leading "A" in the Summary.

done

> - IMHO, the Summary can sounds better: "PHP module for communicating with
> icecast servers" or something similar.

done

> - %{php_version} macro is not needed, %{php_extdir} macro can be cleaned a little
> - Since php-5.1.2-5, the new virtual is provided: "php-api". It should be used
> for dependencies. An appropriate macro %{apiver} can be auto-determined (see in
> the patch below).

I like this approach much better, your changes have been implemented

> - There is a test stuff in the tarball ("./tests" subdir). After phpize'ing
> under FC5, it will be possible to run "make test" in the %{check} section
> (instead of the current fake one derived from php-json package ;)).
>  Currently "make test" fails for me. Perhaps some additional work should be done
> before the test is run. Anyway, if tests are present, they should be run at
> rpmbuild time to make sure that all is OK.

I have changed %check to execute a 'make test' for consistency across modules,
however note that in most circumstances the php-shout tests will fail.  They
attempt to connect to an icecast server and issue commands, by default on
localhost with a generic name and password.  If your machine is not running an
icecast server with default security settings, the tests will not be able to
connect, and will fail.  In order to properly execute 'make test', you must
first have access to an icecast server, and then edit the connection setting in
tests/connect.inc.  I intend on making a VERY basic test that simply checks for
the ability to load the shout.so moduleso that SOMETHING passes, but haven't
gotten around to it yet.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.