[PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Peter Urbanec]

init-ifupdown inherits default implementations of prerm, postrm, preinst
and postinst from update-rc.d.bbclass. Unfortunately these default
implementations don't deal with package upgrades as well as they could.

What ends up happening is that opkg starts downloading and unpacking
packages sequentially. As it downloads and unpacks each package, opkg
calls prerm, preinst and postrm hook scripts. Once all packages are
unpacked, the postinst script is finally called for all packages as part
of the "configure" stage.

In the case of init-ifupdown, the default prerm and preinst scripts stop
networking. Networking is not brought back up again until the init-ifupdown
postinst is called, but that only happens after all the packages have been
downloaded, unpacked and installed. This leaves a window where any package
that needs to be downloaded after init-ifupdown is encountered will fail.

This patch fixes the problem by enhancing opkg to also provide the "upgrade"
argument to prerm and postrm, to (partially) match what dpkg does. See
https://wiki.debian.org/MaintainerScripts for dpkg diagrams that clarify
the intended process. opkg lacks the full functionality of dpkg, but for
the purpose of this exercise, they are similar enough.

I have submitted a patch to the opkg-devel list to include the "upgrade"
argument in future version of opkg.

The second part of the solution is an update to the default implementations
of the pre- and post- scripts provided by update-rc.d.bbclass. The scripts
are now careful to remove the package init scripts using the old package
context and to delay the restart of a service until the configure stage,
called from the postinst script.

---
 meta/classes/update-rc.d.bbclass                   | 75 +++++++++++++---------
 .../opkg/upgrade-argument-for-pre_postrm.patch     | 30 +++++++++
 meta/recipes-devtools/opkg/opkg_0.2.2.bb           |  1 +
 3 files changed, 77 insertions(+), 29 deletions(-)
 create mode 100644 meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch

diff --git a/meta/classes/update-rc.d.bbclass b/meta/classes/update-rc.d.bbclass
index bc1aa7d..c29457f 100644
--- a/meta/classes/update-rc.d.bbclass
+++ b/meta/classes/update-rc.d.bbclass
@@ -14,45 +14,62 @@ INITSCRIPT_PARAMS ?= "defaults"
  INIT_D_DIR = "${sysconfdir}/init.d"
 -updatercd_preinst() {
-if [ -z "$D" -a -f "${INIT_D_DIR}/${INITSCRIPT_NAME}" ]; then
-	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
+# During an upgrade, the pre/postrm scripts from old package are called
+# and the pre/postinst scripts called are from the new package.
+# See https://wiki.debian.org/MaintainerScripts for dpkg diagrams.
+# opkg uses a subset, which lacks most of the error handling.
+
+# Old package context, step 1
+updatercd_prerm() {
+if [ "x$1" != "xupgrade" ] ; then
+  ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
 fi
-if type update-rc.d >/dev/null 2>/dev/null; then
-	if [ -n "$D" ]; then
-		OPT="-f -r $D"
-	else
-		OPT="-f"
-	fi
-	update-rc.d $OPT ${INITSCRIPT_NAME} remove
+if [ -z "$D" ]; then
+  OPT=""
+else
+  OPT="-r $D"
 fi
-}
-
-updatercd_postinst() {
 if type update-rc.d >/dev/null 2>/dev/null; then
-	if [ -n "$D" ]; then
-		OPT="-r $D"
-	else
-		OPT="-s"
-	fi
-	update-rc.d $OPT ${INITSCRIPT_NAME} ${INITSCRIPT_PARAMS}
+  update-rc.d $OPT ${INITSCRIPT_NAME} remove
 fi
 }
 -updatercd_prerm() {
-if [ -z "$D" ]; then
-	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
-fi
+# New package context, step 2
+updatercd_preinst() {
+case "$1" in
+  upgrade)
+    ;;
+  *)
+    ;;
+esac
 }
 +# Old package context, step 3
 updatercd_postrm() {
+case "$1" in
+  upgrade)
+    ;;
+  *)
+    ;;
+esac
+}
+
+# N.B. Step 4 runs after all packages have been through steps 1-3 and therefore we
+#      need to delay service restarts during upgrade until here. Otherwise we end
+#      up with situations, like networking going down in the middle of "opkg upgrade",
+#      thus resulting in failures to fetch further packages.
+
+# New package context, step 4
+updatercd_postinst() {
 if type update-rc.d >/dev/null 2>/dev/null; then
-	if [ -n "$D" ]; then
-		OPT="-r $D"
-	else
-		OPT=""
-	fi
-	update-rc.d $OPT ${INITSCRIPT_NAME} remove
+  if [ -n "$D" ]; then
+    OPT="-r $D"
+  else
+    # This will catch the upgrade case and result in a restart.
+    ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
+    OPT="-s"
+  fi
+  update-rc.d $OPT ${INITSCRIPT_NAME} ${INITSCRIPT_PARAMS}
 fi
 }
 diff --git a/meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch b/meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch
