Re: vavoom, updated version
Whoa! that is a review :)
On Wed, Feb 29, 2012 at 11:44:59AM +0800, Paul Wise wrote:
> Some issues you might want to look at for the next upload:
>
> If you contact upstream as a result of this review, please include
> these two links in your email:
>
> http://www.freedesktop.org/wiki/Games/Upstream
> http://wiki.debian.org/UpstreamGuide
i've contacted, in private, upstream about licensing, but they don't reply my emails.
i will cc d-d-g to see if i get a better response
>
> You might want to run wrap-and-sort -s to make diffs of debian/control
> and other files more readable.
i didn't know that tool, and many others you mention. thanks
>
> Your packaging isn't committed to the games team VCS repos:
>
> http://wiki.debian.org/Games/VCS
>
this is my first public debian package, i didn't want to mess with VCS
at the same time.
i'm playing with VCS for my next pkg uhexen
> Please check if vavoom works with libpng1.5 as indicated by the PTS page:
>
> http://packages.qa.debian.org/v/vavoom.html
>
> Please edit the debtags for vavoom:
>
> http://debtags.debian.net/edit/vavoom
will do both
>
> There are some build log issues:
>
> https://buildd.debian.org/~brlink/packages/v/vavoom.html
>
> The package FTBFS on Hurd due to the use of PATH_MAX:
>
> https://buildd.debian.org/status/package.php?p=vavoom
> https://www.gnu.org/software/hurd/hurd/porting/guidelines.html
i was wrinting an email to d-d-g and d-mentors asking for advice.
i see 3 ways to fix it,
- disable vavoom on hurd
after all, most hurd installs are on VM, no much gain on running a
game there
- define the macro PATH_MAX
easy, but is not really a fix
- replace the macro with a function, as is recomended by hurd folks
i'm not a C expert, so it might take a while and can be buggy
after some thinking, i choose the third option
>
> I don't think it is appropriate to recommend contrib/non-free deps,
> please move heretic-wad, game-data-packager to Suggests:
>
FTP master agrees with it. is not the first doom engine, and game i
guess, with non-free recommends. anyway non-free is not default so if
you are using defaults you won't get any of these stuff
>
> The patch headers are a bit weird, they include snippets from
> debian/changelog and duplicated DEP-3 headers. Please delete them all
> and write some DEP-3 headers from scratch:
>
> http://dep.debian.net/deps/dep3/
>
> Please forward the patches, manual pages and desktop file upstream and
> add DEP-3 headers or comments indicating where they have been
> forwarded.
>
> The upstream docs/vavoom.txt file includes installation instructions,
> which isn't useful for users of binary packages. Please ask upstream
> to split those out into README.install.
>
> You may want to make the copyright file machine-readable:
>
> http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
>
> Some of the upstream code contains the incorrect FSF address. I would
> suggest that upstream should point at the GNU website instead, as
> suggested by GNU:
>
> https://www.gnu.org/licenses/gpl-howto.html
>
> I note that upstream uses system() in one file. In this case the usage
> seems safe since it is only run during the build process, but you
> should ensure that future uses of this function are safe or switch to
> the exec*() family of functions.
>
> GCC warnings, these are repeated many times:
>
> utils/vlaunch/vlaunch.xpm:292:1: warning: deprecated conversion from
> string constant to 'char*' [-Wwrite-strings]
> source/gl_model.cpp:317:70: warning: cast to pointer from integer of
> different size [-Wint-to-pointer-cast]
>
> lintian complaints:
>
> W: vavoom source: out-of-date-standards-version 3.9.2 (current is 3.9.3)
that went out btw the upload and the build, i miss it
> W: vavoom: binary-without-manpage usr/games/vlaunch
>
> cppcheck warnings:
>
> [libs/core/str.h:355]: (error) Uninitialized variable: Len
> [libs/core/str.h:356]: (error) Uninitialized variable: Len
> [libs/core/stream.h:134]: (error) Uninitialized variable: Val
[....]
> [source/r_main.cpp:376]: (error) Throwing exception in destructor
> [source/r_main.cpp:392]: (error) Throwing exception in destructor
>
> Probably more cppcheck issues exist, I killed cppcheck at this point
> because it uses so much CPU.
>
> dpkg-shlibdeps warnings:
>
> The wxWidgets ones are probably the fault of that, not vavoom. The
> other ones should probably be fixed in the upstream build system
>
> dpkg-shlibdeps: warning: dependency on libwx_gtk2u_richtext-2.8.so.0
> could be avoided if "debian/vavoom/usr/games/vlaunch" were not
[....]
> uselessly linked against it (they use none of its symbols).
>
> There are some duplicate files. Not sure if unduplicating them is
> possible or useful to do upstream. The Debian manual pages should not
> be duplicate, instead they should use symlinks or the .SO mechanism.
i will fix the duplicate manpages, and the missing.
i don't like the idea to mess that much with upstream source
thanks again for you deep review, i appreciate it
--
1AE0 322E B8F7 4717 BDEA BF1D 44BB 1BA7 9F6C 6333
Reply to: