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@xxxxxxxxxxxxxxxxxxxxx 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. -- fedora-extras-list mailing list fedora-extras-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/fedora-extras-list
