[PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes

[PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes

[joe]

From: Joe MacDonald <joe@deserted.net>

A side-effect of making vars_from_file return None for non-recipes is that
PV gets 'None' if you have an included file named <recipe>.inc.  If there
is a recipe with a version number in the name for a .inc file, it's
probably reasonable to calculate PV based on that file, rather than giving
it 'None' (which becomes 1.0 in most cases).

Signed-off-by: Joe MacDonald <joe@deserted.net>
---
 bitbake/lib/bb/parse/__init__.py |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I ran into this when building meta-selinux where there's a chain of includes:

   refpolicy-mls_2.20130424.bb
    -> refpolicy_2.20130424.inc
      -> refpolicy_common.inc

refpolicy_2.20130424.inc sets FILESEXTRAPATH to pick up patches out of the
refpolicy-2.20130424 directory, but during the build PV would be 2.20130424 only
until the .inc file was included, then it became 1.0 and the patches were never
found.


diff --git a/bitbake/lib/bb/parse/__init__.py b/bitbake/lib/bb/parse/__init__.py
index e4a44dd..5157d80 100644
--- a/bitbake/lib/bb/parse/__init__.py
+++ b/bitbake/lib/bb/parse/__init__.py
@@ -127,7 +127,7 @@ def resolve_file(fn, d):
 # Used by OpenEmbedded metadata
 __pkgsplit_cache__={}
 def vars_from_file(mypkg, d):
-    if not mypkg or not mypkg.endswith((".bb", ".bbappend")):
+    if not mypkg or not mypkg.endswith((".bb", ".bbappend", ".inc")):
         return (None, None, None)
     if mypkg in __pkgsplit_cache__:
         return __pkgsplit_cache__[mypkg]
-- 
1.7.10.4

Re: [PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes

[Richard Purdie]

On Mon, 2014-03-31 at 16:44 -0400, joe@deserted.net wrote:
> From: Joe MacDonald <joe@deserted.net>
> 
> A side-effect of making vars_from_file return None for non-recipes is that
> PV gets 'None' if you have an included file named <recipe>.inc.  If there
> is a recipe with a version number in the name for a .inc file, it's
> probably reasonable to calculate PV based on that file, rather than giving
> it 'None' (which becomes 1.0 in most cases).
> 
> Signed-off-by: Joe MacDonald <joe@deserted.net>
> ---
>  bitbake/lib/bb/parse/__init__.py |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I ran into this when building meta-selinux where there's a chain of includes:
> 
>    refpolicy-mls_2.20130424.bb
>     -> refpolicy_2.20130424.inc
>       -> refpolicy_common.inc

The question to ask here is why is PV being expanded during the .inc
file?

When this code was changed, we did find some issues like this but it
usually means something unexpected was happening and "bad" PV values
were actually being used in some cases. It was therefore a conscious
decision not to do this.

If I take this change, you could get PV of "common" being used in the
second .inc file which is nasty.

I'd therefore much rather figure out what is happening to cause the
expansion of PV in the .inc file and see if we can't avoid that.

Looking at the layer, the issue is:

FILESEXTRAPATHS_prepend := "${THISDIR}/refpolicy-${PV}:"

and I'd suggest that line move to the .bb files in this case.

Cheers,

Richard

Re: [PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes

[Joe MacDonald]

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

[Re: [bitbake-devel] [PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes] On 14.04.01 (Tue 09:58) Richard Purdie wrote:

> On Mon, 2014-03-31 at 16:44 -0400, joe@deserted.net wrote:
> > From: Joe MacDonald <joe@deserted.net>
> > 
> > A side-effect of making vars_from_file return None for non-recipes is that
> > PV gets 'None' if you have an included file named <recipe>.inc.  If there
> > is a recipe with a version number in the name for a .inc file, it's
> > probably reasonable to calculate PV based on that file, rather than giving
> > it 'None' (which becomes 1.0 in most cases).
> > 
> > Signed-off-by: Joe MacDonald <joe@deserted.net>
> > ---
> >  bitbake/lib/bb/parse/__init__.py |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > I ran into this when building meta-selinux where there's a chain of includes:
> > 
> >    refpolicy-mls_2.20130424.bb
> >     -> refpolicy_2.20130424.inc
> >       -> refpolicy_common.inc
> 
> The question to ask here is why is PV being expanded during the .inc
> file?
> 
> When this code was changed, we did find some issues like this but it
> usually means something unexpected was happening and "bad" PV values
> were actually being used in some cases. It was therefore a conscious
> decision not to do this.

Oh, okay.  I scanned back through the mailing list traffic from the time
when it went in but didn't see a discussion there, but likely it was a
more interactive thing.

> If I take this change, you could get PV of "common" being used in the
> second .inc file which is nasty.

I don't want to keep using meta-selinux as an example here, (since it
mostly ends up looking bad for meta-selinux, I suppose) but I can
actually see a case where -common would be organizationally desirable.
Or at least not ugly.  recipies-security/selinux has a collection of
core selinux recipes, each has their own patches directory which
potentially co-mingles patches for versioned and git-based recipes or
both.  Right now none of the recipes in there are doing the
<specific> -> <versioned> -> <common> include thing, but it's not a huge
stretch to imagine it being useful.

In other cases that'd be done with a versioned directory name and a
files/ directory, but that would make things less clear, not more in
this scenario.  So having a <recipie>-common directory would be kind of
good.

> I'd therefore much rather figure out what is happening to cause the
> expansion of PV in the .inc file and see if we can't avoid that.
> 
> Looking at the layer, the issue is:
> 
> FILESEXTRAPATHS_prepend := "${THISDIR}/refpolicy-${PV}:"
> 
> and I'd suggest that line move to the .bb files in this case.

That's what I'll do if you're not convinced on this patch, but when I
started down that path yesterday afternoon I got thinking about what I'd
laid out above and that I would be duplicating the exact same line in
three recipes right now (-mcs, -mls and -standard) and adding it to any
other new policies that get submitted.  That really is a common piece
shared among all the variants and it seemed like the right thing to do
was have it only in one place.

-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

Re: [PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes

[Richard Purdie]

On Tue, 2014-04-01 at 09:50 -0400, Joe MacDonald wrote:
> > On Mon, 2014-03-31 at 16:44 -0400, joe@deserted.net wrote:

> I don't want to keep using meta-selinux as an example here, (since it
> mostly ends up looking bad for meta-selinux, I suppose) but I can
> actually see a case where -common would be organizationally desirable.
> Or at least not ugly.  recipies-security/selinux has a collection of
> core selinux recipes, each has their own patches directory which
> potentially co-mingles patches for versioned and git-based recipes or
> both.  Right now none of the recipes in there are doing the
> <specific> -> <versioned> -> <common> include thing, but it's not a huge
> stretch to imagine it being useful.
> 
> In other cases that'd be done with a versioned directory name and a
> files/ directory, but that would make things less clear, not more in
> this scenario.  So having a <recipie>-common directory would be kind of
> good.

Well, you can do that, but you should name it as such in the variable,
not abuse the PV variable like this! :)

> > I'd therefore much rather figure out what is happening to cause the
> > expansion of PV in the .inc file and see if we can't avoid that.
> > 
> > Looking at the layer, the issue is:
> > 
> > FILESEXTRAPATHS_prepend := "${THISDIR}/refpolicy-${PV}:"
> > 
> > and I'd suggest that line move to the .bb files in this case.
> 
> That's what I'll do if you're not convinced on this patch, but when I
> started down that path yesterday afternoon I got thinking about what I'd
> laid out above and that I would be duplicating the exact same line in
> three recipes right now (-mcs, -mls and -standard) and adding it to any
> other new policies that get submitted.  That really is a common piece
> shared among all the variants and it seemed like the right thing to do
> was have it only in one place.

I understand the concern there, you could do this with a "common" name
in the variable instead of PV though.

Cheers,

Richard

Re: [PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes

[Joe MacDonald]

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

[Re: [bitbake-devel] [PATCH] bitbake: parse: allow vars_from_file to consider inc files as recipes] On 14.04.01 (Tue 14:56) Richard Purdie wrote:

> On Tue, 2014-04-01 at 09:50 -0400, Joe MacDonald wrote:
> > > On Mon, 2014-03-31 at 16:44 -0400, joe@deserted.net wrote:
> 
> > I don't want to keep using meta-selinux as an example here, (since it
> > mostly ends up looking bad for meta-selinux, I suppose) but I can
> > actually see a case where -common would be organizationally desirable.
> > Or at least not ugly.  recipies-security/selinux has a collection of
> > core selinux recipes, each has their own patches directory which
> > potentially co-mingles patches for versioned and git-based recipes or
> > both.  Right now none of the recipes in there are doing the
> > <specific> -> <versioned> -> <common> include thing, but it's not a huge
> > stretch to imagine it being useful.
> > 
> > In other cases that'd be done with a versioned directory name and a
> > files/ directory, but that would make things less clear, not more in
> > this scenario.  So having a <recipie>-common directory would be kind of
> > good.
> 
> Well, you can do that, but you should name it as such in the variable,
> not abuse the PV variable like this! :)
> 
> > > I'd therefore much rather figure out what is happening to cause the
> > > expansion of PV in the .inc file and see if we can't avoid that.
> > > 
> > > Looking at the layer, the issue is:
> > > 
> > > FILESEXTRAPATHS_prepend := "${THISDIR}/refpolicy-${PV}:"
> > > 
> > > and I'd suggest that line move to the .bb files in this case.
> > 
> > That's what I'll do if you're not convinced on this patch, but when I
> > started down that path yesterday afternoon I got thinking about what I'd
> > laid out above and that I would be duplicating the exact same line in
> > three recipes right now (-mcs, -mls and -standard) and adding it to any
> > other new policies that get submitted.  That really is a common piece
> > shared among all the variants and it seemed like the right thing to do
> > was have it only in one place.
> 
> I understand the concern there, you could do this with a "common" name
> in the variable instead of PV though.

Sure, but implementing that solution here amounts to having two
variables (well, one for now, but potentially adding a second one down
the road for functionality that would otherwise have been a beneficial
side effect of variable expansion) one of which contains ${PV}.  That
was actually the very first thing I did in tracking this back to the
source, but that really seemed like a wink and a nod and I frankly felt
kind of dumb for doing something that I could already get from ${PV}.

-- 
-Joe MacDonald.
:wq

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]