intetsim review (Re: ITP: inetsim)

[Lukas Schwaighofer]

Hi GengYu,

On Fri, 10 Nov 2017 15:53:36 +0000
GengYu Rao <zouyoo at outlook.com> wrote:

> On Tuesday, October 31, 2017 04:53 AM, Lukas Schwaighofer wrote:
> > After you've fixed all that, I should be done with my review and we
> > can ask one of the DDs here to take a look! :)  
> I fixed all that, and there is an email from upstream
> 
> The runtime data directory is/var/lib/inetsim/.
> Sample files for some services are included in/usr/share/inetsim/.
> Some of those files are copied by the postinst script
> to/var/lib/inetsim/ where they can safely be replaced/deleted.
> 
> the upstream's package is somehow chaotic, and i put
> them all into /var/lib/inetsim/ in the install script.
> the upstream's deb is here.
> http://www.inetsim.org/debian/binary/inetsim_1.2.7-1_all.deb
> But i managed to get everything fine in my repo:)

I have some more feedback:

* You added quite a few patches in one commit (with the message "add
  the upstream patches").  Please add a DEP-3 [1] compatible header to
  those patches.  The patches should include a link to their origin.

* A lot of files are now installed to both
  /usr/share/inetsim/lib/ and /usr/share/perl5, they should only be
  installed once.

* You changed the location for a lot of files, but the "old" (empty)
  directories are still part of the package.
  - The "report" symlink still points to /var/lib/inetsim/report .

* Please write the commit messages more carefully.  It will be hosted
  on a Debian server, so your work is public and, if you maintain a
  package, it reflects on Debian:
  - I find a commit message "the ugly install", noting that you are not
    satisfied with the format of the install files, inappropriate.  The
    debhelper folks are making a lot of effort to make packaging as
    easy as possible.  If you have constructive feedback on how they
    can improve something, you can discuss with them on their mailing
    list.
  - "got an email from upstream i will post it here" (followed by the
    email) is not a good commit message either.  Explain what you did
    and add the information provided by upstream 
  - "this is important, please check previous commit message" is not
    helpful
  - If you have important notes for people handling the source (which
    includes future maintainers), create a debian/README.source
    file [2].  Commit messages are not a good place for that.

* It seems like default-disabled currently does not work as it should:
  The status is not synced properly to systemd… I think for now it's
  better to simply revert 62896682920aa5d56fc5dcf71d19dd2e5e3a225d and
  keep the ENABLED=0, even though I don't like that…

Regards
Lukas

[1] https://dep.alioth.debian.org/deps/dep3/
[2] https://www.debian.org/doc/debian-policy/#source-package-handling-debian-readme-source