Bug 181445 - Review Request: php-shout
Review Request: php-shout
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-02-14 01:58 EST by Brandon Holbrook
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-16 18:15:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Suggestions and nitpicks for the spec file (1.58 KB, patch)
2006-03-28 07:58 EST, Dmitry Butskoy
no flags Details | Diff
Spec file patch (1.81 KB, patch)
2006-03-30 12:09 EST, Matthias Saou
no flags Details | Diff

  None (edit)
Description Brandon Holbrook 2006-02-14 01:58:06 EST
Spec Name or Url: http://prdownloads.sourceforge.net/phpshout/php-shout.spec?download
SRPM Name or Url: http://prdownloads.sourceforge.net/phpshout/php-shout-0.1.3-1.src.rpm?download
Description:

php-shout is a PHP extension that wraps the libshout library. Libshout is used to collect and stream audio data to an Icecast Streaming Media server. php-shout allows PHP developers to use native libshout functions from within their PHP scripts. This will ultimately allow developers to write PHP-powered web jukebox applications for the storage and on-demand streaming of their personal music libraries. While writing these applications, php-shout allows developers to concentrate on a richer set of features, without the need to worry about the details of the Icecast server communication.

NOTE: php-shout was designed to work with libshout 2.x (2.0 was released Jul 2003), but Fedora Extras currently contains libshout-devel-1.0.9. Development ceased on Libshout 1.x in Apr 2002, and has been deprecated in favor of libshout 2.x.  I have contacted the maintainer of libshout-devel about upgrading to libshout-2.2, but have not received a response.  The inclusion of php-shout WILL require an updated version of libshout-devel :-/  If the package has been orphaned, I will gladly adopt it, and update it (pending community approval).
Comment 1 Jochen Schmitt 2006-02-14 14:54:06 EST
I will suggest, that you file bugzilla with a regquest for updating libshout.
Comment 2 Brandon Holbrook 2006-02-15 22:13:26 EST
I should also mention that this is my first submission, so I need a sponsor /
mentor :)
Comment 3 Dmitry Butskoy 2006-02-16 07:04:38 EST
Just some remarks:
- it is better to do "phpize --clean; phpize" anyway, as it adjust the source to
the appropriate php-devel build system (4.3, or 5.0, or 5.1 etc...)
- Specifying "requires: libshout >= 2.0.0" is extra, as "BR: libshout-devel >=
2.0.0" is already present, which cause to build with and autodepend by the
needed library version.
Comment 4 Brandon Holbrook 2006-02-16 17:21:35 EST
Spec Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout.spec?download
SRPM Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.1.4-1.src.rpm?download

Thanks for the review!  Both of these issues have been fixed in this next
release.  I also made some other ./configure fixes to better detect libshout's
compile-time dependencies with pkg-config and link in the appropriate libraries,
resulting in release 0.1.4

Comment 5 Brandon Holbrook 2006-02-16 17:52:59 EST
One other thing, when I started this PHP extension project, I named it
"phpShout", and registered as such under sourceforge.  However, when packaging
it up for Fedora, it made much more sense to name the package "php-shout", in
the age-old 'package-subpackage' format.  Is this a violation of FE's Naming
Guidelines, that the RPM doesn't match the tarball?

The Wiki told me to "use your best judgment", and that "other developers will
help in the final decision."  I'd much prefer to call it php-shout, and CAN
rename the upstream source for future versions.
Comment 6 Ville Skyttä 2006-02-17 01:49:04 EST
php-shout is fine.
Comment 7 Dmitry Butskoy 2006-02-17 07:00:36 EST
The Fedora style of the name of the php-module-packages is "php-MODULE_NAME",
irrespective of how they are named upstream. (It seems to be some precedent rule).

php-shout is fine.
Comment 8 Brandon Holbrook 2006-02-23 01:36:34 EST
Spec Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.1.5-1.fc4.spec?download
SRPM Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.1.5-1.fc4.src.rpm?download

Some bugfixes upstream bumped php-shout to 0.1.5, so I rolled some new RPMs. 
Another review/sponsor would be appreciated!  Also, I'm still trying to contact
Thomas Vander Stichele with regards to updating libshout to 2.2 for fe5 (bz
181523), but I haven't heard a word.  Is there a statute for how long a
developer can go without responding before we make decisions for him?...
Comment 9 Brandon Holbrook 2006-03-05 00:07:05 EST
Spec Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.2.1-1.fc4.spec?download
SRPM Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.2.1-1.fc4.src.rpm?download

In preparation for Review Day, I'm re-submitting the latest version of php-shout
for review.  Happy Review Day!
Comment 10 Brandon Holbrook 2006-03-11 13:19:55 EST
Spec Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.3-1.spec?download
SRPM Name or Url:
http://prdownloads.sourceforge.net/phpshout/php-shout-0.3-1.src.rpm?download

Re-submitting the latest version of php-shout for review.  Now that Thomas has
updated libshout to 2.2, php-shout should compile and build cleanly on devel,
and the review process can continue.
Comment 11 Brandon Holbrook 2006-03-12 23:55:37 EST
I now have a Fedora Account and am in cla_done... just waiting on some reviews

/me twiddles his thumbs
Comment 12 Dmitry Butskoy 2006-03-14 10:22:17 EST
(for comment #10):
> ...has updated libshout to 2.2 

libshout-2.2 appears in FC4 branch only, and is still missed in FC5/development.
Maybe some issues under FC5 environment?

Don't forget to close or reopen bug #181523 , currently it has status NEEDINFO
-- i.e., the next step is yours.

> I now have a Fedora Account and am in cla_done
But it does not mean that you are sponsored ;)


Comment 13 Brandon Holbrook 2006-03-14 11:24:27 EST
(In reply to comment #12)
> (for comment #10):
> > ...has updated libshout to 2.2 
> 
> libshout-2.2 appears in FC4 branch only, and is still missed in FC5/development.
> Maybe some issues under FC5 environment?

I just did a CVS checkout of libshout, and devel/libshout.spec was updated on
March 10 by Thomas, and specifies "Version: 2.2".  To me this means that devel
has been updated to version 2.2  Am I wrong?

> 
> Don't forget to close or reopen bug #181523 , currently it has status NEEDINFO
> -- i.e., the next step is yours.

Closed.

> 
> > I now have a Fedora Account and am in cla_done
> But it does not mean that you are sponsored ;)
> 

I never meant to imply that meant I was sponsored, just making it known that I
have taken as many steps as I know to take on my own, and the next step is up to
the community, i.e. review / approval / sponsorship.
Comment 14 Dmitry Butskoy 2006-03-14 12:00:26 EST
> devel/libshout.spec was updated on March 10 by Thomas, and specifies "Version:
2.2". 

But I can't find any libshout-2.2 rpms under
ftp://download.fedora.redhat.com/pub/fedora/linux/extras/development .
Either he forgot to build for devel, or there are some issues under the devel
branch...

IMHO, the devel/libshout-2.2 stuff should appear first, to make sure that there
are no any problems in php-shout dependencies.
Comment 15 Brandon Holbrook 2006-03-14 12:08:11 EST
(In reply to comment #14)
> But I can't find any libshout-2.2 rpms under
> ftp://download.fedora.redhat.com/pub/fedora/linux/extras/development .
> Either he forgot to build for devel, or there are some issues under the devel
> branch...

I agree... 2.2's not in there, strange.  Thomas didn't mention having any
trouble building it, maybe he just forgot ;)  I sent him an email, we'll see
what he says.

> IMHO, the devel/libshout-2.2 stuff should appear first, to make sure that there
> are no any problems in php-shout dependencies.

I agree
Comment 16 Brandon Holbrook 2006-03-14 15:41:09 EST
(In reply to comment #15)
> I agree... 2.2's not in there, strange.  Thomas didn't mention having any
> trouble building it, maybe he just forgot ;)  I sent him an email, we'll see
> what he says.

Thomas wrote me back and said that when he originally tried building libshout on
devel is when it was wasn't building anything due to dependency issues.  He said
he just tried it again and everything worked, just have to wait for the repo to
sync.
Comment 17 Dmitry Butskoy 2006-03-17 12:51:57 EST
> He said he just tried it again and everything worked, just have to wait for
the > repo to sync.

libshout-2.2 is still missed in the development branch.
But there is libshout-1.0.9, updated on 2006-03-15 .

It seems he just update the old version...
Comment 18 Dmitry Butskoy 2006-03-21 10:46:40 EST
Well, libshout-2.2 has appeared in FC5 branch.

And is still missed in the devel branch. I can't understand why, as FC5 is just
a recent copy of the devel...
Comment 19 Brandon Holbrook 2006-03-21 10:58:21 EST
I've talked to Thomas a few times, and pointed out that devel is still using
1.0.9, I'll send him another email
Comment 20 Dmitry Butskoy 2006-03-28 07:56:25 EST
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.

- .spec file must not contain version/release, rename it to "php-shout.spec"
- remove leading "A" in the Summary.
- IMHO, the Summary can sounds better: "PHP module for communicating with
icecast servers" or something similar.
- %{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).
- 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.

Comment 21 Dmitry Butskoy 2006-03-28 07:58:54 EST
Created attachment 126899 [details]
Suggestions and nitpicks for the spec file
Comment 22 Matthias Saou 2006-03-29 06:54:13 EST
Once the changes suggeested by Dimitry are made, I wouldn't mind making the
formal review of the package and sponsoring you.
Comment 23 Brandon Holbrook 2006-03-30 02:41:19 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.
Comment 24 Dmitry Butskoy 2006-03-30 05:01:35 EST
Well, if "make test" IMHO already fails without an icecast server, then don't
use "make test" in the %check section at all. Just revert back to the old
variant (checking that compiled module is loadable). I assume it would be enough
in such a situation.
Comment 25 Matthias Saou 2006-03-30 12:08:45 EST
Further "nitpicks" :-)
- Keep consistent between tabs and spaces. Only the two top lines had tabs.
- For extdir and apiver, you need "failovers" for when PHP isn't available, for
instance when simply building a source rpm to send to a build system, or when
using mach which expands these outside of the buildroot first, then inside later.
- Having "Minor %define fixes" in the %changelog is wrong. Put %%define!
- You can save half the install lines by using "install -D" to create the dirs.
- I'd put the phpize calls in the %build section, and possibly remove the
--clean one (arguable, I guess)
- I find it easier to put files in %doc in alphabetical order, it's easier to
chack if they're all properly included.

Since it seems you are the upstream author :
- php_shout.c has some "implicit declaration of function 'php_info_print_table_*"
- /php_shout.c has some "'return' with no value, in function returning non-void"
...and a few assorted warnings you might like to look into (it always gives a
bad impression to see so many).

Let me know what you think of all these points. I'll attach a spec file patch.
I'll still have to do some testing of the actual module later on :-)
Comment 26 Matthias Saou 2006-03-30 12:09:54 EST
Created attachment 127060 [details]
Spec file patch
Comment 27 Dmitry Butskoy 2006-03-30 12:24:04 EST
> For extdir and apiver, you need "failovers" for when PHP isn't available, for
> instance when simply building a source rpm to send to a build system, or when
> using mach which expands these outside of the buildroot first, then inside later.

Matthias,
It seems to be a corner case in the Fedora Extras context. Both local and FE
builds (mock?) works properly with such a macro.
On the other hand, the failovers make these macros useless, as you should
specify true values explicitly anyway. (Specifying "non-true" values cause build
to fail, am I right?)

> remove the --clean one (arguable, I guess)
Let it be more robust... ;)
Comment 28 Brandon Holbrook 2006-03-31 01:57:48 EST
I'm working on the .spec file but had a few questions / comments in the mean time.

(In reply to comment #25)
> Further "nitpicks" :-)
> - Keep consistent between tabs and spaces. Only the two top lines had tabs.
I'm usually pretty anal about my spaces vs. tabs, I must have just
highlight+middle-click'ed in vi and forgot that it inserted spaces.  Fixed!

