Re: leechcraft (closes ITP bug, 33 days have passed)
> While I think this package is interesting, I do not intend to sponsor it.
Thanks a lot for such detailed review!
And for possibility to practice in English. =)
> That said, here is a review of the source package:
>
> I'm not sure it is appropriate to change the ABI names used by
> upstream to a Debian-specific one. If you want to change the library
> names that should be done upstream.
>
> Please talk to upstream about how to make your patches unnecessary.
I have talked a lot with main upstream developer. It was our common conclusion
that I will maintain such patches special for Debian and Ubuntu.
> Please add DEP-3 headers to your patches:
>
> http://dep.debian.net/deps/dep3/
Is it really necessary? I can make it if yes. But these patches are trivial...
And they are under the same copyright as debian/ directory.
> qxmpp, Qxt, eiskaltdcpp, miniupnpc, parseUri, lightbox,
> hunspell/myspell (and anything else in 3rdparty or third-party dirs)
> are either already in Debian or should be packaged separately, you
> should not include them in the binary package or tarball.
You are really mistaken at this point. Maybe you have read my description
inattentive or I have write it not enough clean. So I should explain here...
* qxmpp package already exist in Debian. But it is absolutely not suitable for
leechcraft. Leechcraft authors have made a lot of patches which are not included
in upstream of qxmpp project. Some patches they (upstream) applied but not all.
More over qxmpp is the static library. And nobody uses patched version of qxmpp
except leechcraft project. So it should not be moved into separate package.
* eiskaltdcpp package already exist in Debian. But it is another project with
own binaries. Its authors have made special modification for leechcraft project
(it also includes a lot of patches). And this modification is now just usual
leechcraft plugin. It can't be used separately. So them should not be splitted.
* etc...
All these files were carefully described in suitable sections of
debian/copyright file. And I don't see any reason to break current structure of
the package.
> Some of the icons were created in Inkscape but the source code (SVG
> files) is missing.
I haven't sighted it. What files are you talking about? I'll ask in upstream.
> There are some pre-compressed audio files, I guess the preferred form
> for modification for those is missing and therefore we cannot
> distribute them since they are probably GPL licensed. In addition
> there is DFSG item 2.
What files are you talking about? And what's the problem with
pre-compressed audio files? I am not sure that understood you correctly.
> Your version number and your debian/rules get-orig-source do not
> indicate the reason for the tarball repacking.
But it is described in debian/copyright:
https://github.com/tehnick/leechcraft-debian/blob/master/debian/copyright#L6
Non-free content and unnecessary files were removed from tarball. And yes, I
have read Best Packaging Practices section in the Debian Developer's Reference.
But these unnecessary files are not using anywhere and staying in repository
due to historical or other reasons. I can described reasons for each deleted
file or directory if you want.
So it was our common decision with upstream developers to remove them from
tarball in Debian.
> You can use wildcards in .install files like these:
>
> usr/share/leechcraft/translations/*/LC_MESSAGES/libeiskaltdcpp.mo
> usr/share/leechcraft/translations/leechcraft_poshuku_*.qm
Hmm, I use wildcards. Maybe I have missed it somewhere.
> Some of the files have the incorrect FSF address in them, you might
> want to report that upstream.
I had already sent suitable patches to upstream and they were applied in
upstream git repository. So next stable release will be clean from this point.
But this can't cause to not upload the package in Debian. So we don't need to
wait next release.
> P: leechcraft source: unversioned-copyright-format-uri
> http://dep.debian.net/deps/dep5/
I use last actual version of this document.
> There are some .DS_Store files in the tarball, these are a waste of
> space, you might want to ask upstream to remove them.
Already done some time ago. And files already removed from the git repository.
So next stable release will be clean from this point.
But this also can't cause to not upload the package in Debian. So we don't need
to wait next release...
> cppcheck finds some issues:
>
> qxmpp/src/QXmppTransferManager.cpp:423]: (error) Memory leak: buffer
> [src/plugins/azoth/plugins/metacontacts/managecontactsdialog.cpp:50]:
> (error) Possible null pointer dereference: entry - otherwise it is
> redundant to check if entry is null at line 53
> [src/plugins/bittorrent/tabwidget.cpp:705]: (error) instance of
> "Applier" object destroyed immediately
> [src/plugins/bittorrent/tabwidget.cpp:715]: (error) instance of
> "Applier" object destroyed immediately
> [src/plugins/eiskaltdcpp/dcpp/PerFolderLimit.cpp:92]: (error) Possible
> null pointer dereference: message - otherwise it is redundant to check
> if message is null at line 84
> ...
> I stopped it that this point but there might be more issues.
cppcheck is not an panacea. It makes a lot of false positives.
Also developers from corresponding projects (qxmpp, leechcraft and eiskaltdcpp)
already know about these cppcheck notes.
And this also can't cause to not upload the package in Debian.
But unfortunately nobody interested to sponsor the package yet.
Thanks again.
Best regards,
Boris.
Reply to: