On 03/03/2013 10:34 AM, Paul Wise wrote:
> On Sat, Mar 2, 2013 at 5:47 AM, Alex Mestiashvili wrote:
>
>
>> I am looking for a sponsor for my package "authprogs"
>>
> I don't intend to sponsor this but here is a review:
>
Dear Paul, thanks a lot for the review and sorry for such a long delay
for the answer.
Even if this package will not get into Debian I learned a lot from the
review.
I've contacted the author, and he probably will rewrite the code, may be
in other language.
That's why I want to wait with the current package but nevertheless I've
uploaded the updated version.
> The patch combines multiple logical changes into one, please split it up.
>
Done.
> The patch removes upstream copyright statements, license grants and
> changelog bits. I'd suggest reverting those parts.
>
>
It moves it to other place. But with new version I leave it as you
suggested.
> The patch adds three incorrectly spelled words (Standart, fro, debian).
>
>
fixed.
> I think /etc/authprogs would be a better place for the Debian config file.
>
there are 2 "configuration" files
the one which I decided to place in /etc/default defines loglevel and
location of authptogs.conf
and I think that /etc/defaults is a good place for it.
and second which can be in /etc/authprogs.conf or
/etc/authprogs/authprogs.conf
which actually defines what can be executed via ssh .
Placing by default authprogs.conf ( if not defined other location in
/etc/defaults/authprogs) to the users directory gives permission to
setup authprogs even without having root privileges.
The problem with this approach that if a user has no admin rights than
he can't change the loglevel.
> Please get upstream to include the remainder of your patch.
>
> It is not correct to build stuff in override_dh_auto_install, please
> change that to override_dh_auto_build and install the manual page with
> dh_installman.
>
>
I decided to do not provide manual page so far.
> Most of the README.Debian looks to be copies of stuff from elsewhere,
> I would suggest dropping it. Anything remaining can be added to the
> upstream README using a patch.
>
>
dropped it.
> debian/changelog has UNRELEASED in it.
>
>
yes, its done by purpose, because the package will change and it's not
yet ready for the unstable/experimental
or is it a wrong approach ?
> /tmp/authprogs.log is a very bad place for a log file.
>
why /tmp ? if not defined in /etc/default/authprogs than it is
$ENV{HOME}/.ssh/authprogs.log
> Do the examples need to be in both the manual page and the example config file?
>
>
fixed. only in examples directory.
> Please add a get-orig-source debian/rules target so anyone can
> recreate the tarball.
>
>
done.
> Perl::Critic spews a lot of warnings about the upstream code, but I'm
> not sure how many of them are valid.
>
> One of the articles referenced by the package mentions it is version 0.5.
>
Thanks for the hint, changed to the version to 0.5
I have the book, but I didn't know that there is a module for the book!
(Perl::Critic)
I created 2 patches fixing potentially "bad practice" code for severity
5 and 4.
> Automatic checks:
>
> https://wiki.debian.org/HowToPackageForDebian#Check_points_for_any_package
>
> lintian:
>
> I: authprogs source: vcs-field-not-canonical
> http://git.debian.org/?p=collab-maint/authprogs.git;a=summary
> http://anonscm.debian.org/gitweb/?p=collab-maint/authprogs.git;a=summary
> I: authprogs source: vcs-field-not-canonical
> git://git.debian.org/collab-maint/authprogs.git
> git://anonscm.debian.org/collab-maint/authprogs.git
> P: authprogs: no-upstream-changelog
>
>
lintian -Iivcm authprogs_0.5-1_amd64.changes
in a future Lintian release.
N: Using profile debian/main.
N: Setting up lab in /tmp/temp-lintian-lab-YxXbw4EAyO ...
N: ----
N: Processing changes file authprogs (version 0.5-1, arch source all) ...
N: ----
N: Processing source package authprogs (version 0.5-1, arch source) ...
N: ----
N: Processing binary package authprogs (version 0.5-1, arch all) ...
W: authprogs: binary-without-manpage usr/bin/authprogs
...
Best regards,
Alex