> - For extdir and apiver, you need "failovers" for when PHP isn't available, for
> instance when simply building a source rpm to send to a build system, or when
> using mach which expands these outside of the buildroot first, then inside later.
I used to have failovers, then Dmitry convinced me to get rid of them :), I
agree that FE is a corner case, and either way the build will fail anyway if a
valid php isn't installed.

> - Having "Minor %define fixes" in the %changelog is wrong. Put %%define!
Woops, I can honestly claim ignorance on this one... fixed!

> - You can save half the install lines by using "install -D" to create the dirs.
After switching my mkdir+install to a single install -D, the first one succeeds,
but the second install bombs out with:
+ install -D -p -m 0644 shout.ini /var/tmp/php-shout-0.3a-2-root-brandon/etc/php.d/
install: target `/var/tmp/php-shout-0.3a-2-root-brandon/etc/php.d/' is not a
directory: No such file or directory

But the directory is fully writable by me, any ideas?  It's strange that the
first install -D creates its parent dirs fine, but the second one doesn't.  Does
the 664 have any affect on parent directories that may be created?

> - I'd put the phpize calls in the %build section, and possibly remove the
> --clean one (arguable, I guess)
Like Dmitry, I'd like to leave the --clean in there, because it doesn't hurt
anything, and makes sure you've covered all your bases.

> - I find it easier to put files in %doc in alphabetical order, it's easier to
> chack if they're all properly included.
done

> 
> Since it seems you are the upstream author :
> - php_shout.c has some "implicit declaration of function 'php_info_print_table_*"
> - /php_shout.c has some "'return' with no value, in function returning non-void"
> ...and a few assorted warnings you might like to look into (it always gives a
> bad impression to see so many).
I agree all the warning give a bad impression.  Those warning just now all
showed up with FC5 ( or maybe FC5 rpmbuild is the first to use -Wall so this is
the first I've seen them ? ).  I noticed them last week and started looking into
it.  Most are coming from standard PHP functions like zend_hash_find(), and it
seems the others are in fact my mis-use of some PHP macros (the "return with no
value", for instance).

Dmitry packages PHP modules, do you know of any PHP API changes with 5.1 ( other
than call-time pass-by-ref ;) that have started causing new warnings?
Comment 29 Matthias Saou 2006-03-31 03:58:59 EST
For the "install -D" : The second argument must be the destination file, not a
destination directory. This is why for few files like here it's worth it, but if
you had to "install foo1 foo2 foo3 destdir/", probably not.

As for the failovers, try "rpmbuild -bs --nodeps file.spec" on a machine where
php isn't installed. It'll fail miserably instead of creating a nice .src.rpm
which you can then send to a build system for instance. And as I wrote, mach
will fail too, which is a problem since it's far superior to mock when it comes
to testing builds (yes, I use mach, not mock), so those harmless failover values
_are_ really useful.
Comment 30 Dmitry Butskoy 2006-03-31 06:56:22 EST
>  try "rpmbuild -bs --nodeps file.spec" on a machine where
> php isn't installed. It'll fail miserably instead of creating a nice .src.rpm
Nope!
It produces good srpm for me! (with some stderr output, but it does not
influence the result).

Maybe the reason is both %{extdir} and %{apiver} are needed for binary rpms
only, and have no much sense for the source rpm.

Maybe you think about the %{version}, but "Version: " tag is not auto-detected here.

Matthias,
Could you please write some more info about how to reproduce your failure case?
Comment 31 Dmitry Butskoy 2006-03-31 09:55:22 EST
Due to some strange behavior of ppc build system, we should change dir before
apiver auto-dtection (for example, to %{_tmppath}):

%define apiver  %(cd %{_tmppath}; echo PHP_API_VERSION | %{__cc} -E
--include=php.h `php-config --includes` - | tail -1)

else buildsys on PPC arch reports: "./php.h: Permissin denied"

(i386 and i86_64 arches are not affected).

Surely it seems to be an ugly hack, but IMHO it is some reliable workaround.
Comment 32 Brandon Holbrook 2006-04-10 00:10:22 EDT
http://prdownloads.sourceforge.net/phpshout/php-shout.spec?download
http://prdownloads.sourceforge.net/phpshout/php-shout-0.3a-5.src.rpm?download

Sorry for the delay, my research took me on a trip to San Antonio so I was
unable to work on this for a while.  Here are my newest files, the 'install -D'
issues have been completely cleared up.

I have not added a php fallback version dependency, but as sponsor Matthias is
in charge, so tell me if you see this as a showstopper.

Also of note, the 'php-api' virtual dependency suggested by Dmitry works great
for FC5 and Devel, but FC3 and FC4 don't have that virtual, so installation of
the RPM fails with a missing depency.  I have included the php-api dependency in
the currently submitted .spec file, but the .spec will have to revert to PHP
versioning in the FE[34] branches of CVS.
Comment 33 Brandon Holbrook 2006-04-19 02:04:26 EDT
Matthias, can you take another look at this package and let me know what you think?
Comment 34 Brandon Holbrook 2006-06-21 00:19:23 EDT
Haven't heard or seen anything on this in a long while... anybody out there?
Comment 35 Jason Tibbitts 2006-06-21 10:39:24 EDT
Part of the problem is that the PHP packaging guidelines are still not
finalized, so many reviewers are avoiding PHP packages for the time being.

Also, I notice that this is still blocking FE-NEEDSPONSOR.  There's sort of a
conditional sponsorship offer in comment 22, but I haven't seen Matthias around
lately and in any case he .  I'd suggest checking
http://fedoraproject.org/wiki/Extras/HowToGetSponsored in case Matthias doesn't
comment soon.
Comment 36 Dmitry Butskoy 2006-06-22 07:07:49 EDT
> Part of the problem is that the PHP packaging guidelines are still not 
> finalized,
IMHO that "PHP packaging guidelines" mean the packaging of some PHP code (+
pear, pecl etc.), not the binary plugins for the main php engine.
Comment 37 Christopher Stone 2006-06-28 22:43:09 EDT
Agree to comment #36.  There are many such plugins already available that can be
used as reference.  I took a cursory glance at the spec file and noticed:

BuildRequires:  pkgconfig

This should not be there, this is a bug in the libshout package.  The libshout
package should have:

%package devel
...
Requires: pkgconfig

As you can see here:
http://cvs.fedora.redhat.com/viewcvs/devel/libshout/libshout.spec?root=extras&rev=1.12&view=markup
it currently does not.
Comment 38 Christopher Stone 2006-06-28 22:51:37 EDT
Actually pkgconfig is most likely picked up by one of the Requires in libshout,
so unless none of those devel packages require pkgconfig, then this should not
even be necessary.

Does this build properly without that BuildRequires?  I will do a formal review
on this package later tonight or tomorrow if no one else does.
Comment 39 Brandon Holbrook 2006-06-28 23:25:30 EDT
Yes it appears to build fine without the BuildRequires: pkgconfig, it has been
removed from my .spec.  Honestly, this was my first RPM that actually had to
compile something.  Everything up until this was just 'copy text files to here',
so I guess I was being over-cautious on my buildrequires.  I knew I used
pkgconfig in my %build section, so it seemed reasonable at the time.  You make a
good (and obvious, now) point in that libshout or somewhere up the chain has to
require pkgconfig or libshout wouldn't be in the pkgconfig database at all.
Comment 40 Jason Tibbitts 2006-06-28 23:46:27 EDT
Brandon, I'll go ahead and sponsor you.  Honestly I thought you already were
sponsored as I remember you reviewing and (I thought) approving packages back in
February.
Comment 41 Brandon Holbrook 2006-06-29 18:48:40 EDT
I did do a handful of reviews back then, and there was some confusion on
terminology and whether or not a newcomer like me was actually allowed to
approve and close a bug or just offer comments... then through a long series of
waiting on libshout>=2 and several iterations of my RPM, I never have gotten
approved / sponsored.

Though this is my first package I submitted, I also have horde and
php-pear-Mail-Mine up for review, so I think theoretically you could 'sponsor'
me with either of those packages if you still feel php-shout isn't up to par,
and I can then import the other packages when they get approved.
Comment 42 Dmitry Butskoy 2006-06-30 07:27:53 EDT
(for my comment #31 above):

The final auto-detection macro should be:

%define extdir  %(php-config --extension-dir 2>/dev/null || echo be_happy_mock)
%define apiver  %(( phpize --version 2>/dev/null || echo 'PHP Api Version:
be_happy_mock' ) | sed -n '/PHP Api Version/ s/.*:  *//p')

Note, that "echo be_happy_mock" workaround is needed, because the FE build
system uses mock now.

There are two .spec file reads, first by mock (before rpmbuild stage), and
second by rpmbuild (as normal). At the second stage all is OK (as php-devel
package, which owns the "php-config" and "phpize", is already present in the
build environment), but at the first stage there still is no
"php-config/phpize", and mock fails when extdir/apiver are left undefined.
(Surely at the actual package build stage extdir/apiver will be defined properly).
Comment 43 Brandon Holbrook 2006-06-30 10:36:15 EDT
Spec URL: http://theholbrooks.org/RPMS/php-shout.spec
SRPM URL: http://theholbrooks.org/RPMS/php-shout-0.3.1-3.src.rpm

The latest and greatest, including the removed pkgconfig and the macro fixes
from Dmitry.  Still get a list of warnings from gcc when compiling... will get
to those eventually :)
Comment 44 Christopher Stone 2006-07-03 00:47:38 EDT
I would use these macros instead:

%define extdir %(php-config --extension-dir || echo %{_libdir}/php/modules)
%define apiver %((phpize --version 2>/dev/null || echo 'PHP Api Version:
20041225' ) | sed -n '/PHP Api Version/ s/.*:  *//p')
Comment 45 Dmitry Butskoy 2006-07-03 05:59:00 EDT
"be_happy_mock" variant gives you a chance to not overlook that this is just the
"mock" issue.
Note, rpmbuild always detects these macros correctly. The "correct" fallback
variant (%{_libdir}/php/modules etc.) is not needed here anyway. (Moreover such
a fallback looks a little ugly, because to be clean you are caused to change it
each time the extdir/apiver was changed, which makes all the auto-detection
superfluous).
Comment 46 Brandon Holbrook 2006-07-03 13:31:28 EDT
No offense guys, but debate about these macros have been going on for over 3
months now, and I've made several new rpm releases just to appease one reviewer
or the next, and I'm tired of going back and forth.  Christopher's latest
recommendation look like exactly what I started with, and I'm not real keen on
spending all this time just to end up back where I started.

SO, since Jason is the one actually assigned to this review and will be the one
to sponsor me, I leave it to him to decide what the final macros should be.
Comment 47 Jason Tibbitts 2006-07-03 13:43:29 EDT
I have to agree; we should pick one thing and stick with it.  However,
unfortunately the PHP guidelines did not pass at the recent packaging committee
meeting, so I cannot approve this package at this time no matter what happens to
the guidelines.

I will make sure that the ratified guidelines include the proper macros for
extracting the PHP API version and the review this package against those.

Honestly, my preference will be to avoid BS like "be_happy_mock" or hardcoding a
specific default API version and just using "0".  The only requirement is that
the result after the macro expansions be a specfile with the proper syntax so
that the initial BuildRequires can be extracted.  It shouldn't even be necessary
to play with the extdir define.
Comment 48 Brandon Holbrook 2006-07-03 13:51:24 EDT
I thought (also in comment 36 and comment 37) that the php moratorium only
applied to packages containing php code or pear/pecl packages, not packages
containing binary extensions to the php engine, like this one.
Comment 49 Jason Tibbitts 2006-07-03 14:15:06 EDT
Sorry, that's incorrect.  Applications which are simply written in PHP are OK. 
Extensions to PHP, either PEAR, PECL, or binary are not to be approved until we
have guidelines for packaging them.

Feel free to join the fedora-packaging list and help us to complete those
guidelines.
Comment 50 Jason Tibbitts 2006-08-10 17:53:58 EDT
I know this has sat around for too long, but the end is in sight.  The packaging
committee accepted the PHP guideline draft, although honestly it didn't say much
about extensions like this one.

In any case, I'm ready to move forward with this review.

The first problem is that it doesn't build; I had to add a BR: on pkgconfig. 
Previously you indicated that this wasn't required, but it certainly is a
problem.  Perhaps the new minimal buildroots have changed things since you were
able to build without the BR.

rpmlint finds this in SRPM:
W: php-shout macro-in-%changelog buildroot
  You just need to double a percent sign somewhere.

and in the built RPM:
E: php-shout script-without-shellbang /usr/share/doc/php-shout-0.3.1/TODO
E: php-shout script-without-shellbang /usr/share/doc/php-shout-0.3.1/README
  These are executable for some reason, and shouldn't be.

E: php-shout-debuginfo script-without-shellbang
/usr/src/debug/phpShout-0.3.1/php_shout.h
E: php-shout-debuginfo script-without-shellbang
/usr/src/debug/phpShout-0.3.1/php_shout.c
  These source files are executable, and should be chmodded in %prep.

The final set of macros which were chosen:
%global php_apiver  %((echo 0; php -i 2>/dev/null | sed -n 's/^PHP API => //p')
| tail -1)
%global php_extdir  %(php-config --extension-dir 2>/dev/null || echo "undefined")

Care to fix things up to use those?

Finally, I can't fetch the upstream source from the given URL. The only thing
that seems to be available is 0.3a.
Comment 51 Brandon Holbrook 2006-08-11 00:14:34 EDT
I'm taking a quick trip this weekend (Six Flags wooo!), and will get to these
after the weekend!
Comment 52 Brandon Holbrook 2006-08-11 13:06:31 EDT
Spec URL: http://theholbrooks.org/RPMS/php-shout.spec
SRPM URL: http://theholbrooks.org/RPMS/php-shout-0.3.1-5.src.rpm

Okay I did find some time to update the package with these fixes.  The reason I
removed pkgconfig is that it was decided that libshout-devel would already
require pkgconfig (and I still think it should...), but you're right... I tried
it in mock and it fails without pkgconfig.

You should also be able to find the 0.3.1 sources on SourceForge now.
Comment 53 Jason Tibbitts 2006-08-11 20:23:37 EDT
You might even trace the pkgconfig problem further back; libshout-devel depends
on  libogg-devel, which also has a .pc file but no pkgconfig dependency.  It
seems there's bustage all around.  I filed a bug against libogg-devel so there's
a chance of this getting fixed in the future, but of course you have to target
existing releases and so keeping the pkgconfig dependency here is the right
thing to do.

In any case, things look much better now; rpmlint is quiet and I can grab the
upstream source.

The only thing I notice, which I'm not too clear on, is your requirement of a
minimum php-api version.  Shouldn't this be a requirement of a specific php-api
version (i.e. "=" instead of ">=")?  I'll wager that you know more about PHP
APIs than I do, so I'll leave it to you to decide what's best here.

Well, there is one tiny thing.  Your most recent changelog line is dated Jun 30
instead of Aug 11.  You can fix it when you check in.

Review:
* source files match upstream:
   3a630c1953e0bd0c42a3324f5e449077  phpShout-0.3.1.tar.gz
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   config(php-shout) = 0.3.1-5.fc6
   shout.so()(64bit)
   php-shout = 0.3.1-5.fc6
  =
   config(php-shout) = 0.3.1-5.fc6
   libogg.so.0()(64bit)
   libshout.so.3()(64bit)
   libspeex.so.1()(64bit)
   libtheora.so.0()(64bit)
   libvorbis.so.0()(64bit)
   php-api >= 20041225
* %check is present and the included test seems to pass.
* shared libraries are present, internal to PHP.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.

APPROVED.

Go ahead and apply for cvsextras membership (and fedorabugs if you want it). 
I'll set you up and then you can check in and request your builds.  Let me know
if you need any help.
Comment 54 Jason Tibbitts 2006-08-16 18:50:47 EDT
Somehow things got a bit screwed up.

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