[RFC PATCH 0/6] Fix first boot deferral and postinst error reporting

[RFC PATCH 0/6] Fix first boot deferral and postinst error reporting

[Alexander Kanavin]

This patchset is aiming to fix long-standing problems with package post-installation
scriptlets:

1) The way to defer them to first boot is to issue 'exit 1'. On the other hand,
'exit 1' is also the way to indicate an error in the script. The patches decouple
these two things.

2) Scriptlets should be executed using '-e', otherwise failing commands in
the middle of the scriptlet will not be reported as such.

3) The patchset also adds a convenient shortcut for deferring scriptlets to
first boot execution:

pkg_postinst_ontarget_PACKAGENAME() {
      # Actions to carry out on the device go here
      ....
}

Please review the following changes for suitability for inclusion. If you have
any objections or suggestions for improvement, please respond to the patches. If
you agree with the changes, please provide your Acked-by.

The following changes since commit 205cfd702190026e64eed9cae27c05ff62d1637e:

  bitbake: fetch2/__init__: Disable pseudo in runfetchcmd() (2018-01-08 08:48:54 +0000)

are available in the Git repository at:

  git://git.yoctoproject.org/poky-contrib akanavin/dnf-rpm4-postinst-fix
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=akanavin/dnf-rpm4-postinst-fix

Alexander Kanavin (6):
  postinst_intercepts: add a delay_to_first_boot hook
  meta/lib/oe/rootfs.py: separate first boot deferral logic into a
    separate function
  meta/lib/oe/rootfs.py: do not execute defer_to_first_boot when
    processing postinst_intercept hooks
  package.bbclass: add support for pkg_postinst_ontarget()
  meta/lib/oe/package_manager.py: deprecate 'exit 1' as a way to defer
    to first boot
  package_rpm.bbclass: run pre/post installation scriptlets using sh -e

 .../recipes-test/postinst/postinst_1.0.bb          | 14 ++------
 meta/classes/package.bbclass                       | 20 +++++++++++
 meta/classes/package_rpm.bbclass                   |  8 ++---
 meta/lib/oe/package_manager.py                     |  3 ++
 meta/lib/oe/rootfs.py                              | 39 +++++++++++++---------
 scripts/postinst-intercepts/delay_to_first_boot    |  2 ++
 6 files changed, 54 insertions(+), 32 deletions(-)
 create mode 100644 scripts/postinst-intercepts/delay_to_first_boot

-- 
2.15.1

[RFC PATCH 1/6] postinst_intercepts: add a delay_to_first_boot hook

[Alexander Kanavin]

This allows postinsts scripts to explicitly request a deferral to first boot
(by calling 'postinst_intercept delay_to_first_boot') instead of 'exit 1' which
should be used only to indicate actual script failures.

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 scripts/postinst-intercepts/delay_to_first_boot | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 scripts/postinst-intercepts/delay_to_first_boot

diff --git a/scripts/postinst-intercepts/delay_to_first_boot b/scripts/postinst-intercepts/delay_to_first_boot
new file mode 100644
index 00000000000..ecdbef95dde
--- /dev/null
+++ b/scripts/postinst-intercepts/delay_to_first_boot
@@ -0,0 +1,2 @@
+#!/bin/sh
+exit 1
-- 
2.15.1

[RFC PATCH 2/6] meta/lib/oe/rootfs.py: separate first boot deferral logic into a separate function

[Alexander Kanavin]

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 meta/lib/oe/rootfs.py | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/meta/lib/oe/rootfs.py b/meta/lib/oe/rootfs.py
index 754ef563abe..d3ec8a56c48 100644
--- a/meta/lib/oe/rootfs.py
+++ b/meta/lib/oe/rootfs.py
@@ -293,6 +293,24 @@ class Rootfs(object, metaclass=ABCMeta):
             # Remove the package manager data files
             self.pm.remove_packaging_data()
 
+    def _postpone_to_first_boot(self, postinst_intercept_hook):
+        with open(postinst_intercept_hook) as intercept:
+            registered_pkgs = None
+            for line in intercept.read().split("\n"):
+                m = re.match("^##PKGS:(.*)", line)
+                if m is not None:
+                    registered_pkgs = m.group(1).strip()
+                    break
+
+            if registered_pkgs is not None:
+                bb.warn("The postinstalls for the following packages "
+                        "will be postponed for first boot: %s" %
+                        registered_pkgs)
+
+                # call the backend dependent handler
+                self._handle_intercept_failure(registered_pkgs)
+
+
     def _run_intercepts(self):
         intercepts_dir = os.path.join(self.d.getVar('WORKDIR'),
                                       "intercept_scripts")
@@ -314,22 +332,7 @@ class Rootfs(object, metaclass=ABCMeta):
             except subprocess.CalledProcessError as e:
                 bb.warn("The postinstall intercept hook '%s' failed, details in log.do_rootfs" % script)
                 bb.note("Exit code %d. Output:\n%s" % (e.returncode, e.output.decode("utf-8")))
-
-                with open(script_full) as intercept:
-                    registered_pkgs = None
-                    for line in intercept.read().split("\n"):
-                        m = re.match("^##PKGS:(.*)", line)
-                        if m is not None:
-                            registered_pkgs = m.group(1).strip()
-                            break
-
-                    if registered_pkgs is not None:
-                        bb.warn("The postinstalls for the following packages "
-                                "will be postponed for first boot: %s" %
-                                registered_pkgs)
-
-                        # call the backend dependent handler
-                        self._handle_intercept_failure(registered_pkgs)
+                self._postpone_to_first_boot(script_full)
 
     def _run_ldconfig(self):
         if self.d.getVar('LDCONFIGDEPEND'):
-- 
2.15.1

[RFC PATCH 3/6] meta/lib/oe/rootfs.py: do not execute defer_to_first_boot when processing postinst_intercept hooks

[Alexander Kanavin]

That hook is empty, and doesn't need to be executed; it merely indicates that packages
that have used it are requesting to defer their postinst scripts to first boot
unconditionally.

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 meta/lib/oe/rootfs.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/meta/lib/oe/rootfs.py b/meta/lib/oe/rootfs.py
index d3ec8a56c48..8bf77aea492 100644
--- a/meta/lib/oe/rootfs.py
+++ b/meta/lib/oe/rootfs.py
@@ -324,6 +324,10 @@ class Rootfs(object, metaclass=ABCMeta):
             if script == "postinst_intercept" or not os.access(script_full, os.X_OK):
                 continue
 
+            if script == "delay_to_first_boot":
+                self._postpone_to_first_boot(script_full)
+                continue
+
             bb.note("> Executing %s intercept ..." % script)
 
             try:
-- 
2.15.1

[RFC PATCH 4/6] package.bbclass: add support for pkg_postinst_ontarget()

[Alexander Kanavin]

This function is a convenient and more readable shortcut for situations
when the postinst code always needs to run on target. All commands that
cannot be executed during cross-install and can only be run on target
should go into this function. They will only be executed on first boot
(if package was cross-installed) or immediately during package installation
on target.

Plain pkg_postinst() works as before: it is run during cross-install time,
it can contain a request to defer to first boot, and it is also run
during package installation on target.

Also fix the oeqa test for this functionality to use the new function
where appropriate.

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 meta-selftest/recipes-test/postinst/postinst_1.0.bb | 14 ++------------
 meta/classes/package.bbclass                        | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/meta-selftest/recipes-test/postinst/postinst_1.0.bb b/meta-selftest/recipes-test/postinst/postinst_1.0.bb
index 112aa08c80f..d4bab6dcc22 100644
--- a/meta-selftest/recipes-test/postinst/postinst_1.0.bb
+++ b/meta-selftest/recipes-test/postinst/postinst_1.0.bb
@@ -35,14 +35,9 @@ pkg_postinst_${PN}-rootfs () {
 
 # Depends on rootfs, delays until first boot, verifies that the rootfs file was
 # written.
-pkg_postinst_${PN}-delayed-a () {
+pkg_postinst_ontarget_${PN}-delayed-a () {
     set -e
 
-    if [ -n "$D" ]; then
-        echo "Delaying ${PN}-delayed-a until first boot"
-        exit 1
-    fi
-
     if [ ! -e ${TESTDIR}/rootfs ]; then
         echo "${PN}-delayed-a: ${TESTDIR}/rootfs not found"
         exit 1
@@ -53,14 +48,9 @@ pkg_postinst_${PN}-delayed-a () {
 
 # Depends on delayed-a, delays until first boot, verifies that the delayed-a file was
 # written. This verifies the ordering between delayed postinsts.
-pkg_postinst_${PN}-delayed-b () {
+pkg_postinst_ontarget_${PN}-delayed-b () {
     set -e
 
-    if [ -n "$D" ]; then
-        echo "Delaying ${PN}-delayed-b until first boot"
-        exit 1
-    fi
-
     if [ ! -e ${TESTDIR}/delayed-a ]; then
         echo "${PN}-delayed-b: ${TESTDIR}/delayed-a not found"
         exit 1
diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
index 7dc759699f4..6a7f35a3e78 100644
--- a/meta/classes/package.bbclass
+++ b/meta/classes/package.bbclass
@@ -1320,6 +1320,25 @@ python emit_pkgdata() {
     from glob import glob
     import json
 
+    def process_postinst_on_target(pkg, mlprefix):
+        defer_fragment = """
+if [ -n "$D" ]; then
+    $INTERCEPT_DIR/postinst_intercept delay_to_first_boot %s mlprefix=%s
+    exit 0
+fi
+""" % (pkg, mlprefix)
+
+        postinst = d.getVar('pkg_postinst_%s' % pkg)
+        postinst_ontarget = d.getVar('pkg_postinst_ontarget_%s' % pkg)
+
+        if postinst_ontarget:
+            bb.debug(1, 'adding deferred pkg_postinst_ontarget() to pkg_postinst() for %s' % pkg)
+            if not postinst:
+                postinst = '#!/bin/sh\n'
+            postinst += defer_fragment
+            postinst += postinst_ontarget
+            d.setVar('pkg_postinst_%s' % pkg, postinst)
+
     def write_if_exists(f, pkg, var):
         def encode(str):
             import codecs
@@ -1415,6 +1434,7 @@ python emit_pkgdata() {
         write_if_exists(sf, pkg, 'ALLOW_EMPTY')
         write_if_exists(sf, pkg, 'FILES')
         write_if_exists(sf, pkg, 'CONFFILES')
+        process_postinst_on_target(pkg, d.getVar("MLPREFIX"))
         write_if_exists(sf, pkg, 'pkg_postinst')
         write_if_exists(sf, pkg, 'pkg_postrm')
         write_if_exists(sf, pkg, 'pkg_preinst')
-- 
2.15.1

[RFC PATCH 5/6] meta/lib/oe/package_manager.py: deprecate 'exit 1' as a way to defer to first boot

[Alexander Kanavin]

'exit 1' is not optimal for two reasons:

1) Code is hard to read; it is not obvious that it means 'defer what follows to first boot'.
2) Worse, this hides actual errors in the scriptlets; there is no difference between scriptlet
failing because it's intended to be run on target and scriptlet failing because there's a bug or
a regression somewhere.

The new, supported way is to place the code that has to run on target into pkg_postinst_ontarget(),
or, if a more fine-tuned control is required, call 'postinst-intercepts defer_to_first_boot' from
pkg_postinst() to explicitly request deferral to first boot.

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 meta/lib/oe/package_manager.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meta/lib/oe/package_manager.py b/meta/lib/oe/package_manager.py
index ea99165d61b..0cc7f3cb3cc 100644
--- a/meta/lib/oe/package_manager.py
+++ b/meta/lib/oe/package_manager.py
@@ -604,6 +604,9 @@ class RpmPM(PackageManager):
             if line.startswith("Non-fatal POSTIN scriptlet failure in rpm package"):
                 failed_scriptlets_pkgnames[line.split()[-1]] = True
 
+        if len(failed_scriptlets_pkgnames) > 0:
+            bb.warn("Intentionally failing postinstall scriptlets of %s to defer them to first boot is deprecated. Please place them into pkg_postinst_ontarget_${PN} ()." %(list(failed_scriptlets_pkgnames.keys())))
+            bb.warn("If deferring to first boot wasn't the intent, then scriptlet failure may mean an issue in the recipe, or a regression elsewhere.")
         for pkg in failed_scriptlets_pkgnames.keys():
             self.save_rpmpostinst(pkg)
 
-- 
2.15.1

[RFC PATCH 6/6] package_rpm.bbclass: run pre/post installation scriptlets using sh -e

[Alexander Kanavin]

This allows catching errors in the scriptlets which would otherwise
go unnoticed, e.g. this sequence:

====
bogus_command
proper_command
====

would work just fine. Note that this patch needs all of the preceding
patches, as otherwise running failing scriptlets with -e would defer
them to first boot, instead of properly reporting failure and aborting
the package installation.

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 meta/classes/package_rpm.bbclass | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
index b08608c8466..f4b6d0b65f2 100644
--- a/meta/classes/package_rpm.bbclass
+++ b/meta/classes/package_rpm.bbclass
@@ -465,12 +465,12 @@ python write_specfile () {
 
         # Now process scriptlets
         if splitrpreinst:
-            spec_scriptlets_bottom.append('%%pre -n %s' % splitname)
+            spec_scriptlets_bottom.append('%%pre -n %s -p "/bin/sh -e"' % splitname)
             spec_scriptlets_bottom.append('# %s - preinst' % splitname)
             spec_scriptlets_bottom.append(splitrpreinst)
             spec_scriptlets_bottom.append('')
         if splitrpostinst:
-            spec_scriptlets_bottom.append('%%post -n %s' % splitname)
+            spec_scriptlets_bottom.append('%%post -n %s -p "/bin/sh -e"' % splitname)
             spec_scriptlets_bottom.append('# %s - postinst' % splitname)
             spec_scriptlets_bottom.append(splitrpostinst)
             spec_scriptlets_bottom.append('')
@@ -559,12 +559,12 @@ python write_specfile () {
     spec_preamble_top.append('')
 
     if srcrpreinst:
-        spec_scriptlets_top.append('%pre')
+        spec_scriptlets_top.append('%pre -p "/bin/sh -e"')
         spec_scriptlets_top.append('# %s - preinst' % srcname)
         spec_scriptlets_top.append(srcrpreinst)
         spec_scriptlets_top.append('')
     if srcrpostinst:
-        spec_scriptlets_top.append('%post')
+        spec_scriptlets_top.append('%post -p "/bin/sh -e"')
         spec_scriptlets_top.append('# %s - postinst' % srcname)
         spec_scriptlets_top.append(srcrpostinst)
         spec_scriptlets_top.append('')
-- 
2.15.1