Re[2]: Bug#660519: RFS: manaplus/1.2.2.19
Thanks for review.
manaplus updated to version 1.2.3.4
22 февраля 2012, 05:23 от Paul Wise <pabs@debian.org>:
> Here is another review of manaplus:
>
> If you contact upstream as a result of this review, please point them
> at these two pages:
>
> http://wiki.debian.org/UpstreamGuide
> http://www.freedesktop.org/wiki/Games/Upstream
I am upstream
>
> Did you intend for this to be maintained as part of the Debian games team?
Now i not sure. For me now task add manaplus to debian, after we will see.
>
> Do you intend to package the servers for manaplus too?
For now no, but in future yes.
Now server not in state for packaging, need many work for adapt it to work from separate directories.
Also it can work only in 32 bit version. Server from what it forked storing pointers in int and have
some other 64 bit issues.
>
> All the servers appear to require a login to play, are there any I
> could use for 'testing' the game without registering?
Registration is simple, In game client need press register and enter login and password.
I can create preregistered account but not sure what password i can put in public, because some one can
change it after.
>
> Please consider wrapping debian/menu with one item per line.
>
> description= is not valid in debian/menu, you want longtitle= IIRC.
Fixed.
>
> xz compression is better than bzip2, have you considered switching?
> Personally I don't think the package is large enough to bother
> switching from gzip though.
for debian/* files i switched to gz, for tarball to xz. Look like this modes giving better compressing
>
> Some of the images were created in Inkscape or GIMP but there are no
> source SVG or XCF images, that might be a GPL/DFSG violation.
SVG image was not drawed for manaplus, but used same as in mana. I removed it
For other one image i add xcf file, but sad it in 2.7 gimp format or like this.
I also removed some themes because license GPL and i dont have response from
author about sources.
Other themes relicensed from GPL to CC-BY-SA.
Other themes from data/graphics/gui is a bit modified theme from mana client/package
data/themes/wood is theme from tmw package (branding for mana)
tmw and mana now in debian and i think if it accepted, then this themes here
can be accepted too.
>
> There is one pre-mixed, pre-encoded audio file, could you ask upstream
> about how it was created?
It based in one of ogg files from The Mana World sounds. I simple cut and add delay
to one channel.
I can include ogg file as source if need.
Also in new version i add sounds from psiplus package converted to ogg and some renamed.
Should i also include source wav files?
>
> In the upstream .desktop files, none of the language-specific Icon or
> Name lines appear to be needed.
Fixed
>
> You are installing the upstream PNG icon into the wrong location, it
> should be /usr/share/pixmaps since none of the
> /usr/share/icons/hicolor/*/apps/ dirs are for the same resolution as
> the image. Please also install manaplus.svg into
> /usr/share/icons/hicolor/scalable/apps/ and use python-scour to strip
> down the installed image.
I think fixed only without svg.
>
> The manaplus.svg file looks slightly different to manaplus.png and the
> other files, it looks like that means the source SVG for manaplus.png
> is missing and there is a GPL/DFSG violation.
>
> There is no need to install manaplusttest at all IMO since it does
> nothing useful to users, please get upstream to disable that. If they
> are not willing to do that, please at least remove the desktop file
> for it.
Manaplustest need for autocheck graphics settings/speed and add it to configuration.
Desktop file removed.
>
> Please ask upstream to use fontconfig to find font files on Linux or
> switch to a font rendering system that does that (such as SDL-Pango,
> QuesoGLC). Then you will not need to use those symlinks, which are
> fragile when font paths move around and are not portable between Linux
> distros.
I thinking about SDL-Pango not sure is it good to use it here.
I cant use QuesoGLC because look like it only for opengl. But here exists software and
two opengl backends.
About fontconfig. Is possible with it create link to fonts with given names?
>
> Some of the upstream code has the wrong address for the FSF in the
> license grant, please let them know about that.
Fixed.
>
> Some of the copyright/license information is missing from
> debian/copyright. Please audit every file, licensecheck --copyright
> helps to find missed stuff though.
I think fixed.
>
> You are missing a depends on x11-utils because the code uses xmessage
> to display errors.
Fixed
>
> I'm not sure if any of the errors contain untrusted network/other
> input, but it is not a good idea to use the system() function for
> anything other than constant commands. Please send upstream a patch to
> use the exec family of functions, they are the only ones that are
> guaranteed to be safe from arbitrary code execution in the face of
> untrusted input.
I left system call for now, but strict text what can be given to it.
(only static or always safe strings allowed)
>
> I note that the upstream help system uses plain text files for
> translations, has upstream considered using standard po files for
> that?
I thinking about this. This possible in future versions.
>
> There are some duplicate files (I detect them using fdupes), you might
> want to ask upstream to remove them from the source package and in the
> meantime reduce the size of the package slightly using symlinks:
>
> rdfind -outputname /dev/null -makesymlinks true debian/manaplus/
> symlinks -r -s -c debian/manaplus/
I removed most duplicates. Some left because this files accessed by physfs and
in physfs soft links not allowed. I allow it only for fonts enumeration.
>
> cppcheck warnings (send upstream):
>
> [src/graphicsvertexes.h:140]: (error) Memory leak: ImageVertexes::ogl
> [src/game.cpp:1828]: (error) Possible null pointer dereference: newMap
> - otherwise it is redundant to check if newMap is null at line 1823
This is all false positives.
>
> Lintian complaints:
>
> O: manaplus-data: package-contains-empty-directory
> usr/share/manaplus/data/themes/classic/
Empty dir for classic theme removed.
> I: manaplus: spelling-error-in-binary usr/games/manaplus dont don't
Some "dont" fixed other is by design in internal commands/events.
current dsc file: http://mentors.debian.net/debian/pool/main/m/manaplus/manaplus_1.2.3.4-1.dsc
Sorry for long time response.
Reply to: