[PATCH] packagegroup: Add init-manager sanity check

[PATCH] packagegroup: Add init-manager sanity check

[Richard Purdie]

Currently, you can set VIRTUAL-RUNTIME_init_manager to an init system that
isn't in DISTRO_FEATURES. This leads to head scratching over unbootable images.

This adds a sanity check which ensures more valid systems are built.

(From OE-Core rev: 485413a33e981114722dfa096caf226f083f7aab)

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
diff --git a/meta/classes/packagegroup.bbclass b/meta/classes/packagegroup.bbclass
index 201309c..9bc9cc2 100644
--- a/meta/classes/packagegroup.bbclass
+++ b/meta/classes/packagegroup.bbclass
@@ -38,3 +38,10 @@ do_configure[noexec] = "1"
 do_compile[noexec] = "1"
 do_install[noexec] = "1"
 do_populate_sysroot[noexec] = "1"
+
+python () {
+    initman = d.getVar("VIRTUAL-RUNTIME_init_manager", True)
+    if initman and initman in ['sysvinit', 'systemd'] and not base_contains('DISTRO_FEATURES', initman, True, False, d):
+        bb.fatal("Please ensure that your setting of VIRTUAL-RUNTIME_init_manager (%s) matches the entries enabled in DISTRO_FEATURES" % initman)
+}
+

Re: [PATCH] packagegroup: Add init-manager sanity check

[Enrico Scholz]

Richard Purdie
<richard.purdie-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
writes:

> Currently, you can set VIRTUAL-RUNTIME_init_manager to an init system
> that isn't in DISTRO_FEATURES. This leads to head scratching over
> unbootable images.

Because this sanity check is placed into an anonymous function, this
change affects also images which do not not include packagegroup-core*
in their images and are not using VIRTUAL-RUNTIME_init_manager at all.

It would be probably better to execute this check only, when the package
gets built.  Or add

DEPENDS += "${@some_check(d)}"

where some_check(d) evaluates to 'broken-virtual-runtime' or so when bad
configuration has been detected and empty else.



Enrico

Re: [PATCH] packagegroup: Add init-manager sanity check

[Richard Purdie]

On Thu, 2013-04-18 at 12:19 +0200, Enrico Scholz wrote:
> 
> Richard Purdie
> <richard.purdie-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> writes:
> 
> > Currently, you can set VIRTUAL-RUNTIME_init_manager to an init system
> > that isn't in DISTRO_FEATURES. This leads to head scratching over
> > unbootable images.
> 
> Because this sanity check is placed into an anonymous function, this
> change affects also images which do not not include packagegroup-core*
> in their images and are not using VIRTUAL-RUNTIME_init_manager at all.

Affects in that it runs the anonymous python fragment but does nothing? 

If you're worried about that overhead, there are fragments elsewhere
which have a much more significant overhead.

> It would be probably better to execute this check only, when the package
> gets built.  Or add
> 
> DEPENDS += "${@some_check(d)}"
> 
> where some_check(d) evaluates to 'broken-virtual-runtime' or so when bad
> configuration has been detected and empty else.

I did give this quite a bit of thought and couldn't come up with a
better way to handle it. The DEPENDS change above would certainly
trigger an error but it wouldn't be obvious to the user what happened or
how they might fix it. Making that approach give a better error message
would be much more invasive and have worse performance impact.

Other proposals for solutions are welcome. I thought it better to catch
a common user misconfiguration than generate broken images silently
though.

Cheers,

Richard

Re: [PATCH] packagegroup: Add init-manager sanity check

[Enrico Scholz]

Richard Purdie <richard.purdie@linuxfoundation.org> writes:

>> > Currently, you can set VIRTUAL-RUNTIME_init_manager to an init
>> > system that isn't in DISTRO_FEATURES. This leads to head scratching
>> > over unbootable images.
>> 
>> Because this sanity check is placed into an anonymous function, this
>> change affects also images which do not not include packagegroup-core*
>> in their images and are not using VIRTUAL-RUNTIME_init_manager at all.
>
> Affects in that it runs the anonymous python fragment but does nothing?

no; the 'parsing recipes' phase throws an exceptions

| ERROR: Please ensure that your setting of VIRTUAL-RUNTIME_init_manager (sysvinit) matches the entries enabled in DISTRO_FEATURES##                                                               | ETA:  00:00:14
| ERROR: Unable to parse /srv/oe/dev/org.openembedded.core/meta/recipes-extended/packagegroups/packagegroup-core-basic.bb: Exited with "1"
| ERROR: Command execution failed: Exited with 1


Of course, I can BBMASK out these packagegroup-core recipes or simply
define VIRTUAL-RUNTIME_init_manager.


> Other proposals for solutions are welcome. I thought it better to
> catch a common user misconfiguration than generate broken images
> silently though.

You can put this check into e.g. do_configure[prefuncs].



Enrico

Re: [PATCH] packagegroup: Add init-manager sanity check

[Richard Purdie]

On Thu, 2013-04-18 at 12:39 +0200, Enrico Scholz wrote:
> Richard Purdie <richard.purdie@linuxfoundation.org> writes:
> 
> >> > Currently, you can set VIRTUAL-RUNTIME_init_manager to an init
> >> > system that isn't in DISTRO_FEATURES. This leads to head scratching
> >> > over unbootable images.
> >> 
> >> Because this sanity check is placed into an anonymous function, this
> >> change affects also images which do not not include packagegroup-core*
> >> in their images and are not using VIRTUAL-RUNTIME_init_manager at all.
> >
> > Affects in that it runs the anonymous python fragment but does nothing?
> 
> no; the 'parsing recipes' phase throws an exceptions
> 
> | ERROR: Please ensure that your setting of VIRTUAL-RUNTIME_init_manager (sysvinit) matches the entries enabled in DISTRO_FEATURES##                                                               | ETA:  00:00:14
> | ERROR: Unable to parse /srv/oe/dev/org.openembedded.core/meta/recipes-extended/packagegroups/packagegroup-core-basic.bb: Exited with "1"
> | ERROR: Command execution failed: Exited with 1
> 
> 
> Of course, I can BBMASK out these packagegroup-core recipes or simply
> define VIRTUAL-RUNTIME_init_manager.

Hmm, I understand now.

The way I see this is that the system can't know if anything in your
environment is going to use packagegroup-* or not. Given that, I do
think this is a valid warning since whilst you know not to use them,
others may not.

> > Other proposals for solutions are welcome. I thought it better to
> > catch a common user misconfiguration than generate broken images
> > silently though.
> 
> You can put this check into e.g. do_configure[prefuncs].

So it errors at build time some time later? Where we can is it not
better to inform the user of configuration issues earlier?

Cheers,

Richard

Re: [PATCH] packagegroup: Add init-manager sanity check

[Enrico Scholz]

Richard Purdie <richard.purdie@linuxfoundation.org> writes:

>> no; the 'parsing recipes' phase throws an exceptions
>> 
>> | ERROR: Please ensure that your setting of
>> | VIRTUAL-RUNTIME_init_manager (sysvinit) matches the entries
>> | enabled in DISTRO_FEATURES## | ETA: 00:00:14
>> | ERROR: Unable to parse
>> | /srv/oe/dev/org.openembedded.core/meta/recipes-extended/packagegroups/packagegroup-core-basic.bb:
>> | Exited with "1"
>> | ERROR: Command execution failed: Exited with 1
>> 
>> 
>> Of course, I can BBMASK out these packagegroup-core recipes or simply
>> define VIRTUAL-RUNTIME_init_manager.
>
> Hmm, I understand now.
>
> The way I see this is that the system can't know if anything in your
> environment is going to use packagegroup-* or not.

Exactly; at this stage, the system does not known whether
VIRTUAL-RUNTIME_init_manager will ever be used and should not check it
there hence.


> Given that, I do think this is a valid warning

It's not a warning, but an error stopping the build


>> > Other proposals for solutions are welcome. I thought it better to
>> > catch a common user misconfiguration than generate broken images
>> > silently though.
>> 
>> You can put this check into e.g. do_configure[prefuncs].
>
> So it errors at build time some time later? Where we can is it not
> better to inform the user of configuration issues earlier?

The differrence is that the anonymous python function fails regardless
whether VIRTUAL-RUNTIME_init_manager is used or not (and can not cause
problems hence).

The check in do_configure() will fail only, when it is really used.

I know that the check in the anonymous python function is executed very
early while the do_configure() check might trigger hours later.  I do
not know whether such early warnings are really important; perhaps my
initial

| DEPENDS += "${@some_check(d)}"

suggestion could be enhanced so that some_check() throws an exception
and puts a note like the actual one.


Enrico