intetsim review (Re: ITP: inetsim)

[Lukas Schwaighofer]

Hi GenYu,

package looks a lot better now, I think we're almost there!

* Since your package only creates one architecture independent package,
  I would move gcc-mingw-w64-i686 to normal Build-Depends and use
  "override_dh_auto_build" instead of the "-indep" variant.

* I think status_of_proc probably doesn't work as is, because the main
  process is called "inetsim_main" (and is no longer
  the /usr/bin/inetsim file); insert »-p "$PIDFILE"« right after
  status_of proc so it looks for the main process based on the pidfile.

* Since inetsim reads configuration relative to the current working
  directory, the helper script executing it actually does some harm.
  I'd propose to do the following:
  - Add a patch to adjust the lib patch of the inetsim perl script
    (diff syntax):
        -use lib "./lib";
        +use lib "/usr/share/inetsim/lib";
  - Install inetsim directly to /usr/bin, drop the helper script
  - To make sure the correct conf, data and log directories are used by
    inetsim when started from the init script, add
        --chdir /usr/share/inetsim
    to the `start-stop-daemon --start (...)` line of the init script
    (anywhere before the two dashes which mark the end of the options
    intended for start-stop-daemon)

* There is a new pedantic lintian tag regarding trailing whitespaces
  (since version 2.5.56), which you also might want to address:
  - file-contains-trailing-whitespace debian/control (line 17)
  - file-contains-trailing-whitespace debian/control (line 18)
  - file-contains-trailing-whitespace debian/control (line 19)
  - file-contains-trailing-whitespace debian/rules (line 4)
  - file-contains-trailing-whitespace debian/rules (line 8)
  - file-contains-trailing-whitespace debian/rules (line 9)
  - file-contains-trailing-whitespace debian/changelog (line 3)
  - file-contains-trailing-whitespace debian/changelog (line 8)
  - file-contains-trailing-whitespace debian/changelog (line 55)

* Changelog rewriting:
  - I would propose to remove all Closes: headers from before the first
    Debian release so this doesn't cause any confusion or people trying
    to look up those bugs in the Debian BTS.
  - Just for clarity, I would rewrite the very first changelog from
        "Initial release"
    to
        "Initial Kali release"

* Your own changelog entry should usually be more verbose than it is
  now.  Since this is the first upload to Debian, this is not as
  important this time… but at least add the following:
  - Debhelper compatibility level change (9→10)
  - Update of standards version

* And at last there is one thing that's a bit tricky: I'd rather get
  rid of the ENABLED=0 in /etc/default/inetsim since this has some
  problems and will probably be deprecated by the Debian policy
  soon [1].  Migrating away from that later is *really* ugly, so I'd
  rather get this right now.

  I've just asked in the #debian-mentors channel on IRC how to do
  that, here is a transcript (with permission from Niels Thykier to
  publish it here):
  21:13 < lus> And I've one more question.  I've seen there is a policy bug to 
               deprecate ENABLED=0/DISABLED=1 in /etc/default/$DAEMON (#601455)
  21:13 -zwiebelbot:#debian-mentors- Debian#601455: Standardize how to disable an 
            init script - https://bugs.debian.org/601455
  21:13 < lus> I've also seen that init-system-helpers now (version 1.50) 
               supports the "defaults-disabled" action.
  21:14 < lus> But there doesn't seem to be a way yet to generate code for the 
               maintainer scripts for services which should be disabled by 
               default using dh_installinit…
  21:14 < lus> What should I do if I want to introduce a new package now that 
               needs a service disabled by default?
  21:14 < lus> Write my own snippets for the maintainer scripts, wait until this 
               is supported, or use /etc/default/$NAME still?
  21:15 < nthykier> dh_installinit -- defaults-disabled ?
  21:15 < lus> oh, I missed it? ;)
  21:15 < lus> I thought I had checked…
  21:15 < nthykier> (you probably need to manually add the necessary dependency 
                    relation to inesure that init-system-helpers 1.50 is 
                    available at the correct points in time)
  21:16 < lus> but does that also ensure that the service is not started?
  21:16  * nthykier does not remember if that needs a Pre-Depends or if a Depends 
            is enough
  21:16 < nthykier> no, start / stop of a service is a separate thing
  21:17 < lus> hum… well I apparently need both… ;)
  21:17 < nthykier> There is a "--no-start"
  21:17 < nthykier> unfortunately, it implies "not stopping on removal"
  21:17 < lus> ok, so I only need a small snippet that will stop it on removal… 
               thanks!
  21:18 < lus> (and probably restart on upgrade in case it is running… need to 
               check that)

  I've just checked: adding »init-system-helpers (>= 1.50)« to Depends
  should be sufficient (no need for Pre-Depends).  Please try to update
  your package accordingly.  I didn't try this myself yet so this may
  not work as expected.  As usual, feel free to ask for help!

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! :)

Regards
Lukas

[1] https://bugs.debian.org/601455