new file mode 100644
index 0000000..5bea82a
--- /dev/null
+++ b/meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch
@@ -0,0 +1,30 @@
+From: Peter Urbanec
+Subject: [opkg] Use "upgrade" argument in prerm and postrm scripts
+
+Current implementation of opkg makes it difficult to distinguish between
+package removal or upgrade in prerm and postrm scripts. The following patch
+will make it easier and is close(r) to what dpkg does.
+
+diff --git a/libopkg/opkg_remove.c b/libopkg/opkg_remove.c
+index 07401b2..52454f8 100644
+--- a/libopkg/opkg_remove.c
++++ b/libopkg/opkg_remove.c
+@@ -292,7 +292,7 @@ opkg_remove_pkg(pkg_t *pkg, int from_upgrade)
+      pkg->state_want = SW_DEINSTALL;
+      opkg_state_changed++;
+ +-     if (pkg_run_script(pkg, "prerm", "remove") != 0) {
++     if (pkg_run_script(pkg, "prerm", from_upgrade ? "upgrade" : "remove") != 0) {
+          if (!conf->force_remove) {
+              opkg_msg(ERROR, "not removing package \"%s\", "
+                              "prerm script failed\n", pkg->name);
+@@ -310,7 +310,7 @@ opkg_remove_pkg(pkg_t *pkg, int from_upgrade)
+        feel free to fix this. */
+      remove_data_files_and_list(pkg);
+
+-     err = pkg_run_script(pkg, "postrm", "remove");
++     err = pkg_run_script(pkg, "postrm", from_upgrade ? "upgrade" : "remove");
+
+      remove_maintainer_scripts(pkg);
+      pkg->state_status = SS_NOT_INSTALLED;
+ diff --git a/meta/recipes-devtools/opkg/opkg_0.2.2.bb b/meta/recipes-devtools/opkg/opkg_0.2.2.bb
index 3dd7489..9c0045e 100644
--- a/meta/recipes-devtools/opkg/opkg_0.2.2.bb
+++ b/meta/recipes-devtools/opkg/opkg_0.2.2.bb
@@ -4,6 +4,7 @@ SRC_URI = "http://downloads.yoctoproject.org/releases/${BPN}/${BPN}-${PV}.tar.gz
            file://no-install-recommends.patch \
            file://add-exclude.patch \
            file://opkg-configure.service \
+           file://upgrade-argument-for-pre_postrm.patch \
 "
  S = "${WORKDIR}/${BPN}-${PV}"
-- 
2.1.2

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Paul Barker]

On 10 October 2014 17:45, Peter Urbanec <openembedded-devel@urbanec.net> wrote:
> init-ifupdown inherits default implementations of prerm, postrm, preinst
> and postinst from update-rc.d.bbclass. Unfortunately these default
> implementations don't deal with package upgrades as well as they could.
>
> What ends up happening is that opkg starts downloading and unpacking
> packages sequentially. As it downloads and unpacks each package, opkg
> calls prerm, preinst and postrm hook scripts. Once all packages are
> unpacked, the postinst script is finally called for all packages as part
> of the "configure" stage.
>
> In the case of init-ifupdown, the default prerm and preinst scripts stop
> networking. Networking is not brought back up again until the init-ifupdown
> postinst is called, but that only happens after all the packages have been
> downloaded, unpacked and installed. This leaves a window where any package
> that needs to be downloaded after init-ifupdown is encountered will fail.
>
> This patch fixes the problem by enhancing opkg to also provide the "upgrade"
> argument to prerm and postrm, to (partially) match what dpkg does. See
> https://wiki.debian.org/MaintainerScripts for dpkg diagrams that clarify
> the intended process. opkg lacks the full functionality of dpkg, but for
> the purpose of this exercise, they are similar enough.
>
> I have submitted a patch to the opkg-devel list to include the "upgrade"
> argument in future version of opkg.
>
> The second part of the solution is an update to the default implementations
> of the pre- and post- scripts provided by update-rc.d.bbclass. The scripts
> are now careful to remove the package init scripts using the old package
> context and to delay the restart of a service until the configure stage,
> called from the postinst script.
>

This patch needs a Signed-off-by line and a better subject as per the
OpenEmbedded guidelines
(http://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines).

It may also be better to split this into two consecutive patches, the
first of which makes changes to opkg, and the second of which makes
changes to update-rc.d.bbclass.

> ---
>  meta/classes/update-rc.d.bbclass                   | 75 +++++++++++++---------
>  .../opkg/upgrade-argument-for-pre_postrm.patch     | 30 +++++++++
>  meta/recipes-devtools/opkg/opkg_0.2.2.bb           |  1 +
>  3 files changed, 77 insertions(+), 29 deletions(-)
>  create mode 100644 meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch
>
> diff --git a/meta/classes/update-rc.d.bbclass b/meta/classes/update-rc.d.bbclass
> index bc1aa7d..c29457f 100644
> --- a/meta/classes/update-rc.d.bbclass
> +++ b/meta/classes/update-rc.d.bbclass
> @@ -14,45 +14,62 @@ INITSCRIPT_PARAMS ?= "defaults"
>   INIT_D_DIR = "${sysconfdir}/init.d"
>  -updatercd_preinst() {
> -if [ -z "$D" -a -f "${INIT_D_DIR}/${INITSCRIPT_NAME}" ]; then
> -       ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> +# During an upgrade, the pre/postrm scripts from old package are called
> +# and the pre/postinst scripts called are from the new package.
> +# See https://wiki.debian.org/MaintainerScripts for dpkg diagrams.
> +# opkg uses a subset, which lacks most of the error handling.
> +
> +# Old package context, step 1
> +updatercd_prerm() {
> +if [ "x$1" != "xupgrade" ] ; then
> +  ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
>  fi
> -if type update-rc.d >/dev/null 2>/dev/null; then
> -       if [ -n "$D" ]; then
> -               OPT="-f -r $D"
> -       else
> -               OPT="-f"
> -       fi
> -       update-rc.d $OPT ${INITSCRIPT_NAME} remove
> +if [ -z "$D" ]; then
> +  OPT=""
> +else
> +  OPT="-r $D"
>  fi
> -}
> -
> -updatercd_postinst() {
>  if type update-rc.d >/dev/null 2>/dev/null; then
> -       if [ -n "$D" ]; then
> -               OPT="-r $D"
> -       else
> -               OPT="-s"
> -       fi
> -       update-rc.d $OPT ${INITSCRIPT_NAME} ${INITSCRIPT_PARAMS}
> +  update-rc.d $OPT ${INITSCRIPT_NAME} remove
>  fi
>  }
>  -updatercd_prerm() {
> -if [ -z "$D" ]; then
> -       ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> -fi
> +# New package context, step 2
> +updatercd_preinst() {
> +case "$1" in
> +  upgrade)
> +    ;;
> +  *)
> +    ;;
> +esac
>  }
>  +# Old package context, step 3
>  updatercd_postrm() {
> +case "$1" in
> +  upgrade)
> +    ;;
> +  *)
> +    ;;
> +esac
> +}
> +
> +# N.B. Step 4 runs after all packages have been through steps 1-3 and therefore we
> +#      need to delay service restarts during upgrade until here. Otherwise we end
> +#      up with situations, like networking going down in the middle of "opkg upgrade",
> +#      thus resulting in failures to fetch further packages.
> +
> +# New package context, step 4
> +updatercd_postinst() {
>  if type update-rc.d >/dev/null 2>/dev/null; then
> -       if [ -n "$D" ]; then
> -               OPT="-r $D"
> -       else
> -               OPT=""
> -       fi
> -       update-rc.d $OPT ${INITSCRIPT_NAME} remove
> +  if [ -n "$D" ]; then
> +    OPT="-r $D"
> +  else
> +    # This will catch the upgrade case and result in a restart.
> +    ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> +    OPT="-s"
> +  fi
> +  update-rc.d $OPT ${INITSCRIPT_NAME} ${INITSCRIPT_PARAMS}
>  fi
>  }

Someone else will have to review the above as I don't know much about
update-rc.d.

>  diff --git a/meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch b/meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch
> new file mode 100644
> index 0000000..5bea82a
> --- /dev/null
> +++ b/meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch
> @@ -0,0 +1,30 @@
> +From: Peter Urbanec
> +Subject: [opkg] Use "upgrade" argument in prerm and postrm scripts
> +
> +Current implementation of opkg makes it difficult to distinguish between
> +package removal or upgrade in prerm and postrm scripts. The following patch
> +will make it easier and is close(r) to what dpkg does.
> +

This patch file also requires a Signed-off-by line and an
Upstream-status line. The upstream status will be 'submitted' as this
patch has been sent to the opkg mailing list.

> +diff --git a/libopkg/opkg_remove.c b/libopkg/opkg_remove.c
> +index 07401b2..52454f8 100644
> +--- a/libopkg/opkg_remove.c
> ++++ b/libopkg/opkg_remove.c
> +@@ -292,7 +292,7 @@ opkg_remove_pkg(pkg_t *pkg, int from_upgrade)
> +      pkg->state_want = SW_DEINSTALL;
> +      opkg_state_changed++;
> + +-     if (pkg_run_script(pkg, "prerm", "remove") != 0) {
> ++     if (pkg_run_script(pkg, "prerm", from_upgrade ? "upgrade" : "remove") != 0) {
> +          if (!conf->force_remove) {
> +              opkg_msg(ERROR, "not removing package \"%s\", "
> +                              "prerm script failed\n", pkg->name);
> +@@ -310,7 +310,7 @@ opkg_remove_pkg(pkg_t *pkg, int from_upgrade)
> +        feel free to fix this. */
> +      remove_data_files_and_list(pkg);
> +
> +-     err = pkg_run_script(pkg, "postrm", "remove");
> ++     err = pkg_run_script(pkg, "postrm", from_upgrade ? "upgrade" : "remove");
> +
> +      remove_maintainer_scripts(pkg);
> +      pkg->state_status = SS_NOT_INSTALLED;

I'm happy with this change, both here and upstream.

> + diff --git a/meta/recipes-devtools/opkg/opkg_0.2.2.bb b/meta/recipes-devtools/opkg/opkg_0.2.2.bb
> index 3dd7489..9c0045e 100644
> --- a/meta/recipes-devtools/opkg/opkg_0.2.2.bb
> +++ b/meta/recipes-devtools/opkg/opkg_0.2.2.bb
> @@ -4,6 +4,7 @@ SRC_URI = "http://downloads.yoctoproject.org/releases/${BPN}/${BPN}-${PV}.tar.gz
>             file://no-install-recommends.patch \
>             file://add-exclude.patch \
>             file://opkg-configure.service \
> +           file://upgrade-argument-for-pre_postrm.patch \
>  "
>   S = "${WORKDIR}/${BPN}-${PV}"

Again, this change looks good. If you're resubmitting this, put the
new patch above the service file just so that it's slightly easier to
see the grouping of file types.

Cheers,

-- 
Paul Barker

Email: paul@paulbarker.me.uk
http://www.paulbarker.me.uk

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Andreas Oberritter]

On 10.10.2014 18:45, Peter Urbanec wrote:
> init-ifupdown inherits default implementations of prerm, postrm, preinst
> and postinst from update-rc.d.bbclass. Unfortunately these default
> implementations don't deal with package upgrades as well as they could.
> 
> What ends up happening is that opkg starts downloading and unpacking
> packages sequentially. As it downloads and unpacks each package, opkg
> calls prerm, preinst and postrm hook scripts. Once all packages are
> unpacked, the postinst script is finally called for all packages as part
> of the "configure" stage.
> 
> In the case of init-ifupdown, the default prerm and preinst scripts stop
> networking.

I think prerm and postrm scripts should exit silently on upgrade. At
least that's what happens with rpm and with a patch just submitted for
deb [1]. I guess ipk should implement the same logic, for which your
patch to opkg is a prerequisite.

> Networking is not brought back up again until the init-ifupdown
> postinst is called, but that only happens after all the packages have been
> downloaded, unpacked and installed. This leaves a window where any package
> that needs to be downloaded after init-ifupdown is encountered will fail.
> 
> This patch fixes the problem by enhancing opkg to also provide the "upgrade"
> argument to prerm and postrm, to (partially) match what dpkg does. See
> https://wiki.debian.org/MaintainerScripts for dpkg diagrams that clarify
> the intended process. opkg lacks the full functionality of dpkg, but for
> the purpose of this exercise, they are similar enough.
> 
> I have submitted a patch to the opkg-devel list to include the "upgrade"
> argument in future version of opkg.
> 
> The second part of the solution is an update to the default implementations
> of the pre- and post- scripts provided by update-rc.d.bbclass. The scripts
> are now careful to remove the package init scripts using the old package
> context and to delay the restart of a service until the configure stage,
> called from the postinst script.
> 
> ---
>  meta/classes/update-rc.d.bbclass                   | 75 +++++++++++++---------
>  .../opkg/upgrade-argument-for-pre_postrm.patch     | 30 +++++++++
>  meta/recipes-devtools/opkg/opkg_0.2.2.bb           |  1 +
>  3 files changed, 77 insertions(+), 29 deletions(-)
>  create mode 100644 meta/recipes-devtools/opkg/opkg/upgrade-argument-for-pre_postrm.patch
> 
> diff --git a/meta/classes/update-rc.d.bbclass b/meta/classes/update-rc.d.bbclass
> index bc1aa7d..c29457f 100644
> --- a/meta/classes/update-rc.d.bbclass
> +++ b/meta/classes/update-rc.d.bbclass
> @@ -14,45 +14,62 @@ INITSCRIPT_PARAMS ?= "defaults"
>   INIT_D_DIR = "${sysconfdir}/init.d"
>  -updatercd_preinst() {
> -if [ -z "$D" -a -f "${INIT_D_DIR}/${INITSCRIPT_NAME}" ]; then
> -	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> +# During an upgrade, the pre/postrm scripts from old package are called
> +# and the pre/postinst scripts called are from the new package.
> +# See https://wiki.debian.org/MaintainerScripts for dpkg diagrams.
> +# opkg uses a subset, which lacks most of the error handling.
> +
> +# Old package context, step 1
> +updatercd_prerm() {
> +if [ "x$1" != "xupgrade" ] ; then

I think using "x..." syntax is obsolete. "$1" != "upgrade" should work
with every supported shell and is easier to read.

This part of the patch wouldn't be needed if package_ipk.bbclass
implemented the same logic as rpm mentioned above.

But, on the other hand, maybe package_rpm.bbclass is wrong... ;-)

> +  ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
>  fi
> -if type update-rc.d >/dev/null 2>/dev/null; then
> -	if [ -n "$D" ]; then
> -		OPT="-f -r $D"
> -	else
> -		OPT="-f"
> -	fi
> -	update-rc.d $OPT ${INITSCRIPT_NAME} remove
> +if [ -z "$D" ]; then
> +  OPT=""
> +else
> +  OPT="-r $D"
>  fi
> -}
> -
> -updatercd_postinst() {
>  if type update-rc.d >/dev/null 2>/dev/null; then
> -	if [ -n "$D" ]; then
> -		OPT="-r $D"
> -	else
> -		OPT="-s"
> -	fi
> -	update-rc.d $OPT ${INITSCRIPT_NAME} ${INITSCRIPT_PARAMS}
> +  update-rc.d $OPT ${INITSCRIPT_NAME} remove
>  fi
>  }
>  -updatercd_prerm() {
> -if [ -z "$D" ]; then
> -	${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> -fi
> +# New package context, step 2
> +updatercd_preinst() {
> +case "$1" in
> +  upgrade)
> +    ;;
> +  *)
> +    ;;
> +esac
>  }
>  +# Old package context, step 3
>  updatercd_postrm() {
> +case "$1" in
> +  upgrade)
> +    ;;
> +  *)
> +    ;;
> +esac
> +}
> +

The two functions above don't do anything, so they should be removed.

> +# N.B. Step 4 runs after all packages have been through steps 1-3 and therefore we
> +#      need to delay service restarts during upgrade until here. Otherwise we end
> +#      up with situations, like networking going down in the middle of "opkg upgrade",
> +#      thus resulting in failures to fetch further packages.
> +
> +# New package context, step 4
> +updatercd_postinst() {
>  if type update-rc.d >/dev/null 2>/dev/null; then
> -	if [ -n "$D" ]; then
> -		OPT="-r $D"
> -	else
> -		OPT=""
> -	fi
> -	update-rc.d $OPT ${INITSCRIPT_NAME} remove
> +  if [ -n "$D" ]; then
> +    OPT="-r $D"
> +  else
> +    # This will catch the upgrade case and result in a restart.
> +    ${INIT_D_DIR}/${INITSCRIPT_NAME} stop
> +    OPT="-s"
> +  fi
> +  update-rc.d $OPT ${INITSCRIPT_NAME} ${INITSCRIPT_PARAMS}

I have patches to update-rc.d [2] and update-rc.d.bbclass [3] to improve
handling of distributions with both systemd and sysvinit distro features
enabled. What they try to avoid is direct calls to init scripts, where
systemd may provide own unit files. Would it be feasible to use
update-rc.d to issue a restart command?

Regards,
Andreas

[1]:
http://lists.openembedded.org/pipermail/openembedded-core/2014-October/097872.html
[2]:
http://git.openembedded.org/openembedded-core-contrib/commit/?h=obi/master&id=a4e4f2083f693f1715e589eda6dd403f311086fe
[3]:
http://git.openembedded.org/openembedded-core-contrib/commit/?h=obi/master&id=68080e144cb894b41bca5a1ca74d1e18fb00104c

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Paul Barker]

On 11 October 2014 15:41, Paul Barker <paul@paulbarker.me.uk> wrote:
> On 10 October 2014 17:45, Peter Urbanec <openembedded-devel@urbanec.net> wrote:
>> +diff --git a/libopkg/opkg_remove.c b/libopkg/opkg_remove.c
>> +index 07401b2..52454f8 100644
>> +--- a/libopkg/opkg_remove.c
>> ++++ b/libopkg/opkg_remove.c
>> +@@ -292,7 +292,7 @@ opkg_remove_pkg(pkg_t *pkg, int from_upgrade)
>> +      pkg->state_want = SW_DEINSTALL;
>> +      opkg_state_changed++;
>> + +-     if (pkg_run_script(pkg, "prerm", "remove") != 0) {
>> ++     if (pkg_run_script(pkg, "prerm", from_upgrade ? "upgrade" : "remove") != 0) {
>> +          if (!conf->force_remove) {
>> +              opkg_msg(ERROR, "not removing package \"%s\", "
>> +                              "prerm script failed\n", pkg->name);
>> +@@ -310,7 +310,7 @@ opkg_remove_pkg(pkg_t *pkg, int from_upgrade)
>> +        feel free to fix this. */
>> +      remove_data_files_and_list(pkg);
>> +
>> +-     err = pkg_run_script(pkg, "postrm", "remove");
>> ++     err = pkg_run_script(pkg, "postrm", from_upgrade ? "upgrade" : "remove");
>> +
>> +      remove_maintainer_scripts(pkg);
>> +      pkg->state_status = SS_NOT_INSTALLED;
>
> I'm happy with this change, both here and upstream.
>

I think I need to take that back - the crappiness of the legacy code
in opkg strikes again!

opkg_remove_pkg is never called with from_upgrade set to a non-zero
value. The removal of an old package during an upgrade is handled by
directly calling remove_data_files_and_list and
remove_maintainer_scripts from opkg_install_pkg. The desired behaviour
is given in the comments in prerm_upgrade_old_pkg in opkg_install.c,
the behaviour given by the comment in that function just needs
implementing.

This is already entered as issue 104 in the opkg issue tracker:
https://code.google.com/p/opkg/issues/detail?id=104

Have you tested this patch before submitting it?

-- 
Paul Barker

Email: paul@paulbarker.me.uk
http://www.paulbarker.me.uk

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Peter Urbanec]

On 12/10/14 06:17, Paul Barker wrote:
> opkg_remove_pkg is never called with from_upgrade set to a non-zero
> value. The removal of an old package during an upgrade is handled by
> directly calling remove_data_files_and_list and
> remove_maintainer_scripts from opkg_install_pkg. The desired behaviour
> is given in the comments in prerm_upgrade_old_pkg in opkg_install.c,
> the behaviour given by the comment in that function just needs
> implementing.

Indeed, I got fooled by looking at the call to opkg_install_pkg(pkg, 1)

> This is already entered as issue 104 in the opkg issue tracker:
> https://code.google.com/p/opkg/issues/detail?id=104

So it is. Slightly more work than my trivial opkg patch, but then again, 
my trivial opkg patch is not much use ;-) Please disregard it.

> Have you tested this patch before submitting it?

To be honest, I have not tested the opkg "upgrade" patch in isolation. I 
have tested the opkg patch for multiple package upgrade case together 
with my changes to update-rc.d.bbclass changes (i.e. the entire patch I 
posted to the oe-core list) and this has solved the problems that I 
encountered initially. The "fix" for my use case is due to the changes 
in the way the prerm, postrm and postinst scripts work, but reviewing 
those changes, that's not good enough either.

Thanks for the review.

I'll try again later...

Peter

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Paul Barker]

On 13 October 2014 06:23, Peter Urbanec <openembedded-devel@urbanec.net> wrote:
> On 12/10/14 06:17, Paul Barker wrote:
>>
>> opkg_remove_pkg is never called with from_upgrade set to a non-zero
>> value. The removal of an old package during an upgrade is handled by
>> directly calling remove_data_files_and_list and
>> remove_maintainer_scripts from opkg_install_pkg. The desired behaviour
>> is given in the comments in prerm_upgrade_old_pkg in opkg_install.c,
>> the behaviour given by the comment in that function just needs
>> implementing.
>
>
> Indeed, I got fooled by looking at the call to opkg_install_pkg(pkg, 1)

No worries, there's still a lot of confusing code in opkg I need to
clean up. I've got a patch ready which will remove the second argument
to opkg_remove_pkg to prevent similar confusion in the future.

>
>> This is already entered as issue 104 in the opkg issue tracker:
>> https://code.google.com/p/opkg/issues/detail?id=104
>
>
> So it is. Slightly more work than my trivial opkg patch, but then again, my
> trivial opkg patch is not much use ;-) Please disregard it.
>
>> Have you tested this patch before submitting it?
>
>
> To be honest, I have not tested the opkg "upgrade" patch in isolation. I
> have tested the opkg patch for multiple package upgrade case together with
> my changes to update-rc.d.bbclass changes (i.e. the entire patch I posted to
> the oe-core list) and this has solved the problems that I encountered
> initially. The "fix" for my use case is due to the changes in the way the
> prerm, postrm and postinst scripts work, but reviewing those changes, that's
> not good enough either.
>
> Thanks for the review.
>
> I'll try again later...

I decided to go ahead and implement the required function yesterday as
it's on my todo list for the v0.3.0 release. I'll forward you the
patch once it's been briefly tested, it should apply fairly cleanly to
v0.2.2 or v0.2.3 and so it should be possible to apply it within
OpenEmbedded if you don't want to wait for the v0.3.0 release.

>
> Peter

Thanks,

-- 
Paul Barker

Email: paul@paulbarker.me.uk
http://www.paulbarker.me.uk

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Peter Urbanec]

I have revised the patches and have taken some of your feedback on 
board. See comments further down...

On 12/10/14 03:04, Andreas Oberritter wrote:
> On 10.10.2014 18:45, Peter Urbanec wrote:
>> In the case of init-ifupdown, the default prerm and preinst scripts stop
>> networking.
>
> I think prerm and postrm scripts should exit silently on upgrade. At
> least that's what happens with rpm and with a patch just submitted for
> deb [1]. I guess ipk should implement the same logic, for which your
> patch to opkg is a prerequisite.

I modelled my changes on documented behaviour for dpkg since opkg code 
often refers to dpkg behaviour. dpkg supports error return codes from 
these scripts and can do various error handling. opkg is less featured 
in that respect at this moment, but I think in the interest of future 
compatibility (and the principle of least astonishment) the scripts 
should return non-zero to signify that the install/remove/upgrade 
process can not continue. Having said that, I really don't see many 
reasons for a prerm or postrm script to try to abort an upgrade.


