Le dimanche 6 décembre 2015, 20:34:42 Markus Koschany a écrit :
> Hi Alexandre,
>
> here is my initial review. I am working with the pkg-games Git
> repository of corsix-th and I suggest we continue to use it instead of
> the mentors.debian.net packages.
Ok
> debian/control:
>
> You build-depend on wx2.8-headers but this package is obsolete and will
> be removed soon. Please either use wx3.0-headers instead or remove the
> build-dependency because I don't see anything in the sources that may
> need it.
find . -type f -exec grep ^#include {} \; | grep wx | sort -u
#include <wx/app.h>
#include <wx/bitmap.h>
#include <wx/button.h>
#include <wx/checkbox.h>
#include <wx/dcclient.h>
#include <wx/dcmemory.h>
#include <wx/dirdlg.h>
#include <wx/dir.h>
#include <wx/filedlg.h>
#include <wx/file.h>
#include <wx/filename.h>
#include <wx/frame.h>
#include <wx/glcanvas.h>
#include <wx/image.h>
...
It's used in MapEdit/ (currently disabled) and AnimView/
I need to check if this second direcotry is a depedency of MapEdit only or
if it is used somewhere else.
> Why do you depend on lua-filesystem and lua-lpeg explicitly? If those
> dependencies are really required, the ${shlibs:Depends} substvar should
> include them already. Otherwise the build system should be updated to
> require and incorporate those libraries.
Because they are only needed at runtime...
does they really need to get pulled in the build chroot for nothing ?
I guess it's cleaner that way.
> desktop file: I have already fixed a few things that bothered me. I
> believe it's a simulation game and StartupNotify should be set to false.
> I also added keywords and a comment in German. Please tell me if you can
> live with the changes or if there is a reason to stick with the old
> values. Please forward the new desktop file upstream.
Upstream doesn't ship a .png icon either and had removed
the non-maintained DebianPackager...
It's not the kind of thing I really want to bother an upstream with;
ut rather more usefull things, like having appropriate configure
scripts to avoid the need to patch the build system.
> Please use Debian's system library of tinyxml. The embedded copy of
> tinyxml in AnimView/ is neither mentioned in upstream's LICENSE.txt file
> nor in debian/copyright.
As stated for WX, I need to check if AnimView is needed.
> Please ask upstream to remove the embedded copy
That's downstream work.
> or to offer a build system option to use the system library instead.
I'll come with a P.R if the situation is still applicable for current git master.
> They should also mention the copyright holders and license in
> LICENSE.txt. You can take a look at my Bullet package how I used
> Debian's tinyxml library.
Ok
> There are several issues with debian/copyright:
> Some copyright holders and licenses are missing.
>
> BSD-3-clause
> CMake/CMakeFFmpegLibavMacros.cmake
> CMake/FindFFmpeg.cmake
> CMake/FindLibAV.cmake
> CMake/FindSDL2.cmake
> CMake/FindSDL2_mixer.cmake
>
> public-domain
> CMake/FindDirectX.cmake
> CMake/FindLua.cmake
> CMake/FindPkgMacros.cmake
Oops, I just assumed all these *.cmake files were autogenerated ...
> Several Expat copyright holder are missing in
> CorsixTH/Lua/languages
> CorsixTH/Src/jit_opt.h
> SpriteEncoder/*
> SpriteEncoder/parser.cpp
> SpriteEncoder/tokens.h
This is a small external tool not used during the build.
Well, the debian/copyright must cover the whole source package.
> LevelEdit/*
>
> zlib:
> WindowsInstaller/ReplaceInFile.nsh
> Why don't you build the level editor? Wouldn't this be a useful addition
> to the game?
It is disabled by default in the 0.50 release:
From the release notes:
"The Map Editor is not included in 0.50 due to incompatibilities with the engine change.
We hope to have a new improved map editor available in a future release.
In the mean time we suggest that level designers copy their full 0.40 install to another location before installing 0.50."
> Lintian error: source-contains-unsafe-symlink
>
> I think it is safe to override this error and since upstream already
> removed the debian directory from Git master it will be fixed with the
> next release of corsix-th. Another option might be to remove the
> directory for the current version and to append +ds to the upstream version.
I prefer avoiding repacking if I don't absolutely have to (example: in case of non DFSG assets),
here it's about 1 lintian override + document some non needed
files in d/copyright, so I'll keep the original tarball.
> Forwarding the Lua patch upstream is a good idea.
Well, upstream is already aware of the issue & had planned
to fix it in another way; https://github.com/CorsixTH/CorsixTH/pull/899
I've added a "Forwarded: not needed, see https://github.com/CorsixTH/CorsixTH/pull/899";
DEP-3 tag to the patch.
> I would change usr/lib/games/corsix-th/CorsixTH to
> /usr/lib/corsix-th/CorsixTH. We still use /usr/games and
> /usr/share/games for historical reasons but I think it is OK to omit the
> games subdirectory here.
Will have a look
> debian/scripts/corsix-th
>
> Please use the exec command. It replaces the current process without
> forking a new process.
Done
Thank you alreadyAttachment:
signature.asc
Description: This is a digitally signed message part.