[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-31 01:57 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?

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