>> +if [ "x$1" != "xupgrade" ] ; then
>
> I think using "x..." syntax is obsolete. "$1" != "upgrade" should work
> with every supported shell and is easier to read.

Good to hear. I observed the "prepend-x" idiom in some parts of the 
existing code base and copied it. I knew it solved some compatibility 
issues dating back to last millennium, but if we know that all current 
systems can handle the cleaner syntax, all the better. I changed the 
resubmitted patch.


> This part of the patch wouldn't be needed if package_ipk.bbclass
> implemented the same logic as rpm mentioned above.
>
> But, on the other hand, maybe package_rpm.bbclass is wrong... ;-)

I think you have me confused here. package_ipk.bbclass tries to grab the 
prerm/postrm and preinst/postinst script variables from the package and 
writes the contents into the corresponding files within the *.ipk. These 
scripts are then run by opkg when the *.ipk files are processed.


>> +# New package context, step 2
>> +updatercd_preinst() {
>> +case "$1" in
>> +  upgrade)
>> +    ;;
>> +  *)
>> +    ;;
>> +esac
>>   }
>>   +# Old package context, step 3
>>   updatercd_postrm() {
>> +case "$1" in
>> +  upgrade)
>> +    ;;
>> +  *)
>> +    ;;
>> +esac
>> +}
>> +
>
> The two functions above don't do anything, so they should be removed.

They are used later by update_rcd_package(). They could have been 
simpler, but I thought that both the presence of comments and the 
template code would serve as a reminder of the semantics of the upgrade 
process and the fact that package upgrades are different to package 
installs or removals.


> I have patches to update-rc.d [2] and update-rc.d.bbclass [3] to improve
> handling of distributions with both systemd and sysvinit distro features
> enabled. What they try to avoid is direct calls to init scripts, where
> systemd may provide own unit files. Would it be feasible to use
> update-rc.d to issue a restart command?

The patch I submitted uses the "-s" flag to update-rc.d to start the 
service when it is installed, but as far as I can tell, there is no way 
to just stop the service using update-rc.d and the restart case will not 
work for scripts that don't support the "restart" argument (such as 
urandom). The restart case is also predicated on a number of conditions 
that will generally evaluate to false, since the old links would have 
been removed in prerm.

Cheers,
	Peter

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Andreas Oberritter]

Hello Peter,

sorry, I didn't notice this reply before sending out the previous mail.
I should have looked into my normal inbox first...

On 16.10.2014 03:51, Peter Urbanec wrote:
> I have revised the patches and have taken some of your feedback on
> board. See comments further down...
> 
> On 12/10/14 03:04, Andreas Oberritter wrote:
>> On 10.10.2014 18:45, Peter Urbanec wrote:
>>> In the case of init-ifupdown, the default prerm and preinst scripts stop
>>> networking.
>>
>> I think prerm and postrm scripts should exit silently on upgrade. At
>> least that's what happens with rpm and with a patch just submitted for
>> deb [1]. I guess ipk should implement the same logic, for which your
>> patch to opkg is a prerequisite.
> 
> I modelled my changes on documented behaviour for dpkg since opkg code
> often refers to dpkg behaviour. dpkg supports error return codes from
> these scripts and can do various error handling. opkg is less featured
> in that respect at this moment, but I think in the interest of future
> compatibility (and the principle of least astonishment) the scripts
> should return non-zero to signify that the install/remove/upgrade
> process can not continue. Having said that, I really don't see many
> reasons for a prerm or postrm script to try to abort an upgrade.

