Hi Christian, Gianfranco
First,t hanks for your careful reviews :)
git repo and package on mentors are updated.
Here follows my answers.
Le 14/09/2015 20:37, Christian Kastner wrote :
> Gianfranco already beat me to the review; nevertheless, here are some
> additional notes I had prepared, based on the package I saw on Sunday
> (the package is no longer visible on mentors.d.n).
>
> On 2015-09-14 12:32, Gianfranco Costamagna wrote:
>> lets review:
>>
>> 1) you dropped 0.10.0-2 entry from changelog
Fixed, it disappeared somehow in the process :/
>> 2) entry 0.10.0-1 is targeted wrongly
I don't get what you mean.
>> 3) bump compat level not mentioned in changelog
Done
>> 4) according to setup.py
>> install_requires=['python-musicpd>=0.4.1', 'requests>= 2.0.2'],
>> so I guess there is no need to specify them to runtime dependencies
It is indeed useless to specify them explicitly, dh_python3 and
${python3:Depends}.
>> 5) please mention copyright updates
>> 6) mpd-sima.default
>> "+## NOTA BENE:"
>> this seems to be italian, please use english
Done
>>
>> 7)
>> +## only works with SysV init
>> +## With systemd init: "touch /etc/mpd-sima_not_to_be_run" to prevent mpd-sima from being started
>>
>>
>> well, can't you use some systemd facilities to do the same?
I did specify the systemd way to disable the script in mpd-sima.default.
I also removed support for this file in service file, this was only an
attempt to have systemd to work as SysV did.
I don't believe this is relevant.
> d/control:
> - The Vcs-Browser URL refers to the gitweb viewer, whereas the current
> viewer seems to be cgit (the gitweb URL just redirects there)
Done
> d/changelog:
> - typo: convertion -> conversion
> - When bumping S-V, while not necessary, it is good practice to
> indicate what changes were made in the process, or "no changes
> needed" if that was the case
Done
> - It is helpful to be more explicit about some changes. You mention,
> for example, that the package has been converted to Python 3. The
> fact that the Python 2 package has been dropped is merely implied.
> That is IMHO a significant change, and should be worth a hint
Indeed, though, this is not a python module but only an application.
It is not as important as it might be for python modules.
I did rephrase a bit the changelog anyway.
And I'll try to be more verbose in the future.
> d/copyright:
> - The Upstream-Source URL lists something completely different to the
> the Homepage field in d/control. Could it be that one of them needs
> to be updated?
It does, thanks.
Fields updated.
> d/NEWS:
> - The upgrade path suggestion for the conffile in /etc isn't
> really the prettiest solution, although I really don't know what
> other path I could suggest that wouldn't seem like overkill. How
> did upstream handle this change? Do they perhaps have a script or
> tool that could assist in this process?
Well, I'm actually upstream, I did not write any tool to migrate the
conf. Since the package is not that popular (popcon ~ 100) and because
target users are quite probably advanced users and command line lovers,
I did not try to handle a nice and smoothly conf upgrade.
Well I think the current tradeoff to be acceptable given the package
popularity and users profile.
> d/<somewhere>
> - I encountered a lintian warning when building your package
>
> Apologies for not providing more accurate references and/or possibly
> outdated information. I had your package source in /tmp, which of course
> didn't survive a reboot...
This was a lintian warning command-with-path-in-maintainer-script.
https://lintian.debian.org/full/pkg-multimedia-maintainers@lists.alioth.debian.org.html#mpd-sima_0.10.0-2
I fixed it using which instead of a test:
http://git.kaliko.me/?p=mpd-sima-debian.git;a=commitdiff;h=214b4926
Thanks again for the review.
Geoff
Attachment:
signature.asc
Description: OpenPGP digital signature