Bug#857089: Fix dh_installinit options

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
Hi,

the attached patch seems to work for me.
Please test.

Wolfgang

0001-debian-rules-Fix-typo-in-dh_installinit-option-add-o.patch (813 bytes) Download Attachment
signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
Hi Wolfgang,

On Wed, Mar 08, 2017 at 11:45:45PM +0100, Wolfgang Schweer wrote:
> the attached patch seems to work for me.
> Please test.

Thanks for the patch!

>  override_dh_installinit:
> - dh_installinit --onlyscripts
> + dh_installinit --only-scripts --no-start

Sadly, it is not that easy. --no-start implies that the daemon also won't be
re-started on upgrades. And you really want that ;)

And the typo is not really one. dh accepts both:
                "o" => \$dh{ONLYSCRIPTS},
                "onlyscripts" => \$dh{ONLYSCRIPTS},
                "only-scripts" => \$dh{ONLYSCRIPTS},
(from /usr/share/perl5/Debian/Debhelper/Dh_Getopt.pm)

But given the hyphen-version is the one documented in the manpage,
we probably still should switch.

Regards
Evgeni

Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
Hi Evgeni,

On Sat, Mar 11, 2017 at 12:34:18PM +0100, Evgeni Golov wrote:
> >  override_dh_installinit:
> > - dh_installinit --onlyscripts
> > + dh_installinit --only-scripts --no-start
>
> Sadly, it is not that easy. --no-start implies that the daemon also won't be
> re-started on upgrades. And you really want that ;)

Right; saw it too late. Maybe it would be an idea to generate d/postinst
manually to have more options (compared to dh_*) for the upgrade case?
I tried that, but w/o success so far.

Wolfgang

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
Hi Wolfgang,

On Sat, Mar 11, 2017 at 01:57:47PM +0100, Wolfgang Schweer wrote:

> Hi Evgeni,
>
> On Sat, Mar 11, 2017 at 12:34:18PM +0100, Evgeni Golov wrote:
> > >  override_dh_installinit:
> > > - dh_installinit --onlyscripts
> > > + dh_installinit --only-scripts --no-start
> >
> > Sadly, it is not that easy. --no-start implies that the daemon also won't be
> > re-started on upgrades. And you really want that ;)
>
> Right; saw it too late. Maybe it would be an idea to generate d/postinst
> manually to have more options (compared to dh_*) for the upgrade case?
> I tried that, but w/o success so far.

I'd rather use the half-broken --no-start or rip out sysv support than
writing my own postinst (it just asks for doing something wrong :/).

Anyways. I actually think that writing "our own" postinst would not work
either. update-rc.d just does not give us enough support to do so.
 #705254 update-rc.d: Provide "is-enabled" command
 #857452 update-rc.d: please provide a defaults-disabled option

To recap, the behaviour should be as follows (IMHO):
on fresh install, the service is disabled and not started
on upgrade
* if the service was enabled, it remains enabled and gets restarted
* if the service was disabled, it remains so and is not started

For fresh install this is easy, just ommit the
 update-rc.d thinkfan defaults
call in postinst and be done (invoke-rc.d should behave properly)

For upgrades, we'd need something like:
 if update-rc.d thinkfan is-enabled;
   update-rc.d thinkfan defaults
 fi
or
 update-rc.d thinkfan defaults-desabled
Again, trusting that invoke-rc.d will behave properly afterwards.

But we don't have these commands :(

Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
Hi Evgeni,

On Sat, Mar 11, 2017 at 02:15:06PM +0100, Evgeni Golov wrote:

> To recap, the behaviour should be as follows (IMHO):
> on fresh install, the service is disabled and not started
> on upgrade
> * if the service was enabled, it remains enabled and gets restarted
> * if the service was disabled, it remains so and is not started
>
> For fresh install this is easy, just ommit the
>  update-rc.d thinkfan defaults
> call in postinst and be done (invoke-rc.d should behave properly)
>
> For upgrades, we'd need something like:
>  if update-rc.d thinkfan is-enabled;
>    update-rc.d thinkfan defaults
>  fi
> or
>  update-rc.d thinkfan defaults-desabled
> Again, trusting that invoke-rc.d will behave properly afterwards.
>
> But we don't have these commands :(
Yeah; good summary.
If the half-broken --no-start option would be used (to fix the RC bug)
then maybe a NEWS.Debian file could be shipped to inform about the steps
to take in case of upgrades.

Wolfgang

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
Hi,

On Sat, Mar 11, 2017 at 02:35:20PM +0100, Wolfgang Schweer wrote:

> > To recap, the behaviour should be as follows (IMHO):
> > on fresh install, the service is disabled and not started
> > on upgrade
> > * if the service was enabled, it remains enabled and gets restarted
> > * if the service was disabled, it remains so and is not started
>
> Yeah; good summary.
> If the half-broken --no-start option would be used (to fix the RC bug)
> then maybe a NEWS.Debian file could be shipped to inform about the steps
> to take in case of upgrades.
Agreed.

But with the help of fsateler and the postinst of src:puppet, I think I now
have a working solution. Just call update-rc.d disable on a fresh install.

Can you please test the attached patch?

Regards
Evgeni


0001-call-update-rc.d-thinkfan-disable-on-fresh-installs.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
On Sat, Mar 11, 2017 at 03:12:48PM +0100, Evgeni Golov wrote:
> But with the help of fsateler and the postinst of src:puppet, I think I now
> have a working solution. Just call update-rc.d disable on a fresh install.
>
> Can you please test the attached patch?
 
Tested on an up-to-date stretch system.

Fresh install: ok (no errors during installation, disabled like
expected; works after providing a working config file and running
service thinkfan enable/start).

Upgrade from version currently in stretch:
a) previously disabled: ok (no errors, still disabled).
b) previously enabled/running: ok (no errors, still running.

Good job, congrats!

Wolfgang

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
Hi,

On Sat, Mar 11, 2017 at 08:58:23PM +0100, Wolfgang Schweer wrote:
> On Sat, Mar 11, 2017 at 03:12:48PM +0100, Evgeni Golov wrote:
> > But with the help of fsateler and the postinst of src:puppet, I think I now
> > have a working solution. Just call update-rc.d disable on a fresh install.
> >
> > Can you please test the attached patch?
>  
> Tested on an up-to-date stretch system.

With systemd I guess?

> Fresh install: ok (no errors during installation, disabled like
> expected; works after providing a working config file and running
> service thinkfan enable/start).
>
> Upgrade from version currently in stretch:
> a) previously disabled: ok (no errors, still disabled).
> b) previously enabled/running: ok (no errors, still running.
>
> Good job, congrats!

Thanks!

There is still one thing left: in the SysV case, the user will have to
issue update-rc.d thinkfan enable AND edit /etc/default/thinkfan.

Thinking how to fix that.

Regards

Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
On Sat, Mar 11, 2017 at 09:41:55PM +0100, Evgeni Golov wrote:

> On Sat, Mar 11, 2017 at 08:58:23PM +0100, Wolfgang Schweer wrote:
> > On Sat, Mar 11, 2017 at 03:12:48PM +0100, Evgeni Golov wrote:
> > > But with the help of fsateler and the postinst of src:puppet, I think I now
> > > have a working solution. Just call update-rc.d disable on a fresh install.
> > >
> > > Can you please test the attached patch?
> >  
> > Tested on an up-to-date stretch system.
>
> With systemd I guess?
Yes.
 
> There is still one thing left: in the SysV case, the user will have to
> issue update-rc.d thinkfan enable AND edit /etc/default/thinkfan.
>
> Thinking how to fix that.
 
I guess it would be sufficient to add a line to /etc/default/thinkfan,
so that it's like this:

# Should thinkfan be started automatically on boot?
# Only say "yes" when you know what you are doing, have configured
# thinkfan correctly for *YOUR* machine and loaded thinkpad_acpi
# with fan_control=1 (if you have a ThinkPad).
# Also, running 'update-rc.d thinkfan enable' is needed.
START=no

Wolfgang

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
Hi,
On Sat, Mar 11, 2017 at 10:03:34PM +0100, Wolfgang Schweer wrote:

> > There is still one thing left: in the SysV case, the user will have to
> > issue update-rc.d thinkfan enable AND edit /etc/default/thinkfan.
> >
> > Thinking how to fix that.
>  
> I guess it would be sufficient to add a line to /etc/default/thinkfan,
> so that it's like this:
>
> # Should thinkfan be started automatically on boot?
> # Only say "yes" when you know what you are doing, have configured
> # thinkfan correctly for *YOUR* machine and loaded thinkpad_acpi
> # with fan_control=1 (if you have a ThinkPad).
> # Also, running 'update-rc.d thinkfan enable' is needed.
> START=no
I went a bit further, droped that code, and migrated the setting
to update-rc.d on upgrade. See attached patchset.

What do you think?

0001-call-update-rc.d-thinkfan-disable-on-fresh-installs.patch (2K) Download Attachment
0002-migrate-old-installations-to-a-properly-disabled-sys.patch (2K) Download Attachment
0003-drop-the-old-START-param-from-the-init-script-and-de.patch (2K) Download Attachment
0004-update-docs-on-enabling-the-service-under-sysvinit.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
On Sun, Mar 12, 2017 at 09:16:46AM +0100, Evgeni Golov wrote:
> I went a bit further, droped that code, and migrated the setting
> to update-rc.d on upgrade. See attached patchset.
>
> What do you think?

Looks awesome.

Tested all possible variants using both systemd and sysv related
commands. Seems to work like expected in all cases.

One point:
For the d/NEWS file to be actually shipped, at least for me this
additional patch was needed:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c650e16..c4f500d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -26,7 +26,7 @@ if(USE_ATASMART)
 endif(USE_ATASMART)
 
 install(TARGETS thinkfan DESTINATION sbin)
-install(FILES NEWS COPYING README examples/thinkfan.conf.complex
+install(FILES NEWS COPYING README debian/NEWS examples/thinkfan.conf.complex
  examples/thinkfan.conf.simple DESTINATION share/doc/thinkfan)
 install(FILES thinkfan.1 DESTINATION share/man/man1)
 
Wolfgang

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
Hey,

On Sun, Mar 12, 2017 at 05:42:34PM +0100, Wolfgang Schweer wrote:

> On Sun, Mar 12, 2017 at 09:16:46AM +0100, Evgeni Golov wrote:
> > I went a bit further, droped that code, and migrated the setting
> > to update-rc.d on upgrade. See attached patchset.
> >
> > What do you think?
>
> Looks awesome.
>
> Tested all possible variants using both systemd and sysv related
> commands. Seems to work like expected in all cases.

Thanks for testing!

> One point:
> For the d/NEWS file to be actually shipped, at least for me this
> additional patch was needed:

Uh? It should be automatically installed by dh_installchangelogs.
And happens here (it gets installed as /usr/share/doc/thinkfan/NEWS.Debian.gz).
Wonder why this did not work for you.

Btw, are you interested in co-maintaining thinkfan? The package could certainly
use another pair of eyes :)

Gruesse
Evgeni

Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
On Sun, Mar 12, 2017 at 06:04:00PM +0100, Evgeni Golov wrote:
> On Sun, Mar 12, 2017 at 05:42:34PM +0100, Wolfgang Schweer wrote:
> > One point:
> > For the d/NEWS file to be actually shipped, at least for me this
> > additional patch was needed:
>
> Uh? It should be automatically installed by dh_installchangelogs.
> And happens here (it gets installed as /usr/share/doc/thinkfan/NEWS.Debian.gz).
> Wonder why this did not work for you.

Maybe due to a different build tool; I used 'debuild -- binary'.
 
> Btw, are you interested in co-maintaining thinkfan? The package could
> certainly use another pair of eyes :)