Yes, opkg should behave like dpkg, I agree.

But update-rc.d just generates scripts for dpkg, opkg and rpm, using
OE-Core's expected behaviour, which may well differ from Debian.

What happens when generating rpm (and since few days deb) packages is
that code gets injected that makes these scripts exit with return code 0
on upgrades. This may be just a workaround to emulate opkg's
limitations, but it may also be by design. I guess this should be
decided upon first. Whatever we'll agree on, the most important thing is
that all three supported package formats should behave identically.

>> This part of the patch wouldn't be needed if package_ipk.bbclass
>> implemented the same logic as rpm mentioned above.
>>
>> But, on the other hand, maybe package_rpm.bbclass is wrong... ;-)
> 
> I think you have me confused here. package_ipk.bbclass tries to grab the
> prerm/postrm and preinst/postinst script variables from the package and
> writes the contents into the corresponding files within the *.ipk. These
> scripts are then run by opkg when the *.ipk files are processed.

I meant to say that if package_ipk wanted to behave like package_rpm
(and package_deb since a few days), then it would inject '[ "$1" !=
upgrade ] || exit 0' at the beginning of postrm.

>>> +# New package context, step 2
>>> +updatercd_preinst() {
>>> +case "$1" in
>>> +  upgrade)
>>> +    ;;
>>> +  *)
>>> +    ;;
>>> +esac
>>>   }
>>>   +# Old package context, step 3
>>>   updatercd_postrm() {
>>> +case "$1" in
>>> +  upgrade)
>>> +    ;;
>>> +  *)
>>> +    ;;
>>> +esac
>>> +}
>>> +
>>
>> The two functions above don't do anything, so they should be removed.
> 
> They are used later by update_rcd_package(). They could have been
> simpler, but I thought that both the presence of comments and the
> template code would serve as a reminder of the semantics of the upgrade
> process and the fact that package upgrades are different to package
> installs or removals.

If they are empty, they should be removed from update_rcd_package(),
too. There are many more places in OE-Core where postinst scripts are
used, and the common policy is to implement only those snippets which
are used. Otherwise it's just a waste of space on the target systems.

The semantics of these scripts should get documented inside the Yocto
Project docs, or, if you want to document it inline, in a more central
place like package_{deb,ipk,rpm}, where they are actually used to
assemble package metadata.

>> I have patches to update-rc.d [2] and update-rc.d.bbclass [3] to improve
>> handling of distributions with both systemd and sysvinit distro features
>> enabled. What they try to avoid is direct calls to init scripts, where
>> systemd may provide own unit files. Would it be feasible to use
>> update-rc.d to issue a restart command?
> 
> The patch I submitted uses the "-s" flag to update-rc.d to start the
> service when it is installed, but as far as I can tell, there is no way
> to just stop the service using update-rc.d and the restart case will not
> work for scripts that don't support the "restart" argument (such as
> urandom).

Isn't restart required to be implemented by LSB? Maybe we could just fix
urandom instead. Does urandom even use update-rc.d? I don't know which
recipe actually installs a urandom init script, to be honest.

> The restart case is also predicated on a number of conditions
> that will generally evaluate to false, since the old links would have
> been removed in prerm.

I don't understand which conditions you're referring to. But if prerm
was a no-op on upgrade, as with rpm and deb and opkg before your other
patch, this surely wouldn't be an issue.

Regards,
Andreas

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Peter Urbanec]

Sorry for being slow to respond, I've been snowed under and dealing with 
hardware failures :-(

I'll be brief...

On 17/10/14 07:55, Andreas Oberritter wrote:
> What happens when generating rpm (and since few days deb) packages is
> that code gets injected that makes these scripts exit with return code 0
> on upgrades. This may be just a workaround to emulate opkg's
> limitations, but it may also be by design. I guess this should be
> decided upon first. Whatever we'll agree on, the most important thing is
> that all three supported package formats should behave identically.

I don't think this is a good idea. Consider the case where a package is 
being upgraded and the old package was configured to run in runlevels 3 
and 4. The new package is configured to run in runlevels 2 and 3. You 
need to run the old prerm script so that you correctly remove the script 
from all old runlevels, then you need to run the new postinst script to 
add the script to the correct runlevels. If you prevent prerm from 
running on upgrade, you will never remove the script from runlevel 4.

>>> But, on the other hand, maybe package_rpm.bbclass is wrong... ;-)

I have not studied RPM packaging. Does it use the same scripts and more 
importantly does RPM expect the same install logic? Perhaps it's not 
possible to apply the same logic to all packaging and installation systems.

> I meant to say that if package_ipk wanted to behave like package_rpm
> (and package_deb since a few days), then it would inject '[ "$1" !=
> upgrade ] || exit 0' at the beginning of postrm.

See my two points above. Perhaps one size does not fit all in this case.


>>>> +updatercd_preinst() {
>>>>    updatercd_postrm() {
>>> The two functions above don't do anything, so they should be removed.
>>
>> They are used later by update_rcd_package().
>
> If they are empty, they should be removed from update_rcd_package(),
> too. There are many more places in OE-Core where postinst scripts are
> used, and the common policy is to implement only those snippets which
> are used. Otherwise it's just a waste of space on the target systems.

I guess the logic could be changed so that if the user does not provide 
their own implementation, then nothing is generated. I haven't studied 
the code in depth to establish whether this may have a cascading effect 
down the line. I opted for the option that is less likely to cause 
unintended breakage.

>> The patch I submitted uses the "-s" flag to update-rc.d to start the
>> service when it is installed, but as far as I can tell, there is no way
>> to just stop the service using update-rc.d and the restart case will not
>> work for scripts that don't support the "restart" argument (such as
>> urandom).
>
> Isn't restart required to be implemented by LSB? Maybe we could just fix
> urandom instead. Does urandom even use update-rc.d? I don't know which
> recipe actually installs a urandom init script, to be honest.

I'm not sure what the LSB requirements are, but I did take a quick look 
through a few (random) existing scripts to determine the state of 
affairs and concluded that one can not rely on a "restart" 
implementation. The comments in init-ifupdown/init actually say that 
"restart" is deprecated and should not be used.

>> The restart case is also predicated on a number of conditions
>> that will generally evaluate to false, since the old links would have
>> been removed in prerm.
>
> I don't understand which conditions you're referring to.

It's a bit confusing to follow, but have a look at the part of the code 
that checks the existence of the links in the context of the first point 
I made in this email. If you want to support the upgrade logic properly 
(at least as far as doing things like changing the runlevels), you 
should remove the old links in prerm, which will later cause the code to 
incorrectly assume that this was not an upgrade.

Cheers,
	Peter

Re: [PATCH] Better support for upgrading packages in opkg and update-rc.d.bbclass

[Andreas Oberritter]

On 22.10.2014 16:42, Peter Urbanec wrote:
> Sorry for being slow to respond, I've been snowed under and dealing with
> hardware failures :-(
> 
> I'll be brief...
> 
> On 17/10/14 07:55, Andreas Oberritter wrote:
>> What happens when generating rpm (and since few days deb) packages is
>> that code gets injected that makes these scripts exit with return code 0
>> on upgrades. This may be just a workaround to emulate opkg's
>> limitations, but it may also be by design. I guess this should be
>> decided upon first. Whatever we'll agree on, the most important thing is
>> that all three supported package formats should behave identically.
> 
> I don't think this is a good idea. Consider the case where a package is
> being upgraded and the old package was configured to run in runlevels 3
> and 4. The new package is configured to run in runlevels 2 and 3. You
> need to run the old prerm script so that you correctly remove the script
> from all old runlevels, then you need to run the new postinst script to
> add the script to the correct runlevels. If you prevent prerm from
> running on upgrade, you will never remove the script from runlevel 4.

I guess it should be possible for update-rc.d to update the symlinks in
postinst if desired (remove unused ones, add new ones).

Anyway, ignoring or even breaking rpm and deb is not an option.

>>>> But, on the other hand, maybe package_rpm.bbclass is wrong... ;-)
> 
> I have not studied RPM packaging. Does it use the same scripts and more
> importantly does RPM expect the same install logic? Perhaps it's not
> possible to apply the same logic to all packaging and installation systems.

To the extend used by OE-Core, rpm uses the same install logic.

>> I meant to say that if package_ipk wanted to behave like package_rpm
>> (and package_deb since a few days), then it would inject '[ "$1" !=
>> upgrade ] || exit 0' at the beginning of postrm.
> 
> See my two points above. Perhaps one size does not fit all in this case.
> 
> 
>>>>> +updatercd_preinst() {
>>>>>    updatercd_postrm() {
>>>> The two functions above don't do anything, so they should be removed.
>>>
>>> They are used later by update_rcd_package().
>>
>> If they are empty, they should be removed from update_rcd_package(),
>> too. There are many more places in OE-Core where postinst scripts are
>> used, and the common policy is to implement only those snippets which
>> are used. Otherwise it's just a waste of space on the target systems.
> 
> I guess the logic could be changed so that if the user does not provide
> their own implementation, then nothing is generated. I haven't studied
> the code in depth to establish whether this may have a cascading effect
> down the line. I opted for the option that is less likely to cause
> unintended breakage.

Please just remove them. Users of update-rc.d.bbclass don't need to
override updatercd_preinst if it is empty. All recipes are allowed to
implement their own preinst snippets the way they need to and don't
depend on update-rc.d.bbclass for this.

Preinst scripts are not a feature of update-rc.d.bbclass, but are used
in many places and many classes just append shell snippets as they see fit.

>>> The patch I submitted uses the "-s" flag to update-rc.d to start the
>>> service when it is installed, but as far as I can tell, there is no way
>>> to just stop the service using update-rc.d and the restart case will not
>>> work for scripts that don't support the "restart" argument (such as
>>> urandom).
>>
>> Isn't restart required to be implemented by LSB? Maybe we could just fix
>> urandom instead. Does urandom even use update-rc.d? I don't know which
>> recipe actually installs a urandom init script, to be honest.
> 
> I'm not sure what the LSB requirements are, but I did take a quick look
> through a few (random) existing scripts to determine the state of
> affairs and concluded that one can not rely on a "restart"
> implementation. The comments in init-ifupdown/init actually say that
> "restart" is deprecated and should not be used.

Does it state any reason? How is restart different from stop and start
in the case of init-ifupdown? Most probably, init-ifupdown should never
stop or restart on upgrades in order to keep the network connection
alive in case of errors. If there are other reasons, you could just
implement restart by calling stop followed by start from within the init
script, as many other scripts already do.

If restart is working for the vast majority of recipes, then I think it
should be the default behaviour. Other recipes, if any, may choose to
implement their own scripts.

Regards,
Andreas

>>> The restart case is also predicated on a number of conditions
>>> that will generally evaluate to false, since the old links would have
>>> been removed in prerm.
>>
>> I don't understand which conditions you're referring to.
> 
> It's a bit confusing to follow, but have a look at the part of the code
> that checks the existence of the links in the context of the first point
> I made in this email. If you want to support the upgrade logic properly
> (at least as far as doing things like changing the runlevels), you
> should remove the old links in prerm, which will later cause the code to
> incorrectly assume that this was not an upgrade.
> 
> Cheers,
>     Peter
>