[PATCH 1/4] package.bbclass: run pre/post installation/removal scriptlets using sh -e

[PATCH 1/4] package.bbclass: run pre/post installation/removal 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 without any visible warnings or errors.

This was previously done only for rpm packages; this patch replaces
the rpm-specific tweak with one that works for all package types.

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

diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
index 317c77585fc..83f53a49ef6 100644
--- a/meta/classes/package.bbclass
+++ b/meta/classes/package.bbclass
@@ -1343,6 +1343,17 @@ fi
             postinst += postinst_ontarget
             d.setVar('pkg_postinst_%s' % pkg, postinst)
 
+    def add_set_e_to_scriptlets(pkg):
+        for scriptlet_name in ('pkg_preinst', 'pkg_postinst', 'pkg_prerm', 'pkg_postrm'):
+            scriptlet = d.getVar('%s_%s' % (scriptlet_name, pkg))
+            if scriptlet:
+                scriptlet_split = scriptlet.split('\n')
+                if scriptlet_split[0].startswith("#!"):
+                    scriptlet = scriptlet_split[0] + "\nset -e\n" + "\n".join(scriptlet_split[1:])
+                else:
+                    scriptlet = "set -e\n" + "\n".join(scriptlet_split[0:])
+            d.setVar('%s_%s' % (scriptlet_name, pkg), scriptlet)
+
     def write_if_exists(f, pkg, var):
         def encode(str):
             import codecs
@@ -1439,6 +1450,7 @@ fi
         write_if_exists(sf, pkg, 'FILES')
         write_if_exists(sf, pkg, 'CONFFILES')
         process_postinst_on_target(pkg, d.getVar("MLPREFIX"))
+        add_set_e_to_scriptlets(pkg)
         write_if_exists(sf, pkg, 'pkg_postinst')
         write_if_exists(sf, pkg, 'pkg_postrm')
         write_if_exists(sf, pkg, 'pkg_preinst')
diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
index e26b2ad6625..af64ef62c58 100644
--- a/meta/classes/package_rpm.bbclass
+++ b/meta/classes/package_rpm.bbclass
@@ -470,12 +470,12 @@ python write_specfile () {
 
         # Now process scriptlets
         if splitrpreinst:
-            spec_scriptlets_bottom.append('%%pre -n %s -p "/bin/sh -e"' % splitname)
+            spec_scriptlets_bottom.append('%%pre -n %s' % 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 -p "/bin/sh -e"' % splitname)
+            spec_scriptlets_bottom.append('%%post -n %s' % 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 -p "/bin/sh -e"')
+        spec_scriptlets_top.append('%pre')
         spec_scriptlets_top.append('# %s - preinst' % srcname)
         spec_scriptlets_top.append(srcrpreinst)
         spec_scriptlets_top.append('')
     if srcrpostinst:
-        spec_scriptlets_top.append('%post -p "/bin/sh -e"')
+        spec_scriptlets_top.append('%post')
         spec_scriptlets_top.append('# %s - postinst' % srcname)
         spec_scriptlets_top.append(srcrpostinst)
         spec_scriptlets_top.append('')
-- 
2.16.1

[PATCH 2/4] meta/lib/oe/package_manager.py: warn about failing scriptlets for all package types

[Alexander Kanavin]

Previously this was done only for rpm packages; now also ipk/deb scriptlet
failures are reported.

In the future this will become a hard error, but it can't yet happen
due to the legacy 'exit 1' way of deferring scriptlet execution to first boot which
needs a deprecation period.

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

diff --git a/meta/lib/oe/package_manager.py b/meta/lib/oe/package_manager.py
index c10efb8df4c..0c5cf3cf7f3 100644
--- a/meta/lib/oe/package_manager.py
+++ b/meta/lib/oe/package_manager.py
@@ -83,6 +83,11 @@ def opkg_query(cmd_output):
 
     return output
 
+# Note: this should be bb.fatal in the future.
+def failed_postinsts_warn(pkgs, log_path):
+    bb.warn("""Intentionally failing postinstall scriptlets of %s to defer them to first boot is deprecated. Please place them into pkg_postinst_ontarget_${PN} ().
+If deferring to first boot wasn't the intent, then scriptlet failure may mean an issue in the recipe, or a regression elsewhere.
+Details of the failure are in %s.""" %(pkgs, log_path))
 
 class Indexer(object, metaclass=ABCMeta):
     def __init__(self, d, deploy_dir):
@@ -709,8 +714,7 @@ class RpmPM(PackageManager):
                 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.")
+            failed_postinsts_warn(list(failed_scriptlets_pkgnames.keys()), self.d.expand("${T}/log.do_rootfs"))
         for pkg in failed_scriptlets_pkgnames.keys():
             self.save_rpmpostinst(pkg)
 
@@ -1172,6 +1176,13 @@ class OpkgPM(OpkgDpkgPM):
             bb.note(cmd)
             output = subprocess.check_output(cmd.split(), stderr=subprocess.STDOUT).decode("utf-8")
             bb.note(output)
+            failed_pkgs = []
+            for line in output.split('\n'):
+                if line.endswith("configuration required on target."):
+                    bb.warn(line)
+                    failed_pkgs.append(line.split(".")[0])
+            if failed_pkgs:
+                failed_postinsts_warn(failed_pkgs, self.d.expand("${T}/log.do_rootfs"))
         except subprocess.CalledProcessError as e:
             (bb.fatal, bb.warn)[attempt_only]("Unable to install packages. "
                                               "Command '%s' returned %d:\n%s" %
@@ -1435,9 +1446,10 @@ class DpkgPM(OpkgDpkgPM):
                                 stderr=subprocess.STDOUT).decode("utf-8")
                         bb.note(output)
                     except subprocess.CalledProcessError as e:
-                        bb.note("%s for package %s failed with %d:\n%s" %
+                        bb.warn("%s for package %s failed with %d:\n%s" %
                                 (control_script.name, pkg_name, e.returncode,
                                     e.output.decode("utf-8")))
+                        failed_postinsts_warn([pkg_name], self.d.expand("${T}/log.do_rootfs"))
                         failed_pkgs.append(pkg_name)
                         break
 
-- 
2.16.1

[PATCH 3/4] oe-selftest: add a test for failing package post-installation scriptlets

[Alexander Kanavin]

The test runs a scriptlet that has an intentionally failing command in the middle
and checks for two things:
1) that bitbake does warn the user about the failure
2) that scriptlet execution stops at that point.

The test is run for all three package types: rpm, deb, ipk.

Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
---
 .../recipes-test/postinst/postinst_1.0.bb          | 14 +++++++-
 meta/lib/oeqa/selftest/cases/runtime_test.py       | 37 ++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/meta-selftest/recipes-test/postinst/postinst_1.0.bb b/meta-selftest/recipes-test/postinst/postinst_1.0.bb
index d4bab6dcc22..913bfabf89e 100644
--- a/meta-selftest/recipes-test/postinst/postinst_1.0.bb
+++ b/meta-selftest/recipes-test/postinst/postinst_1.0.bb
@@ -3,11 +3,12 @@ LICENSE = "MIT"
 
 inherit allarch
 
-PACKAGES = "${PN}-rootfs ${PN}-delayed-a ${PN}-delayed-b"
+PACKAGES = "${PN}-rootfs ${PN}-delayed-a ${PN}-delayed-b ${PN}-rootfs-failing"
 
 ALLOW_EMPTY_${PN}-rootfs = "1"
 ALLOW_EMPTY_${PN}-delayed-a = "1"
 ALLOW_EMPTY_${PN}-delayed-b = "1"
+ALLOW_EMPTY_${PN}-rootfs-failing = "1"
 
 RDEPENDS_${PN}-delayed-a = "${PN}-rootfs"
 RDEPENDS_${PN}-delayed-b = "${PN}-delayed-a"
@@ -58,3 +59,14 @@ pkg_postinst_ontarget_${PN}-delayed-b () {
 
     touch ${TESTDIR}/delayed-b
 }
+
+# This scriptlet intentionally includes a bogus command in the middle to test 
+# that we catch and report such errors properly.
+pkg_postinst_${PN}-rootfs-failing () {
+    mkdir -p $D${TESTDIR}
+    touch $D${TESTDIR}/rootfs-before-failure
+    run_a_really_broken_command
+    # Scriptlet execution should stop here; the following commands are NOT supposed to run.
+    # (oe-selftest checks for it).
+    touch $D${TESTDIR}/rootfs-after-failure
+}
diff --git a/meta/lib/oeqa/selftest/cases/runtime_test.py b/meta/lib/oeqa/selftest/cases/runtime_test.py
index 1c69255b568..9c9b4b34111 100644
--- a/meta/lib/oeqa/selftest/cases/runtime_test.py
+++ b/meta/lib/oeqa/selftest/cases/runtime_test.py
@@ -221,3 +221,40 @@ class Postinst(OESelftestTestCase):
                         for filename in ("rootfs", "delayed-a", "delayed-b"):
                             status, output = qemu.run_serial("test -f %s && echo found" % os.path.join(targettestdir, filename))
                             self.assertEqual(output, "found", "%s was not present on boot" % filename)
+
+
+
+    def test_failing_postinst(self):
+        """
+        Summary:        The purpose of this test case is to verify that post-installation
+                        scripts that contain errors are properly reported.
+        Expected:       The scriptlet failure is properly reported.
+                        The file that is created after the error in the scriptlet is not present.
+        Product: oe-core
+        Author: Alexander Kanavin <alexander.kanavin@intel.com>
+        """
+
+        import oe.path
+
+        vars = get_bb_vars(("IMAGE_ROOTFS", "sysconfdir"), "core-image-minimal")
+        rootfs = vars["IMAGE_ROOTFS"]
+        self.assertIsNotNone(rootfs)
+        sysconfdir = vars["sysconfdir"]
+        self.assertIsNotNone(sysconfdir)
+        # Need to use oe.path here as sysconfdir starts with /
+        hosttestdir = oe.path.join(rootfs, sysconfdir, "postinst-test")
+
+        for classes in ("package_rpm", "package_deb", "package_ipk"):
+            with self.subTest(package_class=classes):
+                features = 'CORE_IMAGE_EXTRA_INSTALL = "postinst-rootfs-failing"\n'
+                features += 'PACKAGE_CLASSES = "%s"\n' % classes
+                self.write_config(features)
+                bb_result = bitbake('core-image-minimal')
+                self.assertGreaterEqual(bb_result.output.find("Intentionally failing postinstall scriptlets of ['postinst-rootfs-failing'] to defer them to first boot is deprecated."), 0,
+                    "Warning about a failed scriptlet not found in bitbake output: %s" %(bb_result.output))
+
+                self.assertTrue(os.path.isfile(os.path.join(hosttestdir, "rootfs-before-failure")),
+                                    "rootfs-before-failure file was not created")
+                self.assertFalse(os.path.isfile(os.path.join(hosttestdir, "rootfs-after-failure")),
+                                    "rootfs-after-failure file was created")
+
-- 
2.16.1

[PATCH 4/4] lib/oe/patch.py: add a warning if patch context was ignored

[Alexander Kanavin]

Ignoring patch context increases the chances of patches being
applied incorrectly. Depending on what code is being patched, this can go
completely unnoticed and create subtle bugs, sometimes with security implications.

Please see here for a specific example:
https://bugzilla.yoctoproject.org/show_bug.cgi?id=10450

On the other hand, we cannot simply force all patch context to match exactly:
doing this would break a lot of recipes suddenly, across all layers.

So let's try a softer approach: issue a warning, and gently update
patches over a longer span of time. When most of the warnings are eliminated,
we can start enforcing a strict patch application policy.

I do understand that this patch creates a lot of warnings all of a sudden, however
I believe the problem does need to be addressed. All of oe-core recipes have their
context already fixed.

Sample warning:

WARNING: vulkan-1.0.61.1-r0 do_patch:
Some of the context lines in patches were ignored. This can lead to incorrectly applied patches.
The context lines in the patches can be updated with devtool:

    devtool modify <recipe>
    devtool finish --force-patch-refresh <recipe> <layer_path>

Then the updated patches and the source tree (in devtool's workspace)
should be reviewed to make sure the patches apply in the correct place
and don't introduce duplicate lines (which can, and does happen
when some of the context is ignored).
Details:
Applying patch demos-Don-t-build-tri-or-cube.patch
patching file demos/CMakeLists.txt
Hunk #1 succeeded at 63 (offset 2 lines).
Hunk #2 succeeded at 76 with fuzz 1 (offset 2 lines).

[YOCTO #10450]

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

diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index f02dee8d27d..bfa7d218792 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -36,6 +36,22 @@ def runcmd(args, dir = None):
         (exitstatus, output) = oe.utils.getstatusoutput(cmd)
         if exitstatus != 0:
             raise CmdError(cmd, exitstatus >> 8, output)
+        if " fuzz " in output:
+            bb.warn("""
+Some of the context lines in patches were ignored. This can lead to incorrectly applied patches.
+The context lines in the patches can be updated with devtool:
+
+    devtool modify <recipe>
+    devtool finish --force-patch-refresh <recipe> <layer_path>
+
+Then the updated patches and the source tree (in devtool's workspace)
+should be reviewed to make sure the patches apply in the correct place
+and don't introduce duplicate lines (which can, and does happen
+when some of the context is ignored). Further information:
+http://lists.openembedded.org/pipermail/openembedded-core/2018-March/148675.html
+https://bugzilla.yoctoproject.org/show_bug.cgi?id=10450
+Details:
+{}""".format(output))
         return output
 
     finally:
-- 
2.16.1

Re: [PATCH 4/4] lib/oe/patch.py: add a warning if patch context was ignored

[Martin Jansa]

[-- Attachment #1: Type: text/plain, Size: 3820 bytes --]

Now when more people will use devtool to refresh patches, would it make
sense for devtool to use --no-signature when refreshing patches?

I think git version is not important in this context and causes extra
changes in .patch files (when refreshed by different people with different
git version) or extra work removing that chunk from resulting diff.


On Mon, Mar 12, 2018 at 5:49 PM, Alexander Kanavin <
alexander.kanavin@linux.intel.com> wrote:

> Ignoring patch context increases the chances of patches being
> applied incorrectly. Depending on what code is being patched, this can go
> completely unnoticed and create subtle bugs, sometimes with security
> implications.
>
> Please see here for a specific example:
> https://bugzilla.yoctoproject.org/show_bug.cgi?id=10450
>
> On the other hand, we cannot simply force all patch context to match
> exactly:
> doing this would break a lot of recipes suddenly, across all layers.
>
> So let's try a softer approach: issue a warning, and gently update
> patches over a longer span of time. When most of the warnings are
> eliminated,
> we can start enforcing a strict patch application policy.
>
> I do understand that this patch creates a lot of warnings all of a sudden,
> however
> I believe the problem does need to be addressed. All of oe-core recipes
> have their
> context already fixed.
>
> Sample warning:
>
> WARNING: vulkan-1.0.61.1-r0 do_patch:
> Some of the context lines in patches were ignored. This can lead to
> incorrectly applied patches.
> The context lines in the patches can be updated with devtool:
>
>     devtool modify <recipe>
>     devtool finish --force-patch-refresh <recipe> <layer_path>
>
> Then the updated patches and the source tree (in devtool's workspace)
> should be reviewed to make sure the patches apply in the correct place
> and don't introduce duplicate lines (which can, and does happen
> when some of the context is ignored).
> Details:
> Applying patch demos-Don-t-build-tri-or-cube.patch
> patching file demos/CMakeLists.txt
> Hunk #1 succeeded at 63 (offset 2 lines).
> Hunk #2 succeeded at 76 with fuzz 1 (offset 2 lines).
>
> [YOCTO #10450]
>
> Signed-off-by: Alexander Kanavin <alexander.kanavin@linux.intel.com>
> ---
>  meta/lib/oe/patch.py | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
> index f02dee8d27d..bfa7d218792 100644
> --- a/meta/lib/oe/patch.py
> +++ b/meta/lib/oe/patch.py
> @@ -36,6 +36,22 @@ def runcmd(args, dir = None):
>          (exitstatus, output) = oe.utils.getstatusoutput(cmd)
>          if exitstatus != 0:
>              raise CmdError(cmd, exitstatus >> 8, output)
> +        if " fuzz " in output:
> +            bb.warn("""
> +Some of the context lines in patches were ignored. This can lead to
> incorrectly applied patches.
> +The context lines in the patches can be updated with devtool:
> +
> +    devtool modify <recipe>
> +    devtool finish --force-patch-refresh <recipe> <layer_path>
> +
> +Then the updated patches and the source tree (in devtool's workspace)
> +should be reviewed to make sure the patches apply in the correct place
> +and don't introduce duplicate lines (which can, and does happen
> +when some of the context is ignored). Further information:
> +http://lists.openembedded.org/pipermail/openembedded-
> core/2018-March/148675.html
> +https://bugzilla.yoctoproject.org/show_bug.cgi?id=10450
> +Details:
> +{}""".format(output))
>          return output
>
>      finally:
> --
> 2.16.1
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>

[-- Attachment #2: Type: text/html, Size: 5136 bytes --]

Re: [PATCH 4/4] lib/oe/patch.py: add a warning if patch context was ignored

[Alexander Kanavin]

On 03/15/2018 05:13 PM, Martin Jansa wrote:
> Now when more people will use devtool to refresh patches, would it make 
> sense for devtool to use --no-signature when refreshing patches?

I don't think that will help much to reduce the amount of change in the 
refresh. The refesh will also update the line numbers in chunks so that 
they match reality, and convert patches into git format - all useful things.


Alex

Re: [PATCH 4/4] lib/oe/patch.py: add a warning if patch context was ignored

[Martin Jansa]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Yes, this time it updates more things, but in future updates it might be
just 1 line with line numbers and then different git version in the
signature. Which will make the diff few times bigger than it would need to
be.

On Thu, Mar 15, 2018 at 5:22 PM, Alexander Kanavin <
alexander.kanavin@linux.intel.com> wrote:

> On 03/15/2018 05:13 PM, Martin Jansa wrote:
>
>> Now when more people will use devtool to refresh patches, would it make
>> sense for devtool to use --no-signature when refreshing patches?
>>
>
> I don't think that will help much to reduce the amount of change in the
> refresh. The refesh will also update the line numbers in chunks so that
> they match reality, and convert patches into git format - all useful things.
>
>
> Alex
>

[-- Attachment #2: Type: text/html, Size: 1214 bytes --]

Re: [PATCH 4/4] lib/oe/patch.py: add a warning if patch context was ignored

[Alexander Kanavin]

On 03/15/2018 06:41 PM, Martin Jansa wrote:
> Yes, this time it updates more things, but in future updates it might be 
> just 1 line with line numbers and then different git version in the 
> signature. Which will make the diff few times bigger than it would need 
> to be.

Right. I think the patch extraction is done by extractPatches() in 
meta/lib/oe/patch.py, so if you can make and test the change (should be 
trivial), would be great :)

Alex

Re: [PATCH 4/4] lib/oe/patch.py: add a warning if patch context was ignored

[Khem Raj]

On Thu, Mar 15, 2018 at 8:13 AM, Martin Jansa <martin.jansa@gmail.com> wrote:
> Now when more people will use devtool to refresh patches, would it make
> sense for devtool to use --no-signature when refreshing patches?
>
> I think git version is not important in this context and causes extra
> changes in .patch files (when refreshed by different people with different
> git version) or extra work removing that chunk from resulting diff.
>

and also time/date in diff if they are not pruned already