Well, that would be beyond my skills; but I can try to do testing and
give feedback. Will be around in MG-Wickradt next Saturday, I guess.
 
Wolfgang

signature.asc (981 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Evgeni Golov-2
On Sun, Mar 12, 2017 at 06:30:52PM +0100, Wolfgang Schweer wrote:

> On Sun, Mar 12, 2017 at 06:04:00PM +0100, Evgeni Golov wrote:
> > On Sun, Mar 12, 2017 at 05:42:34PM +0100, Wolfgang Schweer wrote:
> > > One point:
> > > For the d/NEWS file to be actually shipped, at least for me this
> > > additional patch was needed:
> >
> > Uh? It should be automatically installed by dh_installchangelogs.
> > And happens here (it gets installed as /usr/share/doc/thinkfan/NEWS.Debian.gz).
> > Wonder why this did not work for you.
>
> Maybe due to a different build tool; I used 'debuild -- binary'.

Weird. I do build with gbp, but that also uses debuild under the hood.

Anyways, let's see what RT thinks about the diff.

> > Btw, are you interested in co-maintaining thinkfan? The package could
> > certainly use another pair of eyes :)
>
> Well, that would be beyond my skills; but I can try to do testing and
> give feedback. Will be around in MG-Wickradt next Saturday, I guess.

Cool. See you there :)
I would pay you some $favouritedrinks for all the testing,
but I guess my ex-employeer is paying anyways ;)

Reply | Threaded
Open this post in threaded view
|

Bug#857089: Fix dh_installinit options

Wolfgang Schweer-3
Hi Evgeni,

On Sun, Mar 12, 2017 at 06:37:56PM +0100, Evgeni Golov wrote:

> On Sun, Mar 12, 2017 at 06:30:52PM +0100, Wolfgang Schweer wrote:
> > On Sun, Mar 12, 2017 at 06:04:00PM +0100, Evgeni Golov wrote:
> > > On Sun, Mar 12, 2017 at 05:42:34PM +0100, Wolfgang Schweer wrote:
> > > > One point:
> > > > For the d/NEWS file to be actually shipped, at least for me this
> > > > additional patch was needed:
> > >
> > > Uh? It should be automatically installed by dh_installchangelogs.
> > > And happens here (it gets installed as /usr/share/doc/thinkfan/NEWS.Debian.gz).
> > > Wonder why this did not work for you.
> >
> > Maybe due to a different build tool; I used 'debuild -- binary'.
>
> Weird. I do build with gbp, but that also uses debuild under the hood.
Thanks for the hint about gbp.

After installing git-buildpackage, adjusting gbp.conf and running gbp,
the NEWS file is included in the package like expected.

To get rid of a lintian error lsb-base seems to be needed now as an
additional Depends.

Have fun at the BSP.
 
Wolfgang

signature.asc (981 bytes) Download Attachment