Fix first boot deferral and postinst error reporting

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
....
}

The following changes since commit 0c537554fc2cf58a9cbfeaf39ed918c607483dce:

testimage: enable gi test suite (2018-01-29 10:31:58 +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 (7):
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 rootfs.py: change the notice about deferring scripts to first boot
from warning to a note

 .../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

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.

---
 scripts/postinst-intercepts/delay_to_first_boot | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 scripts/postinst-intercepts/delay_to_first_boot

new file mode 100644
index 00000000000..ecdbef95dde
@@ -0,0 +1,2 @@
+#!/bin/sh
+exit 1

2.15.1

---
 meta/lib/oe/rootfs.py | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

index 754ef563abe..d3ec8a56c48 100644
@@ -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

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.

---
 meta/lib/oe/rootfs.py | 4 ++++
 1 file changed, 4 insertions(+)

index d3ec8a56c48..8bf77aea492 100644
@@ -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

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.

---
 meta-selftest/recipes-test/postinst/postinst_1.0.bb | 14 ++------------
 meta/classes/package.bbclass                        | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 12 deletions(-)

index 112aa08c80f..d4bab6dcc22 100644
@@ -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

index 7dc759699f4..6a7f35a3e78 100644
@@ -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

'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.

---
 meta/lib/oe/package_manager.py | 3 +++
 1 file changed, 3 insertions(+)

index 2a07f0e39ad..f7e013437c9 100644
@@ -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

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.

---
 meta/classes/package_rpm.bbclass | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index af64ef62c58..e26b2ad6625 100644
@@ -470,12 +470,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('')

@@ -564,12 +564,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

Now that the first boot deferral needs to be requested explicitly, it's not really something to be concerned about.

---
 meta/lib/oe/rootfs.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8bf77aea492..bf2aea2b25a 100644
@@ -303,7 +303,7 @@ class Rootfs(object, metaclass=ABCMeta):
                     break
 
             if registered_pkgs is not None:
-                bb.warn("The postinstalls for the following packages "
+                bb.note("The postinstalls for the following packages "
                         "will be postponed for first boot: %s" %
                         registered_pkgs)
 

2.15.1

I fully agree with this patchset. The old way of defining seems a bit hacky. Also even if you follow the documented way to create postinstallation scripts [1] you still get noisy warnings which seem like a problem.

Thanks for the effort!

[1] http://www.yoctoproject.org/docs/2.4/mega-manual/mega-manual.html#new-recipe-post-installation-scripts

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Alexander Kanavin
Sent: Monday, January 29, 2018 1:01 PM
To: [email protected]
Subject: [OE-core] [PATCH 0/7] Fix first boot deferral and postinst error reporting

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
....
}

The following changes since commit 0c537554fc2cf58a9cbfeaf39ed918c607483dce:

testimage: enable gi test suite (2018-01-29 10:31:58 +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 (7):
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 rootfs.py: change the notice about deferring scripts to first boot
from warning to a note

 .../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

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

== Series Details ==

Series: Fix first boot deferral and postinst error reporting
Revision: 1
URL : https://patchwork.openembedded.org/series/10741/
State : failure

== Summary ==

Thank you for submitting this patch series to OpenEmbedded Core. This is an automated response. Several tests have been executed on the proposed series by patchtest resulting in the following failures:

* Patch            [3/7] meta/lib/oe/rootfs.py: do not execute defer_to_first_boot when processing postinst_intercept hooks
 Issue             Commit shortlog is too long [test_shortlog_length] 
  Suggested fix    Edit shortlog so that it is 90 characters or less (currently 98 characters)

If you believe any of these test results are incorrect, please reply to the mailing list ([email protected]) raising your concerns. Otherwise we would appreciate you correcting the issues and submitting a new version of the patchset if applicable. Please ensure you add/increment the version number when sending the new version (i.e. [PATCH] -> [PATCH v2] -> [PATCH v3] -> ...).

---

Guidelines:     https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe