Re: prboom review
On Sat, Jul 28, 2007 at 10:12:19PM +1000, Paul Wise wrote:
> A review of the prboom 2.4.7+dfsg-2 package on mentors.debian.net;
Thanks. I couldn't see any other addressed recipients, so I've CCed
debian-devel-games.
> * The part of README.Debian about the source package isn't really
> relevant to end users, best move that to README.Source or
> something
I have no strong opinion on where it goes, but I do think it's a good
idea to document this in the package somewhere. I've moved it to
README.source, which apt-file shows exists in six packages (whereas it
cannot find any README.Source files).
For d-d-g, the section explained how the source package is maintained
and how to go about checking it out.
> * I don't see any .desktop file in the package, might be nice for
> GNOME/KDE users if you added one (and there is a proposal for
> the Debian menu to have .desktop files as the source instead of
> menu files)
I think that it would be a good idea to have a .desktop file eventually,
although I'd like to see the dust settle on the new menu policy and
potential transitions happening. Also providing an icon would be good
but I don't fancy uuencoding one into the diff.gz right now (and it
would have to be derived from the freedoom sprites, or something).
I might persue adding an icon to upstream.
> * debian/rules: the check for nostrip isn't needed, dh_strip
> checks for DEB_BUILD_OPTIONS itself IIRC
Fixed
> * debian/rules: you can use DESTDIR=$(CURDIR)/debian/tmp instead
> of running pwd (and $(shell ...) is preferred to backticks)
Fixed
> * there are quite a few signedness warnings and other scary
> warnings when building the code, does upstream know about these?
Yup. The code is pretty hairy, but they have a light-touch approach as
there are various sensitive timing issues for demo replay etc. that are
easy to break. Most new gcc revisions throw up a hole bunch more
warnings to fix.
> * did you ask upstream to rename the manual page for prboom.cfg?
I personally haven't, no. The conflict will go away soon as lxdoom has
been orphaned and with any luck will be removed from the archive[1].
prboom is a decendent of lxdoom which is why they use the same filename.
> * I notice the -ffast-math option to GCC being used, for me, that
> option caused floating point exceptions on alpha for another
> package. I got some comments at the time[1][2].
This seems to be something upstream have explicitly requested in their
autoconf gubbins. I'll have to try disabling it and comparing the
resulting binary to one with it on (although I doubt I'll spot any
differences). Do you get floating point exceptions on alpha with the
prboom package?
> * lintian warnings; UNRELEASED in changelog,
That's pretty shoddy of me, sorry. It looks like I only ran
lintian/linda over the resulting .deb, which doesn't flag those up for
me.
> ignoring make clean errors
That's something we've inherited in the package. I'll look into removing
that.
Thank you very much for the review!
[1] http://lists.debian.org/debian-qa/2007/08/msg00063.html
--
Jon Dowland
Reply to: