Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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 ;)
Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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
Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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?
>From b5d70ee07a27b4baae726c36786b78725dec0a5a Mon Sep 17 00:00:00 2001
From: Evgeni Golov
Date: Sat, 11 Mar 2017 14:58:59 +0100
Subject: [PATCH 1/4] call update-rc.d thinkfan disable on fresh installs
Closes: #758579
---
debian/thinkfan.lintian-overrides | 3 +++
debian/thinkfan.postinst | 44 +++
2 files changed, 47 insertions(+)
create mode 100644 debian/thinkfan.lintian-overrides
create mode 100644 debian/thinkfan.postinst
diff --git a/debian/thinkfan.lintian-overrides b/debian/thinkfan.lintian-overrides
new file mode 100644
index 000..62ce724
--- /dev/null
+++ b/debian/thinkfan.lintian-overrides
@@ -0,0 +1,3 @@
+# pre-deployment of a disabled service as dh_installinit is
+# not capable to do so for us.
+thinkfan: duplicate-updaterc.d-calls-in-postinst thinkfan
diff --git a/debian/thinkfan.postinst b/debian/thinkfan.postinst
new file mode 100644
index 000..c385313
--- /dev/null
+++ b/debian/thinkfan.postinst
@@ -0,0 +1,44 @@
+#!/bin/sh
+# postinst script for thinkfan
+#
+# see: dh_installdeb(1)
+
+set -e
+
+# summary of how this script can be called:
+#* `configure'
+#* `abort-upgrade'
+#* `abort-remove' `in-favour'
+#
+#* `abort-remove'
+#* `abort-deconfigure' `in-favour'
+#`removing'
+#
+# for details, see https://www.debian.org/doc/debian-policy/ or
+# the debian-policy package
+
+
+case "$1" in
+configure)
+if [ -x "/etc/init.d/thinkfan" ] && [ -z "$2" ]; then
+# Disable the service by default on new installations
+update-rc.d thinkfan defaults >/dev/null || true
+update-rc.d thinkfan disable >/dev/null || true
+ fi
+;;
+
+abort-upgrade|abort-remove|abort-deconfigure)
+;;
+
+*)
+echo "postinst called with unknown argument \`$1'" >&2
+exit 1
+;;
+esac
+
+# dh_installdeb will replace this with shell code automatically
+# generated by other debhelper scripts.
+
+#DEBHELPER#
+
+exit 0
--
2.11.0
>From 38cb2ac4448044f40931ec97065c7a7f8b4fd54c Mon Sep 17 00:00:00 2001
From: Evgeni Golov
Date: Sun, 12 Mar 2017 08:39:33 +0100
Subject: [PATCH 2/4] migrate old installations to a properly disabled sysv
service too
---
debian/thinkfan.lintian-overrides | 4
debian/thinkfan.preinst | 43 +++
2 files changed, 47 insertions(+)
create mode 100644 debian/thinkfan.preinst
diff --git a/debian/thinkfan.lintian-overrides b/debian/thinkfan.lintian-overrides
index 62ce724..292b9cd 100644
--- a/debian/thinkfan.lintian-overrides
+++ b/debian/thinkfan.lintian-overrides
@@ -1,3 +1,7 @@
# pre-deployment of a disabled service as dh_installinit is
# not capable to do so for us.
thinkfan: duplicate-updaterc.d-calls-in-postinst thinkfan
+
+# only called for upgrades from older versions to migrate
+# enabled/disabled state
+thinkfan: preinst-calls-updaterc.d thinkfan
diff --git a/debian/thinkfan.preinst b/debian/thinkfan.preinst
new file mode 100644
index 000..da9ed04
--- /dev/null
+++ b/debian/thinkfan.preinst
@@ -0,0 +1,43 @@
+#!/bin/sh
+# preinst script for thinkfan
+#
+# see: dh_installdeb(1)
+
+set -e
+
+# summary of how this script can be called:
+#* `install'
+#* `install'
+#* `upgrade'
+#* `abort-upgrade'
+# for details, see https://www.debian.org/doc/debian-policy/ or
+# the debian-policy package
+
+
+case "$1" in
+install|upgrade)
+if [ -n "$2" ] && dpkg --compare-versions "$2" lt "0.9.3-2~" && [ -r /etc/default/thinkfan ]; then
+. /etc/default/thinkfan
+if [ "${START}" != "yes" ]; then
+ # Disable the service if it was disabled via /e/d/thinkfan
+ update-rc.d thinkfan defaults >/dev/null || true
+ update-rc.d thinkfan disable >/dev/null || true
+fi
+fi
+;;
+
+abort-upgrade)
+;;
+
+*)
+echo "preinst called with unknown argument \`$1'" >&2
+exit 1
+;;
+esac
+
+# dh_installdeb will replace this with shell code automatically
+# generated by other debhelper scripts.
+
+#DEBHELPER#
+
+exit 0
--
2.11.0
>From a43574c2f
Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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
Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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 >From 87320b76e2d7c7e055eb6ae85828d22c90d02442 Mon Sep 17 00:00:00 2001 From: Evgeni Golov Date: Sat, 11 Mar 2017 14:58:59 +0100 Subject: [PATCH] call update-rc.d thinkfan disable on fresh installs Closes: #758579 --- debian/thinkfan.lintian-overrides | 3 +++ debian/thinkfan.postinst | 21 + 2 files changed, 24 insertions(+) create mode 100644 debian/thinkfan.lintian-overrides create mode 100644 debian/thinkfan.postinst diff --git a/debian/thinkfan.lintian-overrides b/debian/thinkfan.lintian-overrides new file mode 100644 index 000..62ce724 --- /dev/null +++ b/debian/thinkfan.lintian-overrides @@ -0,0 +1,3 @@ +# pre-deployment of a disabled service as dh_installinit is +# not capable to do so for us. +thinkfan: duplicate-updaterc.d-calls-in-postinst thinkfan diff --git a/debian/thinkfan.postinst b/debian/thinkfan.postinst new file mode 100644 index 000..7ea8f60 --- /dev/null +++ b/debian/thinkfan.postinst @@ -0,0 +1,21 @@ +#!/bin/sh +# postinst script for thinkfan +# +# see: dh_installdeb(1) + +set -e + +if [ "$1" = "configure" ]; then + if [ -x "/etc/init.d/thinkfan" ] && [ -z "$2" ]; then + # Disable the service by default on new installations + update-rc.d thinkfan defaults >/dev/null || true + update-rc.d thinkfan disable >/dev/null || true + fi +fi + +# dh_installdeb will replace this with shell code automatically +# generated by other debhelper scripts. + +#DEBHELPER# + +exit 0 -- 2.11.0
Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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 :(
Bug#857089: Fix dh_installinit options
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 Description: PGP signature
Bug#857089: Fix dh_installinit options
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
Bug#857089: Fix dh_installinit options
Hi, the attached patch seems to work for me. Please test. Wolfgang From 4cd12369ca5b1fc384043edf32bd1498cf5bb082 Mon Sep 17 00:00:00 2001 From: Wolfgang Schweer Date: Wed, 8 Mar 2017 23:28:40 +0100 Subject: [PATCH] debian/rules: Fix typo in dh_installinit option, add option '--no-start'. --- debian/rules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/rules b/debian/rules index d017648..8cc923b 100755 --- a/debian/rules +++ b/debian/rules @@ -14,7 +14,7 @@ override_dh_auto_install: install -D -m0644 rcscripts/thinkfan.service $(CURDIR)/debian/thinkfan/lib/systemd/system/thinkfan.service override_dh_installinit: - dh_installinit --onlyscripts + dh_installinit --only-scripts --no-start override_dh_systemd_enable: # Do not enable the file by default on purpose. -- 2.11.0 signature.asc Description: PGP signature
