Re: Looking for a sponsor for a new game 'curseofwar'
On Thu, Jul 25, 2013 at 06:40:14PM +0400, Anton Balashov wrote:
> Hello.
Hi,
> Can someone review my package until Bas is busy?
> https://mentors.debian.net/package/curseofwar
> I'm not sure that I done everything right.
Sorry, I thought you were still fixing other things. I have some more remarks
before I can upload it. Note that many of them are just preferences; if you
just say "I like it better this way", that's perfectly fine. I just want you
to be aware of the options.
in debian/control, you write debhelper (>= 9.0.0). This is not wrong, but
personally I would just write (>= 9), which is the same for practical purposes.
You depend on dpkg-dev (>= 1.16.1~), I presume for the hardening flags? You
don't need to (and shouldn't) do that; debhelper 9 takes care of the hardening,
so getting the right dependencies is its responibility, not yours.
You have two commented-out lines which point to a git repository accesible
through git and through a browser. Those lines are meant for providing links
to the packaging of the program. If you use them, they should not be commented
out. However, the links you have there don't work. If you don't use them,
they should be removed completely. Using them is certainly better, if there is
a public place to find the debian packaging.
In debian/copyright, you write "GPL-3.0+". This is not incorrect, but most
people write it as GPL-3+. This is identical, because in the format
specification is written:
> versions with trailing dot-zeroes are considered to be equivalent to versions
> without (e.g., "2.0.0" is considered equal to "2.0" and "2")
The manpage (which is very nice!) says at the end of the description "See HOW
TO PLAY for more details." This seems to point to another file (several
manpages do that, which I find really annoying), but actually points to another
section in the manpage. That is normal for a manpage: first a short
description, then the commandline arguments, then more details. I would remove
this line from the end of the short description, to avoid confusion.
README seems to be the manual page plus installation instructions. It's useful
in the source (because it's in plain text, and contains installation
instructions), but I don't think it adds any value to the binary package.
In debian/rules, you have a lot of hardening commands which are no longer
required (or useful) with debhelper 9. Since you're depending on that, you can
(and should) remove them.
Why are you installing the icons mode 664? That suggests that you would want
to make them owned by a special group, which you don't. And if you would, that
would mean that that group would be allowed to change them, which doesn't seem
useful. I think you want 644, like all other icons.
And if you do, it seems better to install them directly from
curseofwar.install. You are trying to do that as well (but renamed the file so
it isn't used), but there's a problem there: you're installing the directory
"pixmaps" into /usr/share/pixmaps, leading to a directory
/usr/share/pixmaps/pixmaps. Instead, you should either install pixmaps to
/usr/share, or install pixmaps/* to /usr/share/pixmaps/.
The first line of the install file should be removed; you're installing
directly into debian/curseofwar, so dh_install doesn't need to do that (and in
fact it gives an error because it can't find the file).
Thanks,
Bas
Reply to: