Allegro 5 review (Was: can somebody package libalfont ?)
Am 04.07.2012 00:59, schrieb Paul Wise:
> On Mon, Jul 2, 2012 at 2:38 PM, Paul Wise wrote:
>
>> Since I'm (semi-inactive) upstream for alex4 I'll try to spend some
>> time reviewing the Allegro 5 during DebConf12.
>
> Here is the review:
Thanks!
>
> Please forward the patches upstream and add the URLs to the patches as
> per DEP-3.
>
Already done [2] and a good part is included in 5.0.7. The rest are
corrected spelling errors, which I forwarded again. [3]
> allegro 5.0.7 is available upstream.
>
> I think you want the --builddirectory=build argument to dh, that would
> avoid a lot of the override_* targets.
>
> Why do you not use dh --parallel?
>
> Should override_dh_auto_configure be something like dh_auto_configure
> -- $cmake_options, so that the default arguments that
> dh_auto_configure usually passes to cmake are preserved?
>
> Why do you add the -k parameter to dh_installchangelogs?
I addressed all these things now. [4]
>
> You might want to run the tests under xvfb.
When I experimented with that for some other package it was pretty
error-prone and none of the instructions I found worked properly. Also,
the patch for testing without display was included in Allegro 5.0.7.
>
> README.txt doesn't look useful to the users of the binary package.
Since it's only in the doc package I think it can't hurt to ship that.
>
> Are any of the examples/demos playable games? If so maybe we should ship them.
>
I don't think they stand out above other small games and I'd rather not
drop more minigames into the archive just because they exist.
> Please talk to upstream about fixing the examples permissions there.
Done. [3]
>
> Please talk to upstream about making the internal symbols (_al_*)
> hidden by default.
>
Already done. [2]
> Why do you split only some of the sub-libraries out into separate packages?
I split everything out that pulls in additional dependencies.
>
> I wonder if /usr/include/allegro5/platform/alplatf.h is specific to
> each architecture and should be installed in the relevant multi-arch
> path to allow for cross-compiling?
You are right, the file is for example endianess specific. Done.
>
> src/misc/bstrlib.c is under both the BSD and GPL licenses.
Is now stated in debian/copyright.
>
> Hmmmm, src/x/icon.xpm, misc/icon.*, examples/data/icon.tga look
> suspiciosly like the alex4 icon. Doesn't look like they are used by
> any part of the code. This image is definitely not the right icon for
> allegro on the iPhone either.
That's also the Allegro icon. From [1]:
Johan Peitz:
Fixed and enhanced the Win32 joystick driver, and contributed the 'Alex
the Allegator' icon.
>
> lots of warnings from dpkg-shlibdeps.
Building with -Wl,--as-needed flag now.
>
> lintian warnings:
>
> W: liballegro-image5.0: hardening-no-fortify-functions
> usr/lib/x86_64-linux-gnu/liballegro_image.so.5.0.6
> W: liballegro5.0: hardening-no-fortify-functions
> usr/lib/x86_64-linux-gnu/liballegro.so.5.0.6
> W: liballegro5.0: hardening-no-fortify-functions
> usr/lib/x86_64-linux-gnu/liballegro_color.so.5.0.6
> W: liballegro-ttf5.0: hardening-no-fortify-functions
> usr/lib/x86_64-linux-gnu/liballegro_ttf.so.5.0.6
> W: liballegro-acodec5.0: hardening-no-fortify-functions
> usr/lib/x86_64-linux-gnu/liballegro_acodec.so.5.0.6
That are false positives as in #673112. I added lintian overrides.
> addons/acodec/ogg.c and addons/acodec/flac.c has a copyright holder,
> but no license, I guess it is zlib?
>
> Many of the source files reference readme.txt instead of LICENSE.txt.
>
> You might want to point upstream at this blog post:
>
>
http://tieguy.org/blog/2012/03/17/on-the-importance-of-per-file-license-information/
>
> Embedded copies of (parts) of other projects:
>
> docs/tools/trex.*: http://tiny-rex.sourceforge.net/
> docs/src/autosuggest.js:
> http://www.codeproject.com/KB/scripting/AutoSuggestControl.aspx
> bstrlib.*: http://bstring.sourceforge.net/
> examples/data/DejaVuSans*: http://dejavu-fonts.org/
>
> There are some generated files that should probably be removed from
> the upstream tarball and created at build time instead. At the very
> least the build system should automatically recreate them if they are
> missing and the Debian package should remove them before starting the
> build.
>
> src/scanline_drawers.c src/convert.c
> include/allegro5/opengl/GLext/*gl*_ext_alias.h
> include/allegro5/internal/aintern_convert.h docs/man/ docs/html/
> docs/src/refman/images/*.png
>
> Some of the stuff looks like it might be missing source. In particular:
>
> addons/primitives/directx_shaders.c: contains precompiled shaders
>
> allegro.pcx: some sort of 3D scene, I guess it is not meant to be
> modified, but if it is then it is missing a rendering script, a font
> and the models/textures for the rest of the scene.
>
> fakeamp.tga *font.tga green.tga: have some pre-rendered text in them,
> some of which look like they are vector fonts, might be missing the
> font(s) and a script to render them.
>
> mysha.tga and mysha256x256.png are modified versions of mysha.pcx, I
> guess these can't be built automatically though.
>
> examples/data/DejaVuSans.ttf: the source for the DejaVu font(s) are
> some fontforge files.
>
> examples/data/haiku/*.png: look like some of them might have been
> produced in a vector image program.
>
> examples/data/haiku/*.ogg: source for these are .wav files and are in
> the tarball linked from here:
>
> http://www.allegro.cc/forums/thread/602636/844773
>
> demos/a5teroids/data/sfx/game_music.ogg: sounds like it was made from
> something else.
>
> cppcheck warnings:
>
> [examples/ex_acodec_multi.c:53]: (error) Memory leak: sample_data
> [examples/ex_acodec_multi.c:46]: (error) Memory leak: sample
> [examples/ex_blend2.cpp:110]: (error) Array 'rgba_label[2]' accessed
> at index 2, which is out of bounds
> [examples/ex_drawpixels.c:75]: (error) Buffer access out-of-bounds: stars
> [misc/msvchelp.c:43]: (error) Resource leak: fout
> [misc/vcvars.c:138]: (error) Buffer overrun possible for long cmd-line args
> [src/display.c:388]: (error) Possible null pointer dereference:
> display - otherwise it is redundant to check if display is null at
> line 392
>
> pyflakes warnings:
>
> python/generate_python_ctypes.py:2: 'os' imported but unused
> python/generate_python_ctypes.py:3: 'from ctypes import *' used;
> unable to detect undefined names
> python/generate_python_ctypes.py:314: local variable 'release' is
> assigned to but never used
> python/generate_python_ctypes.py:315: local variable 'version' is
> assigned to but never used
> python/checkdocs.py:19: undefined name 'options'
> python/checkdocs.py:34: undefined name 'options'
> python/checkdocs.py:88: undefined name 'options'
> python/checkdocs.py:90: undefined name 'options'
> python/checkdocs.py:167: undefined name 'options'
> python/checkdocs.py:168: undefined name 'options'
> python/checkdocs.py:173: undefined name 'options'
> python/checkdocs.py:185: undefined name 'options'
> python/checkdocs.py:190: local variable 'n' is assigned to but never used
> python/ex_draw_bitmap.py:3: 'glob' imported but unused
> python/ex_draw_bitmap.py:4: 'from random import *' used; unable to
> detect undefined names
> python/ex_draw_bitmap.py:5: 'from math import *' used; unable to
> detect undefined names
> python/ex_draw_bitmap.py:6: 'from ctypes import *' used; unable to
> detect undefined names
> python/ex_draw_bitmap.py:11: 'from allegro import *' used; unable to
> detect undefined names
> misc/make_converters.py:179: unexpected indent
> if (shift > 0):
> ^
> misc/make_scanline_drawers.py:217: undefined name 'texture'
> misc/make_scanline_drawers.py:257: undefined name 'texture'
> misc/make_scanline_drawers.py:261: undefined name 'opaque'
> misc/make_scanline_drawers.py:261: undefined name 'white'
> misc/make_scanline_drawers.py:266: undefined name 'texture'
> misc/make_scanline_drawers.py:274: undefined name 'opaque'
> misc/make_scanline_drawers.py:330: undefined name 'texture'
> misc/make_scanline_drawers.py:348: local variable 'vv_ofs' is assigned
> to but never used
> misc/make_scanline_drawers.py:348: local variable 'uu_ofs' is assigned
> to but never used
> misc/make_scanline_drawers.py:352: undefined name 'texture'
> misc/make_scanline_drawers.py:372: undefined name 'grad'
> misc/make_scanline_drawers.py:376: undefined name 'white'
> misc/make_scanline_drawers.py:401: undefined name 'shade'
> misc/make_scanline_drawers.py:419: undefined name 'texture'
> misc/make_scanline_drawers.py:438: undefined name 'grad'
>
> gcc warnings:
>
> In file included from src/tri_soft.c:23:0:
> include/allegro5/internal/aintern_blend.h:43:6: warning: always_inline
> function might not be inlinable [-Wattributes]
> In file included from src/tri_soft.c:23:0:
> include/allegro5/internal/aintern_blend.h:29:1: warning: always_inline
> function might not be inlinable [-Wattributes]
>
> src/x/xkeyboard.c: In function '_al_xwin_get_keyboard_mapping':
> src/x/xkeyboard.c:606:10: warning: 'XKeycodeToKeysym' is deprecated
> (declared at /usr/include/X11/Xlib.h:1695) [-Wdeprecated-declarations]
>
> src/x/xrandr.c: In function 'xrandr_realign_crtc_origin':
> src/x/xrandr.c:457:10: warning: variable 'ret' set but not used
> [-Wunused-but-set-variable]
>
> src/opengl/ogl_bitmap.c: In function 'ogl_unlock_region':
> src/opengl/ogl_bitmap.c:813:8: warning: variable 'orig_format' set but
> not used [-Wunused-but-set-variable]
>
> addons/primitives/line_soft.c: In function 'line_stepper':
> addons/primitives/line_soft.c:412:19: warning: variable 'minor' set
> but not used [-Wunused-but-set-variable]
> addons/primitives/line_soft.c:430:19: warning: variable 'minor' set
> but not used [-Wunused-but-set-variable]
> addons/primitives/line_soft.c:449:19: warning: variable 'minor' set
> but not used [-Wunused-but-set-variable]
> addons/primitives/line_soft.c:474:19: warning: variable 'minor' set
> but not used [-Wunused-but-set-variable]
>
> addons/image/png.c: In function 'really_load_png':
> addons/image/png.c:100:8: warning: variable 'tRNS_to_alpha' set but
> not used [-Wunused-but-set-variable]
> addons/image/png.c: In function '_al_save_png_f':
> addons/image/png.c:434:8: warning: variable 'depth' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_blend.c: In function 'draw':
> examples/ex_blend.c:104:18: warning: variable 'white' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_blend2.cpp: In member function 'void Prog::blending_test(bool)':
> examples/ex_blend2.cpp:251:18: warning: variable 'opaque_white' set
> but not used [-Wunused-but-set-variable]
>
> examples/ex_blit.c: In function 'example_bitmap':
> examples/ex_blit.c:31:27: warning: variable 'lock' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_clip.c: In function 'example_bitmap':
> examples/ex_clip.c:33:27: warning: variable 'lock' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_display_options.c: In function 'main':
> examples/ex_display_options.c:194:8: warning: variable 'n' set but not
> used [-Wunused-but-set-variable]
>
> examples/ex_drawpixels.c: In function 'main':
> examples/ex_drawpixels.c:30:27: warning: variable 'lr' set but not
> used [-Wunused-but-set-variable]
>
> examples/ex_logo.c: In function 'generate_logo':
> examples/ex_logo.c:77:35: warning: variable 'lock2' set but not used
> [-Wunused-but-set-variable]
> examples/ex_logo.c:77:27: warning: variable 'lock1' set but not used
> [-Wunused-but-set-variable]
> examples/ex_logo.c: In function 'render':
> examples/ex_logo.c:318:30: warning: variable 'lock' set but not used
> [-Wunused-but-set-variable]
> examples/ex_logo.c:368:14: warning: variable 'h' set but not used
> [-Wunused-but-set-variable]
> examples/ex_logo.c:368:11: warning: variable 'w' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_prim.c: In function 'main':
> examples/ex_prim.c:487:18: warning: variable 'white' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_rotate.c: In function 'main':
> examples/ex_rotate.c:23:8: warning: variable 'bmp_h' set but not used
> [-Wunused-but-set-variable]
> examples/ex_rotate.c:22:8: warning: variable 'bmp_w' set but not used
> [-Wunused-but-set-variable]
>
> examples/ex_threads2.c: In function 'thread_func':
> examples/ex_threads2.c:156:11: warning: variable 'w' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
> docs/scripts/trex.c: In function 'trex_element':
> docs/scripts/trex.c:275:7: warning: variable 'op' set but not used
> [-Wunused-but-set-variable]
>
>
Forwarded this upstream [3]. docs/man/ and docs/html/ can already be
removed, as they are generated anyway.
Best regards,
Tobias
[1] http://alleg.sourceforge.net/latestdocs/en/thanks.html
[2] http://sourceforge.net/mailarchive/message.php?msg_id=29116673
[3] http://sourceforge.net/mailarchive/message.php?msg_id=29507615
[4] http://anonscm.debian.org/gitweb/?p=pkg-games/allegro5.git;a=summary
Reply to: