On Fri, Dec 29, 2017 at 06:58:02PM -0500, Kienan Stewart wrote:
> I've uploaded an updated package to mentors.debian.net. I think it's
> ready for more review/testing. I'm also looking for a sponsor if
> anyone is willing.
Hi!
I've done a thorough review of the package. I haven't reviewed the
upstream source code or attempted to rebuild the package myself. Here
are the issues I could find by looking in the debian/ directory:
1. the version number is a little strange: "2.1.4-stable-1" I also
believe it violates policy (§5.6.12: version), because there is a
"debian revision" ("-1") so there can't be a dash in the upstream
revision ("2.1.4-stable"). In general, I believe it is not permitted
to have two dashes in version numbers: I'm surprised lintian let
that through, maybe a bug there!
https://www.debian.org/doc/debian-policy/#version
i understand you may have chosen the version number to match the
tarball name from upstream, but we should help upstream to do the
right thing here. you can introduce them to semantic versioning, for
example. you can also "mangle" the url in uscan to remove the
"-stable" string, with something like "versionmangle=s/-stable//".
2. The VCS fields are present (good) but they point to the upstream
repository (?) which is fine if you are the upstream, but in this
case there is a "debian revision", which means this is not a native
package, which I assume means you are not the upstream. The Vcs-*
fields should point to a repository where we can find the *debian*
packaging files, not the upstream repository. That is what the
Homepage field is for. I would recommend you try out the new GitLab
instance setup by the Debian project to host that repository, at
https://salsa.debian.org/
3. not sure the "this is my first package" note is necessary in the
changelog :)
4. debhelper is now at version 11, you might want to take a look at the
features (in the debhelper(7) manpage) and bump the compatibility level
in debian/compat and the dependency in debian/control
5. purely cosmetic, but i like to align the dependencies in
debian/control. instead of:
Build-Depends: debhelper (>= 9), clang, scons, libx11-dev, libxinerama-dev, libxcursor-dev, libxrandr-dev, libfreetype6-dev, libpng-dev, libasound2-dev, libpulse-dev, zlib1g-dev, libgl1-mesa-dev, libstdc++-6-dev, libgcc-6-dev, libssl-dev, libglu1-mesa-dev, libwebp-dev, libtheora-dev, libvorbis-dev, libopus-dev, libopusfile-dev, libglew-dev, ca-certificates
i find the following more readable:
Build-Depends: debhelper (>= 9),
clang,
scons,
libx11-dev,
[...]
this makes diffs easier to read for future changes and allows the
packages to be sorted easily.
6. in the control file again, i would split the description in
paragraphs. for example, this:
Godot is an advanced, feature packed, multi-platform 2D and 3D
game engine.
This package allows games to be run from source or data-pack.
should instead be:
Godot is an advanced, feature packed, multi-platform 2D and 3D
game engine.
.
This package allows games to be run from source or data-pack.
also consider having *exactly* the same common description in all
three packages and just change the last paragraph according to the
package.
7. the manpage symlinks in debian/ are strange. why not just list
"misc/dist/linux/godot.6" in godot2.manpages? i'm not sure how to
setup manpage symlinks however, but i would suggest finding another
package that does it and looking at *how* it does so.
http://sources.debian.net has all the answers once you find the right
package. :)
8. debian/godot.manpages probably does nothing and should be removed
9. now for the patches. the add-openssl-1.1 patch has some template
stuff that should be removed. it should also explain why the patch
is necessary (e.g. why isn't it in the upstream release? will it
ever be?)
10. the manpage patch has similar issues: is it from upstream? why
isn't it in the release?
11. the change-ca-cert-sources-path has the above, plus: isn't it
possible to use the #thirdparty pattern to do such a replacement
instead of using a patch? it looks like this is *designed* to be
configurable from the outside
12. the remove-thirdparty-ca-certificates patch is a big wth for me...
if we change the path, why do we need to remove the certs? what's
that thing with the symlink?
13. the remove-unneeded-thirdparty-libraries patch is just scary. 32MB
is just plain wrong. if you want to remove code, you need to
repackage the tarball. it's nasty and weird, but basically, you
rebuild the original tarball without all that crap, period. you
also try to convince upstream to do the same, for example by using
submodules instead of embedding the code directly. then you explain
that change in README.Debian (which you partly did). removing those
libraries in a patch is *worse* than keeping them there: they are
*still* in the original source and *twice* because they're also in
that patch! if you want to just disable those things from being
built, there should be a better way...
14. debian/rules has some ... interesting things. this is strange, for
example:
install -m 755 bin/godot.x11.opt.tools.64.llvm $(CURDIR)/debian/tmp/usr/bin/godot2
here i would list bin/godot.x11.opt.tools.64.llvm in debian/install
and then rename the files after dh_auto_install. i would also open
an issue upstream to propose they install sane binary names.
15. dh_clean definitly needs work as well. isn't scons -c supposed to
do all that stuff? if not, there's probably something wrong in the
upstream and yet another bug can be filed there (and mentioned in
the rules file). also, use dh_auto_clean there, it may do scons -c
on its own!
I understand this may be more than you bargained for. It's a lot of
stuff! Not all of those are absolutely essential: some are personal
cosmetic things or missing documentation. But others (like the version
number) are essential.
Think about finding this package in 5 years: you completely forgot how
all of that stuff works and why you did those things. Could you figure
it out again based on what's there? Because that's what someone will end
up doing eventually, if only to help you with package maintenance at
some point... :)
I know this is your first package, and it's already really impressive
work. This is a hard package to start with, and you've already done an
amazing job at getting the thing to build at all and making lintian
happy. Now you need to make FTP-masters and other Debian developers
(like me!) happy, which is, arguably, a little harder when you start
(partly because some rules are unwritten or just written down somewhere
impossible to find). Don't get discouraged, you're doing great, and
you'll get through this. :)
Good luck!
A.
PS: I assume you actually run the thing using the package now? If so,
try to do that as well...
Attachment:
signature.asc
Description: PGP signature