[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#858860: RFS: arpwatch [ITA]



Hi Lukas,

> I do not have a web based git viewer installed on my server, however
>   git clone https://git.somlen.de/arpwatch.git
> should work. Remember to switch to the `staged-changes` branch.
> 
> (I should probably create a page explaining that this is a git only url
> instead of displaying a 403?)

Fine, thanks !

> > Quick review:
> > 
> > * lintian reports
> > 
> >   P: arpwatch source: source-contains-data-from-ieee-data-oui-db
> > ethercodes.dat 
> > 
> >   but it looks like you already fixed it. If this warning is not
> > relevant anymore please override it.  
> 
> Well, I did not really fix it. ethercodes.dat is still part of the
> source package, it's just no longer put into the binary package. But, as
> it is small and it does not violate the DFSG, Christian Seiler suggested
> on debian-mentors not to repack [1].
> 
> I have not previously overridden that tag because I was not sure if it
> is best practice to override lintian pediantic tags at all (only quite
> few packages seem to do it) as they also don't show up on the
> tracker.debian.org page. Anyways, I've now overridden the following two
> pedantic tags, together with a justification as comment:
> * source-contains-data-from-ieee-data-oui-db

I usually don't override pedantic tags.

However, I think it may be useful in this case because this tag and the following
changelog entry could seem to be somewhat contradictory without additional
explanations:

 * use ethercodes from ieee-data

One could think you just forgot to remove the file.

Overriding the tag allows you to explain why you did not remove the
file and why the tag isn't relevant anymore.

But anyway, that's not important, other developers would probably do it
differently.

> * debian-watch-may-check-gpg-signature

I wouldn't override debian-watch-may-check-gpg-signature btw.

> > * There's no copyright entry for you
> 
> Fixed.
> 
> > Nitpicking:
> > 
> > in debian/changelog: why "remove dmassagevendor" ? This changelog
> > entry could be more verbose.  
> 
> Right, I have a longer justification in the git history; I have expanded
> on the changelog entry.
> 
> > $ cme check dpkg
> > [...]
> > Warning in 'dirs:0' value 'usr/sbin': Make sure that this directory is actually needed. See L<http://www.debian.org/doc/manuals/maint-guide/dother.en.html#dirs> for details
> > Warning in 'dirs:1' value 'var/lib/arpwatch': Make sure that this
> > directory is actually needed. See
> > L<http://www.debian.org/doc/manuals/maint-guide/dother.en.html#dirs>
> > for details  
> 
> If I remove `usr/sbin` from dirs, buildpackage fails complaining that
> the directory does not exist (so something in the build system is
> slightly broken).

The error message is

/usr/bin/install -c -m 555 -o bin -g bin arpwatch /build/arpwatch-2.1a15/debian/arpwatch/usr/sbin
/usr/bin/install: cannot create regular file '/build/arpwatch-2.1a15/debian/arpwatch/usr/sbin': No such file or directory
Makefile:114: recipe for target 'install' failed 

looks like the Makefile installs files under usr/sbin, but doesn't
create the directory if it doesn't exist. This is rather a Makefile bug.

> `var/lib/arpwatch` is an empty directory created by the package that is
> populated with ethercodes.db during postinst (and then with interface
> specific database files once arpwatch is started). Should I create the
> directory during postinst instead? Creating the empty directory as part
> of the package seems nicer since dpkg will take care of the `rmdir` and
> warn the user if the directory is not empty on uninstall.

OK, fine.

> > [...]
> > Warning in 'control source Vcs-Git' value 'git://anonscm.debian.org/collab-maint/arpwatch.git': An unencrypted
> > transport protocol is used for this URI. It is recommended to use a
> > secure transport such as HTTPS for anonymous read-only access.
> > 
> > Warning in 'control source Vcs-Git' value 'git://anonscm.debian.org/collab-maint/arpwatch.git': URL is not the
> > canonical one for repositories hosted on Alioth.  
> 
> I had that on my TODO list but decided to wait and see how I get the
> package sponsored before changing the Url. I've now pointed it to what I
> expect to become its new home in case you are willing to sponsor the
> package:
>   https://anonscm.debian.org/cgit/pkg-security/arpwatch.git
>   https://anonscm.debian.org/git/pkg-security/arpwatch.git
> I've also adjusted debian/control to what I think it should be for team
> maintenance (maintainer is pkg-security-team, added myself as uploader).

Fine. Yes, I'll sponsor your package.

> > Warning in 'control binary:arpwatch Pre-Depends:0' value 'dpkg (>= 1.16.1)': unnecessary versioned dependency: dpkg (>= 1.16.1).
> > Debian has oldstable -> 1.16.18; stable -> 1.17.27; unstable -> 1.18.23; testing -> 1.18.23;  
> 
> Ok, I removed the pre-dependenciy.
> 
> In order to setup the file based trigger I followed man deb-triggers(5)
> from dpkg-dev version 1.18.23 (most recent version in unstable) which
> states:
> > The ?-noawait? variants are only supported since dpkg 1.16.1, and will
> > lead to errors if used with an older dpkg. It is thus recommended to
> > add a ?Pre-Depends: dpkg (>= 1.16.1)? to any package that wish to use
> > those directives.

If the dpkg documentation recommends to do so, then, fine, forget about
this warning.

> > $ codespell *
> > aclocal.m4:784: seperate  ==> separate
> > aclocal.m4:787: independantly  ==> independently
> > aclocal.m4:788: dependancies  ==> dependencies
> > arp2ethers:8: occurance  ==> occurrence
> > config.sub:1161: nto  ==> not  | disable due to \n
> > debian/changelog:129: wont  ==> won't, wont
> > dns.c:140: cannonical  ==> canonical
> > WARNING: Decoding file ethercodes.dat
> > WARNING: using encoding=utf-8 failed.
> > WARNING: Trying next encoding: iso-8859-1
> > ethercodes.dat:785: Intruments  ==> Instruments
> > ethercodes.dat:838: Aircaft  ==> Aircraft
> > ethercodes.dat:1180: Engeneering  ==> Engineering
> > ethercodes.dat:2083: Internation  ==> International
> > ethercodes.dat:7447: MANAGMENT  ==> MANAGEMENT  
> 
> Except for debian/changelog all of these refer to files from upstream.
> From these files, the only thing that will end up being in the binary
> package is the typo in a comment of the arp2ethers /bin/sh script. I you
> prefer that I create a patch to fix that I will do so (I personally
> wouldn't bother).

Indeed, this is kind of useless if they don't appear in the binary/users
don't see it.

> The spelling mistake in debian/changelog is from an old entry. Should I
> rewrite the changelog.Debian history to fix that spelling mistake?

I wouldn't do it.

> Thanks also for including the commands you used to find problems with
> the package, that's really helpful.

These tools are helpful to improve the quality of your packages.

As you can see, there are a lot of false positive, and you may not
always want to "fix" everything. :)

I didn't have enough time to review all your changes to maintainer scripts,
but piuparts didn't report anything bad and we're uploading to experimental,
so I'd say the package is ready for upload.

Does anybody in the team wants to take a second look ?

Are you already a member of the team ? If yes, could you move your git
repository to https://anonscm.debian.org/git/pkg-security/arpwatch.git ?

If needed, I can remove the old one on collab-maint.

Cheers,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ ACB7 B67F 197F 9B32 1533 431C AC90 AC3E C524 065E
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-security-team/attachments/20170405/77b53cb9/attachment.sig>


Reply to: