Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.maint.python > #13778 > unrolled thread

Review of python-bonsai

Back to article view | Back to linux.debian.maint.python

Contents

  Review of python-bonsai Louis-Philippe Véronneau <pollo@debian.org> - 2021-12-13 23:10 +0100
    Re: Review of python-bonsai "Robin Jarry" <robin@jarry.cc> - 2021-12-14 01:10 +0100

#13778 — Review of python-bonsai

Hi!

Here's my review of python-bonsai:

1. d/control:

* You don't need the "(>= 3.6)" restriction for python3-all-dev, as that
version isn't even in oldstable.

* sphinx-common isn't needed for the source package, as python3-sphinx
depends on it

* "python3 (>= 3.6)" is not required for python3-bonsai,
${python3:Depends} should take care of that for you.

* IMO, python3-bonsai should recommend or suggest python3-bonsai-doc,
but that's up to you.

---------

2. d/copyright:

* you forgot to add a debian/* section. AFAIU, noirello isn't the one
who wrote d/rules :)

* .appveyor/run_with_env.cmd is licensed CC0. You probably don't need
those files, so you can exclude them from the source package using
Files-Excluded in d/copyright

* the MIT license in Debian is named "Expat", for historical reasons.

---------

4. d/rules:

* You left "export DH_VERBOSE = 1" uncommented.

* I'm curious to why you need to set "export LC_ALL = C.UTF-8".

---------

5. d/tests:

I don't have an autopkgtests setup that has machine-level isolation. You
ran that code and it works?

---------

6. d/watch:

You left "<project>" in there instead of replacing it by the actual
project's name (have a look at Lintian) :) Note you can use the "git
tag" mode to simplify this file (not that it's required, your file works
as-is): [1]

---------

7. Upstream code

Have you read the upstream code? It's something you should do (and you
should read all the changes for each new update). Not that you have to
do a proper security audit, but you should go through the code to be
sure there's no obvious or dangerous things in there.

Otherwise, good job! Fix those, ping me and if it's OK, I'll read the
upstream code myself and sponsor it.

Cheers,

[1]:
https://salsa.debian.org/python-team/packages/python-mediafile/-/blob/debian/master/debian/watch


-- 
  ⢀⣴⠾⠻⢶⣦⠀
  ⣾⠁⢠⠒⠀⣿⡁  Louis-Philippe Véronneau
  ⢿⡄⠘⠷⠚⠋   pollo@debian.org / veronneau.org
  ⠈⠳⣄

[toc] | [next] | [standalone]

#13779

Hi Louis-Philippe,

I think I have addressed all your comments. I have pushed fixes in
a temporary branch. I'll integrate them on debian/master once the review
is finished:

https://salsa.debian.org/python-team/packages/python-bonsai/-/commits/review/

See my replies below:

Louis-Philippe Véronneau, Dec 13, 2021 at 23:07:
> * I'm curious to why you need to set "export LC_ALL = C.UTF-8".

This was a leftover from another package. It is useless.

> 5. d/tests:
>
> I don't have an autopkgtests setup that has machine-level isolation. You
> ran that code and it works?

Yes, here is the build log:

http://files.diabeteman.com/juoghahf0teG3Phi/python-bonsai_1.3.0-1_amd64.build.txt

I tried to reproduce the test environment which is prepared for the
github CI actions. This involves generating a docker container with an
LDAP server installed and configured, running that container with
elevated privileges and run the test suite to talk to that container.
It also does some tc voodoo to force timeouts on the TCP connections
(see .ci/delay.py).

I did not manage to run autopkgtests with containers but since the test
needs to run multiple services, a virtual machine made more sense to me.
Also, the use of tc may be a bit too much for a container (I'm not sure
about the limitations, depending on the host).

Some tests are explicitly disabled because I could not get them to work.
I will ping the upstream developer with this when I get a chance.

> 7. Upstream code
>
> Have you read the upstream code? It's something you should do (and you
> should read all the changes for each new update). Not that you have to
> do a proper security audit, but you should go through the code to be
> sure there's no obvious or dangerous things in there.

I have read most of the python code and it looks well written,
documented and tested. C python extensions code is always rather obscure
but I did not see anything suspicious. I am no expert in libldap2
however. The project has been around for some years now. It looks stable
enough to go in Debian in my opinion.

Thanks a lot for your time!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.maint.python

csiph-web