Bug#823895: RFS: lsm/1.0.4-1

[Lucas Castro]

On 27-07-2016 14:23, Gianfranco Costamagna wrote:
> Hi,
>
>
>> Who missing? I've checked, it's okay.
>
>
> you right
>
>> About that, it's a new version with some patches I've forwarded.
>> I'll wait for this version get in Debian to investigate the new version
>> changes.
>
> I can sponsor, or wait, let me know.
If you can check what Adam Borowski had pointed, It'll be fine.
> G.
I uploaded a new version with spell patch.



signature.asc
Description: OpenPGP digital signature

Bug#823895: RFS: lsm/1.0.4-1

[Gianfranco Costamagna]

Hi,


>Who missing? I've checked, it's okay.



you right

>About that, it's a new version with some patches I've forwarded.
>I'll wait for this version get in Debian to investigate the new version
>changes.


I can sponsor, or wait, let me know.

G.

Bug#823895: RFS: lsm/1.0.4-1

[Lucas Castro]

On 27-07-2016 13:14, Gianfranco Costamagna wrote:
> Lets finish the review:
> 1)
>  
> grep copyright . -Ri
>
> missing people
Who missing? I've checked, it's okay.
> 2) 
> debian/upstream/changelog
>
> what^
> 3) did you forward patches upstream?
forwarded the changelog? His changelog is merged into spec file.
>
> if you can fix/answer the above I think we are good
>
> check-all-the-things review:
> $ codespell --quiet-level=3
> ./config.c:169: unkown  ==> unknown
> ./lsm.c:137: occured  ==> occurred
> ./lsm.spec:825: lisence  ==> license, licence
It is a false-positive, because the changelog is merged into this file,
so it's fix report in changelog.
> ./debian/lsm.init:12: conection  ==> connection
> ./debian/upstream/changelog:695: lisence  ==> license, licence
this file is extract from changelog, so is the same one above.
I'll fix the others.
>
> $ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname .svn -o 
> -iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o -iname _darcs -o 
> -iname .pc -o -iname .cabal-sandbox -o -iname .cdv -o -iname .metadata -o 
> -iname CMakeFiles -o -iname _build -o -iname _sgbak -o -iname autom4te.cache 
> -o -iname blib -o -iname cover_db -o -iname node_modules -o -iname '~.dep' -o 
> -iname '~.dot' -o -iname '~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( 
> -iname '*.bak' -o -iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 
> 'core.*' -o -iname '*~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname 
> '*.jpeg' -o -iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o 
> -iname '*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o -iname 
> '*.css.min' \) -exec env PERL5OPT=-m-lib=. spellintian --picky {} +
>
>
> $ env PERL5OPT=-m-lib=. uscan --report-status --no-verbose
> uscan: Newest version of lsm on remote site is 1.0.5, local version is 1.0.4
> uscan:=> Newer package available from
> http://lsm.foobar.fi/download/lsm-1.0.5.tar.gz
About that, it's a new version with some patches I've forwarded.
I'll wait for this version get in Debian to investigate the new version
changes.
> G.

-- 
Lucas Castro



signature.asc
Description: OpenPGP digital signature

Bug#823895: RFS: lsm/1.0.4-1

[Gianfranco Costamagna]

Lets finish the review:
1)

grep copyright . -Ri

missing people

2) 
debian/upstream/changelog

what^
3) did you forward patches upstream?


if you can fix/answer the above I think we are good

check-all-the-things review:
$ codespell --quiet-level=3
./config.c:169: unkown  ==> unknown
./lsm.c:137: occured  ==> occurred
./lsm.spec:825: lisence  ==> license, licence
./debian/lsm.init:12: conection  ==> connection
./debian/upstream/changelog:695: lisence  ==> license, licence


$ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname .svn -o 
-iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o -iname _darcs -o 
-iname .pc -o -iname .cabal-sandbox -o -iname .cdv -o -iname .metadata -o 
-iname CMakeFiles -o -iname _build -o -iname _sgbak -o -iname autom4te.cache -o 
-iname blib -o -iname cover_db -o -iname node_modules -o -iname '~.dep' -o 
-iname '~.dot' -o -iname '~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( 
-iname '*.bak' -o -iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 
'core.*' -o -iname '*~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' 
-o -iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o -iname 
'*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o -iname '*.css.min' \) 
-exec env PERL5OPT=-m-lib=. spellintian --picky {} +


$ env PERL5OPT=-m-lib=. uscan --report-status --no-verbose
uscan: Newest version of lsm on remote site is 1.0.5, local version is 1.0.4
uscan:=> Newer package available from
http://lsm.foobar.fi/download/lsm-1.0.5.tar.gz

G.

Bug#823895: RFS: lsm/1.0.4-1

[Adam Borowski]

On Thu, Jul 14, 2016 at 04:25:00PM -0300, Lucas Castro wrote:
> I've got a little busy, but I uploaded a reviewed package,
> if you can take a look at it I'll thanks.
> 
> https://mentors.debian.net/debian/pool/main/l/lsm/lsm_1.0.4-1.dsc

Apologies on making you wait, but -ENOTIME these days :(

I've looked at the new version, it seems good to me.

However, I have no experiences with proper systemd-vs-sysv daemon setup nor
conffile handling myself, so I'd need to fill holes in my knowledge first
which is hard under ENOTIME conditions.  Thus, I'd prefer someone else to
finish sponsoring this package.

If no one helps, I'll do it, but not in the next few days.


Meow!
-- 
An imaginary friend squared is a real enemy.

Bug#823895: RFS: lsm/1.0.4-1

[Adam Borowski]

On Sat, May 21, 2016 at 12:09:14PM -0300, Lucas Castro wrote:
> On 21-05-2016 11:59, Adam Borowski wrote:
> > On Fri, May 20, 2016 at 10:31:49PM -0300, Lucas Castro wrote:
> >> But I don't think I need to write a documentation how to setup
> >> the config file is easy to understand, just feeding back it's needed to
> >> setup to get working.
> >> what do you think?
> > I meant mostly what's needed to get the basics running.
> My problem I didn't noticed problem about setup needed because I've
> installed at a machine was already working. 

Right, that's understandable.

If you're going to make complex changes to the packaging, it'd be a good
idea to test it in virtual machines, both fresh and as upgrades.  However,
if you believe you can make it work without, there's no need to do so --
I'll test it for you as I already have an array of VMs, some simple, some
bridged/etc.  And especially, some with systemd some with modular inits,
as this package has .service divergent from its init script.

> > I got the impression you're -trying- to have it work out of the box, in
> > which case no action is needed.  If I'm wrong and configuration is required,
> > then you need to 1. handle lack of such config gracefully, and 2. point the
> > user as to what needs to be done.
> I've done the most changed you pointed, either the feedback about that
> setup is needed to get it running.
> my question is just about user perspective, if I really need to write a
> documentation how to configure or
> just show to user they need to setup. Something like "Edit
> /etc/lsm/lsm.conf is needed to get it running."

Just that line included in the fail message would be enough, I think.


-- 
An imaginary friend squared is a real enemy.

Bug#823895: RFS: lsm/1.0.4-1

[Lucas Castro]

On 21-05-2016 11:59, Adam Borowski wrote:
> On Fri, May 20, 2016 at 10:31:49PM -0300, Lucas Castro wrote:
>> On 14-05-2016 20:45, Adam Borowski wrote:
>>> Only upon checking the syslog I see:
>>> May 15 00:30:37 umbar lsm[12853]: no targets found in config file
>>> yet according to comments in /etc/lsm/lsm.conf:
>>> # Defaults for the connection entries
>>> # These are set in the code. You may override any values here.
>>> which suggests there's no need to edit the config for basic functionality.
>>>
>>> If I read this wrong and some setup is needed, then the package shouldn't
>>> try to start the daemon on initial install, and provide a feedback that
>>> editing the config file is required.
>>>
>>> There's no documentation describing what's needed to get lsm running.
>> I'm almost fishing.
>> But I don't think I need to write a documentation how to setup
>> the config file is easy to understand, just feeding back it's needed to
>> setup to get working.
>> what do you think?
> I meant mostly what's needed to get the basics running.
My problem I didn't noticed problem about setup needed because I've
installed at a machine was already working. 
> I got the impression you're -trying- to have it work out of the box, in
> which case no action is needed.  If I'm wrong and configuration is required,
> then you need to 1. handle lack of such config gracefully, and 2. point the
> user as to what needs to be done.
I've done the most changed you pointed, either the feedback about that
setup is needed to get it running.
my question is just about user perspective, if I really need to write a
documentation how to configure or
just show to user they need to setup. Something like "Edit
/etc/lsm/lsm.conf is needed to get it running."
>
>




signature.asc
Description: OpenPGP digital signature

Bug#823895: RFS: lsm/1.0.4-1

[Adam Borowski]

On Fri, May 20, 2016 at 10:31:49PM -0300, Lucas Castro wrote:
> On 14-05-2016 20:45, Adam Borowski wrote:
> > Only upon checking the syslog I see:
> > May 15 00:30:37 umbar lsm[12853]: no targets found in config file
> > yet according to comments in /etc/lsm/lsm.conf:
> > # Defaults for the connection entries
> > # These are set in the code. You may override any values here.
> > which suggests there's no need to edit the config for basic functionality.
> >
> > If I read this wrong and some setup is needed, then the package shouldn't
> > try to start the daemon on initial install, and provide a feedback that
> > editing the config file is required.
> >
> > There's no documentation describing what's needed to get lsm running.
> I'm almost fishing.
> But I don't think I need to write a documentation how to setup
> the config file is easy to understand, just feeding back it's needed to
> setup to get working.
> what do you think?

I meant mostly what's needed to get the basics running.

I got the impression you're -trying- to have it work out of the box, in
which case no action is needed.  If I'm wrong and configuration is required,
then you need to 1. handle lack of such config gracefully, and 2. point the
user as to what needs to be done.


-- 
An imaginary friend squared is a real enemy.

Bug#823895: RFS: lsm/1.0.4-1

[Lucas Castro]

On 14-05-2016 20:45, Adam Borowski wrote:
> On Sat, May 14, 2016 at 12:22:13AM -0300, Lucas Castro wrote:
>> On 13-05-2016 11:46, Adam Borowski wrote:
 On 10-05-2016 02:43, Lucas Castro wrote
> I am looking for a sponsor for my package "lsm"
>
> dget -x 
> https://mentors.debian.net/debian/pool/main/l/lsm/lsm_1.0.4-1.dsc
>>> 2. The manpage seems mangled:
>>>
>>>While simple to configure, provides easy way reconfigure routes, 
>>> calling notifyscript
>>>
>>>lsmVery configurable, but doesn't support domain names yet.
>> Thanks, fixed.
> Hmm, it looks like you merely added a space and lowercased V:
>
>While simple to configure, provides easy way reconfigure routes, 
> calling notifyscript.
>
>lsm very configurable program, but doesn't support domain names yet.
>
> These two lines don't quite make sense...
>
>
>>> 3. Typo: exectuble.
>> if you mean man page typo, fixed.
> It's still in the init script, line 32.
>
>
> Too bad, when actually trying to install the package:
>
> [] Starting Link Monitor.: lsminvoke-rc.d: initscript lsm, action "start" 
> failed.
> dpkg: error processing package lsm (--install):
>  subprocess installed post-installation script returned error exit status 1
> Processing triggers for man-db (2.7.5-1) ...
> Errors were encountered while processing:
>  lsm
>
> [~]# /etc/init.d/lsm start
> [] Starting Link Monitor.: lsm[~]# echo $?
> 1
> (no newline, by the way -- init scripts shouldn't use "set -e")
>
> [~]# lsm --config /etc/lsm/lsm.conf 
> [~]# echo $?
> 1
>
> An error message describing what went wrong would be nice...
>
> Only upon checking the syslog I see:
> May 15 00:30:37 umbar lsm[12853]: no targets found in config file
> yet according to comments in /etc/lsm/lsm.conf:
> # Defaults for the connection entries
> # These are set in the code. You may override any values here.
> which suggests there's no need to edit the config for basic functionality.
>
> If I read this wrong and some setup is needed, then the package shouldn't
> try to start the daemon on initial install, and provide a feedback that
> editing the config file is required.
>
> There's no documentation describing what's needed to get lsm running.
I'm almost fishing.
But I don't think I need to write a documentation how to setup
the config file is easy to understand, just feeding back it's needed to
setup to get working.
what do you think?
>
> Also, it appears the only copy of upstream's changelog is hidden inside
> lsm.spec (lines between "%changelog" and "#EOF").  Please cut this (with sed
> or a similar tool) and install as /usr/share/doc/*/changelog.gz
>
>
> In /usr/share/doc/lsm/examples/lsm.conf.sample, there are references to
> /usr/libexec/lsm/ instead of /usr/share/lsm/, it'd be nice to sed that to
> what's installed on Debian.
>
>
>>> Meow!
>> Done.
> Hah!  This was intended as an onomatopeia not an imperative, but I really
> like your interpretation :)
>




signature.asc
Description: OpenPGP digital signature

Bug#823895: RFS: lsm/1.0.4-1

[Gianfranco Costamagna]

control: tags -1 moreinfo

Not sure if Adam is going to sponsor the package, but I'm setting at least the 
moreinfo tag :)



g.


Il Domenica 15 Maggio 2016 1:57, Adam Borowski  ha scritto:
On Sun, May 15, 2016 at 01:45:32AM +0200, Adam Borowski wrote:
> Too bad, when actually trying to install the package:
> 
> [] Starting Link Monitor.: lsminvoke-rc.d: initscript lsm, action "start" 
> failed.
> dpkg: error processing package lsm (--install):
>  subprocess installed post-installation script returned error exit status 1
> Processing triggers for man-db (2.7.5-1) ...
> Errors were encountered while processing:
>  lsm

Failing to uninstall is even worse:
Removing lsm (1.0.4-1) ...
[] Stopping Link Monitor.: lsminvoke-rc.d: initscript lsm, action "stop" 
failed.
dpkg: error processing package lsm (--purge):
subprocess installed pre-removal script returned error exit status 1
Errors were encountered while processing:
lsm

Please either use --oknodo in the stop target or otherwise make it handle
the "wasn't running" case as success.  I see you attempt to do so, but "set
-e" breaks that.



Meow!
-- 
A tit a day keeps the vet away.

Bug#823895: RFS: lsm/1.0.4-1

[Adam Borowski]

On Sun, May 15, 2016 at 01:45:32AM +0200, Adam Borowski wrote:
> Too bad, when actually trying to install the package:
> 
> [] Starting Link Monitor.: lsminvoke-rc.d: initscript lsm, action "start" 
> failed.
> dpkg: error processing package lsm (--install):
>  subprocess installed post-installation script returned error exit status 1
> Processing triggers for man-db (2.7.5-1) ...
> Errors were encountered while processing:
>  lsm

Failing to uninstall is even worse:
Removing lsm (1.0.4-1) ...
[] Stopping Link Monitor.: lsminvoke-rc.d: initscript lsm, action "stop" 
failed.
dpkg: error processing package lsm (--purge):
 subprocess installed pre-removal script returned error exit status 1
Errors were encountered while processing:
 lsm

Please either use --oknodo in the stop target or otherwise make it handle
the "wasn't running" case as success.  I see you attempt to do so, but "set
-e" breaks that.


Meow!
-- 
A tit a day keeps the vet away.

Bug#823895: RFS: lsm/1.0.4-1

[Adam Borowski]

On Sat, May 14, 2016 at 12:22:13AM -0300, Lucas Castro wrote:
> On 13-05-2016 11:46, Adam Borowski wrote:
> >> On 10-05-2016 02:43, Lucas Castro wrote
> >>> I am looking for a sponsor for my package "lsm"
> >>>
> >>> dget -x 
> >>> https://mentors.debian.net/debian/pool/main/l/lsm/lsm_1.0.4-1.dsc
> >
> > 2. The manpage seems mangled:
> >
> >While simple to configure, provides easy way reconfigure routes, 
> > calling notifyscript
> >
> >lsmVery configurable, but doesn't support domain names yet.
> Thanks, fixed.

Hmm, it looks like you merely added a space and lowercased V:

   While simple to configure, provides easy way reconfigure routes, calling 
notifyscript.

   lsm very configurable program, but doesn't support domain names yet.

These two lines don't quite make sense...


> > 3. Typo: exectuble.
> if you mean man page typo, fixed.

It's still in the init script, line 32.


Too bad, when actually trying to install the package:

[] Starting Link Monitor.: lsminvoke-rc.d: initscript lsm, action "start" 
failed.
dpkg: error processing package lsm (--install):
 subprocess installed post-installation script returned error exit status 1
Processing triggers for man-db (2.7.5-1) ...
Errors were encountered while processing:
 lsm

[~]# /etc/init.d/lsm start
[] Starting Link Monitor.: lsm[~]# echo $?
1
(no newline, by the way -- init scripts shouldn't use "set -e")

[~]# lsm --config /etc/lsm/lsm.conf 
[~]# echo $?
1

An error message describing what went wrong would be nice...

Only upon checking the syslog I see:
May 15 00:30:37 umbar lsm[12853]: no targets found in config file
yet according to comments in /etc/lsm/lsm.conf:
# Defaults for the connection entries
# These are set in the code. You may override any values here.
which suggests there's no need to edit the config for basic functionality.

If I read this wrong and some setup is needed, then the package shouldn't
try to start the daemon on initial install, and provide a feedback that
editing the config file is required.

There's no documentation describing what's needed to get lsm running.


Also, it appears the only copy of upstream's changelog is hidden inside
lsm.spec (lines between "%changelog" and "#EOF").  Please cut this (with sed
or a similar tool) and install as /usr/share/doc/*/changelog.gz


In /usr/share/doc/lsm/examples/lsm.conf.sample, there are references to
/usr/libexec/lsm/ instead of /usr/share/lsm/, it'd be nice to sed that to
what's installed on Debian.


> > Meow!
> Done.

Hah!  This was intended as an onomatopeia not an imperative, but I really
like your interpretation :)

-- 
A tit a day keeps the vet away.

Bug#823895: RFS: lsm/1.0.4-1

[Lucas Castro]

On 13-05-2016 11:46, Adam Borowski wrote:
>> On 10-05-2016 02:43, Lucas Castro wrote
>>> I am looking for a sponsor for my package "lsm"
>>>
>>> * Package name: lsm
>>>   Upstream Author : Mika Ilmaranta 
>>> * URL : http://lsm.foobar.fi/
>>>
>>> dget -x 
>>> https://mentors.debian.net/debian/pool/main/l/lsm/lsm_1.0.4-1.dsc
> From a superficial review:
>
> 1. Please delete (or fill out) debian/README.source
removed.
> 2. The manpage seems mangled:
>
>While simple to configure, provides easy way reconfigure routes, 
> calling notifyscript
>
>lsmVery configurable, but doesn't support domain names yet.
Thanks, fixed.
>
> 3. Typo: exectuble.
if you mean man page typo, fixed.
>
> Meow!
Done.



signature.asc
Description: OpenPGP digital signature

Bug#823895: RFS: lsm/1.0.4-1

[Adam Borowski]

> On 10-05-2016 02:43, Lucas Castro wrote
> > I am looking for a sponsor for my package "lsm"
> >
> > * Package name: lsm
> >   Upstream Author : Mika Ilmaranta 
> > * URL : http://lsm.foobar.fi/
> >
> > dget -x 
> > https://mentors.debian.net/debian/pool/main/l/lsm/lsm_1.0.4-1.dsc

>From a superficial review:

1. Please delete (or fill out) debian/README.source

2. The manpage seems mangled:

   While simple to configure, provides easy way reconfigure routes, calling 
notifyscript

   lsmVery configurable, but doesn't support domain names yet.

3. Typo: exectuble.


Meow!
-- 
A tit a day keeps the vet away.

Bug#823895: RFS: lsm/1.0.4-1

[Lucas Castro]

Package: sponsorship-requests
Severity: normal [important for RC bugs, wishlist for new packages]

Dear mentors,

I am looking for a sponsor for my package "lsm"

* Package name: lsm
  Version : 1.0.4-1
  Upstream Author : Mika Ilmaranta 
* URL : http://lsm.foobar.fi/
* License : GPL-2
  Section : utils

  It builds those binary packages:

lsm   - Link connectivity monitor tool

  To access further information about this package, please visit the following 
URL:

  https://mentors.debian.net/package/lsm


  Alternatively, one can download the package with dget using this command:

dget -x https://mentors.debian.net/debian/pool/main/l/lsm/lsm_1.0.4-1.dsc




signature.asc
Description: OpenPGP digital signature