[pkg] backdoor-factory ready for review
Hello Philippe,
On Mon, 19 Jun 2017, Philippe Thierry wrote:
> backdoor-factory is ready for review :)
Comments and questions as I get them:
- the biggest problem is the installation in dist-packages with setup.py
and pybuild, you are polluting the Python namespaces with many generic
top-level packages such as "arm", "intel", "elfbin", "winapi",
"preprocessor", etc. This is not acceptable. There's a reason why we
installed it in a private directory in Kali. ;-)
You should install in dist-packages only when upstream supports it and
when he has namespaced everything under a package that he clearly
"owns" (because it reuses the name of his software for example).
- please switch to Files-Excluded in debian/copyright instead of using
debian/orig-tar.sh as uscan hook script
- in debian/tests/control you have dependencies on "mktemp" which are not
needed, it's provided by coreutils which is essential
- in your tests, you are creating temporary directories but you don't have
to do that, you can just use "$AUTOPKGTEST_TMP" which is a temporary
directory created for you by autopkgtest, if needed you can create
arbitrarily-named sub-directories there obviously.
- in your tests, you use full path "/usr/bin/backdoor-factory" but I
assume that you can rely on PATH being set to a reasonable value (which
includes /usr/bin/)
- as you seem to open arbitrary TCP ports, you probably need to add
"isolation-container" in the restriction.
- you don't need "Testsuite: autopkgtest" in debian/control, dpkg-source
will add it for you in the .dsc
- "XS-Python-Version: >= 2.7" is not very useful (cf previous review)
- use "Priority: optional"
- you don't need to hardcode the dependency on "python", it's added by
${python:Depends} already.
- note that in general, you want to put module dependencies in
Build-Depends as well so that you can run tests when you have some (here
we don't)
- drop debian/dirs it's useless (directories are created on-demand by
dh_install)
- rename debian/docs into debian/backdoor-factory.docs and drop "debian/*"
from that file... README.Debian is automatically installed,
README.source is meant for packagers only and should not be installed
in binary packages.
- let's bump standards-version to 4.0.0 now on all freshly updated
packages
- right now you are installing debian/helper-script/backdoor-factory
in /usr/bin/ (via dh_install) and just afterwards you are overwriting
that file with the content of backdoor.py
=> this will be a non-issue if you switch back to a private directory
instalation as then you will have to rely on Python searching for
modules in the directory of the script being run.
- you don't need to pass "-O--buildsystem=pybuild" yourself when you
override some debhelper targets
- you have those two useless lines in debian/rules (BTW DEB_TARGET_ARCH is
probably not what you think it is, it's only involved when building
cross-compilers IIRC)
# testing target arch.
export DEB_TARGET_ARCH = $(shell dpkg-architecture -qDEB_TARGET_ARCH)
- I would like you to discuss with upstream for the changes in
debian/patches/output_file_target.patch => more control on the
output files is certainly welcome in the general case
At the same time, outputting a message about Debian not allowing
something looks wrong. We might be missing "appack" but the user might
have installed it in some other way. You can fail gracefully if you
don't have it without claiming that Debian doesn't support it.
BTW the correct test is "self.OUTPUT is None" and not "==" IIRC.
And assigning ? self.backdoorfile = "backdoored/" + self.OUTPUT ?
when you have checked that self.OUTPUT is None looks like useless
since you know that self.OUTPUT doesn't contain anything.
=> in the current state, this patch is not good at all
Cheers,
--
Rapha?l Hertzog ? Debian Developer
Support Debian LTS: https://www.freexian.com/services/debian-lts.html
Learn to master Debian: https://debian-handbook.info/get/
Reply to: