Patchwork rootfs_ipk: respect ONLINE_PACKAGE_MANAGEMENT

login
register
mail settings
Submitter Phil Blundell
Date May 17, 2011, 1:55 p.m.
Message ID <1305640549.2429.226.camel@phil-desktop>
Download mbox | patch
Permalink /patch/4255/
State New, archived
Headers show

Comments

Phil Blundell - May 17, 2011, 1:55 p.m.
This makes it work more or less the same way as the current tip of oe
master, except that I didn't copy over the behaviour for O_P_M="add"
because it seemed slightly bogus to me.  

Signed-off-by: Phil Blundell <philb@gnu.org>
---
 meta/classes/rootfs_ipk.bbclass |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
Richard Purdie - May 17, 2011, 2:24 p.m.
On Tue, 2011-05-17 at 14:55 +0100, Phil Blundell wrote:
> This makes it work more or less the same way as the current tip of oe
> master, except that I didn't copy over the behaviour for O_P_M="add"
> because it seemed slightly bogus to me.  
> 
> Signed-off-by: Phil Blundell <philb@gnu.org>
> ---
>  meta/classes/rootfs_ipk.bbclass |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/classes/rootfs_ipk.bbclass b/meta/classes/rootfs_ipk.bbclass
> index 5727d15..c7c8325 100644
> --- a/meta/classes/rootfs_ipk.bbclass
> +++ b/meta/classes/rootfs_ipk.bbclass
> @@ -69,8 +69,16 @@ fakeroot rootfs_ipk_do_rootfs () {
>  	echo ${BUILDNAME} > ${IMAGE_ROOTFS}/${sysconfdir}/version
>  
>  	${ROOTFS_POSTPROCESS_COMMAND}
> -	
> -	rm -f ${IMAGE_ROOTFS}${opkglibdir}/lists/*
> +
> +	case "${ONLINE_PACKAGE_MANAGEMENT}" in
> +	none)
> + 		rm -rf ${IMAGE_ROOTFS}${opkglibdir}
> +		;;
> +
> +	*)
> +		rm -f ${IMAGE_ROOTFS}${opkglibdir}/lists/*
> +		;;
> +	esac
>  
>  	log_check rootfs 	
>  }

What's wrong with:

ROOTFS_POSTPROCESS_COMMAND += "remove_packaging_data_files ; "

as used in the minimal image? The nice thing about this is it works over
several package backends too...

Cheers,

Richard
Phil Blundell - May 17, 2011, 2:50 p.m.
On Tue, 2011-05-17 at 15:24 +0100, Richard Purdie wrote:
> What's wrong with:
> 
> ROOTFS_POSTPROCESS_COMMAND += "remove_packaging_data_files ; "
> 
> as used in the minimal image? The nice thing about this is it works over
> several package backends too...

Well, conceptually it seems a bit nicer to have the rootfs constructor
obey the same variable that is used to control package management
functionality elsewhere (not that oe-core currently has any "elsewhere",
but oe master does and I would like to backport that functionality to
oe-core as well). 

There's also the minor issue that rootfs_ipk's implementation of
remove_packaging_data_files() does leave an empty directory behind,
which I don't especially want in the O_P_M=none case, so I'd end up
having to do some further cleanup of my own.

I guess I could teach remove_packaging_data_files() to not create the
empty directory if O_P_M=="none".  Would you be happier with that?

p.
Richard Purdie - May 17, 2011, 4:22 p.m.
On Tue, 2011-05-17 at 15:50 +0100, Phil Blundell wrote:
> On Tue, 2011-05-17 at 15:24 +0100, Richard Purdie wrote:
> > What's wrong with:
> > 
> > ROOTFS_POSTPROCESS_COMMAND += "remove_packaging_data_files ; "
> > 
> > as used in the minimal image? The nice thing about this is it works over
> > several package backends too...
> 
> Well, conceptually it seems a bit nicer to have the rootfs constructor
> obey the same variable that is used to control package management
> functionality elsewhere (not that oe-core currently has any "elsewhere",
> but oe master does and I would like to backport that functionality to
> oe-core as well). 
> 
> There's also the minor issue that rootfs_ipk's implementation of
> remove_packaging_data_files() does leave an empty directory behind,
> which I don't especially want in the O_P_M=none case, so I'd end up
> having to do some further cleanup of my own.
> 
> I guess I could teach remove_packaging_data_files() to not create the
> empty directory if O_P_M=="none".  Would you be happier with that?

Mostly. The question remains what happens if there are postinstalls that
are expecting to run on the device. I'm guessing that is why the mkdir
is there since at present we don't want to break the postinstalls.

The answer could be we ensure that directory exists if postinstalls
exist...

Cheers,

Richard
Phil Blundell - May 18, 2011, 3:37 p.m.
On Tue, 2011-05-17 at 17:22 +0100, Richard Purdie wrote:
> On Tue, 2011-05-17 at 15:50 +0100, Phil Blundell wrote:
> > I guess I could teach remove_packaging_data_files() to not create the
> > empty directory if O_P_M=="none".  Would you be happier with that?
> 
> Mostly. The question remains what happens if there are postinstalls that
> are expecting to run on the device. I'm guessing that is why the mkdir
> is there since at present we don't want to break the postinstalls.

The comment says something about ensuring that the lock directory
exists, presumably for future invocations of opkg.  I guess this is to
support O_P_M="add" kind of semantics.  

As far as I know there's no other reason that postinsts should
expect/require the opkg directory to exist.  So I think it should
probably be safe just to delete it if O_P_M="none".

p.
Richard Purdie - May 19, 2011, 10:15 a.m.
On Wed, 2011-05-18 at 16:37 +0100, Phil Blundell wrote:
> On Tue, 2011-05-17 at 17:22 +0100, Richard Purdie wrote:
> > On Tue, 2011-05-17 at 15:50 +0100, Phil Blundell wrote:
> > > I guess I could teach remove_packaging_data_files() to not create the
> > > empty directory if O_P_M=="none".  Would you be happier with that?
> > 
> > Mostly. The question remains what happens if there are postinstalls that
> > are expecting to run on the device. I'm guessing that is why the mkdir
> > is there since at present we don't want to break the postinstalls.
> 
> The comment says something about ensuring that the lock directory
> exists, presumably for future invocations of opkg.  I guess this is to
> support O_P_M="add" kind of semantics.  
> 
> As far as I know there's no other reason that postinsts should
> expect/require the opkg directory to exist.  So I think it should
> probably be safe just to delete it if O_P_M="none".

Let me put this another way:

We have postinstalls that require to run on the target device. If
they're present, we need opkg there to run them, or at least some kind
of script to handle that and I think both cases have a requirement on
that directory (the rpm backend has a suitable script).

So the current patches which just don't create it don't really address
the problem. At the very least the image generation should error if
these types of postinstall are present which I guess is my real concern.

Cheers,

Richard
Phil Blundell - May 19, 2011, 10:31 a.m.
On Thu, 2011-05-19 at 11:15 +0100, Richard Purdie wrote:
> We have postinstalls that require to run on the target device. If
> they're present, we need opkg there to run them, or at least some kind
> of script to handle that and I think both cases have a requirement on
> that directory (the rpm backend has a suitable script).
> 
> So the current patches which just don't create it don't really address
> the problem. At the very least the image generation should error if
> these types of postinstall are present which I guess is my real concern.

Ah, I see what you mean now.  Yes, it is true that packages which expect
to get their postinst run on the target will lose in this case.  But,
practically speaking, this is not likely to be too much of a limitation:
devices which have O_P_M=none tend to be the same ones that have their
root filesystem on readonly media, so running postinsts on the target is
likely to be futile anyway.  (Also, O_P_M=none targets will generally
not install opkg in their rootfs in any case.)

I agree it would be a good thing for the build to emit an error in this
case rather than just causing the postinsts to silently not run.  I
guess the ideal way of dealing with this would be:

a) introduce a new variable, TARGET_FILESYSTEM_READONLY or some such,
and make it an error for there to be any packages left in "install ok
unpacked" at the end of rootfs construction if this flag is set;

b) for the case where O_P_M="none" but T_F_R is not set, provide an
additional postprocessor to distill the opkg status file down to just
the bits that are needed for postinst running, and also provide some
lightweight script to run the postinsts and then clean up afterwards.

p.
Richard Purdie - May 19, 2011, 11:21 a.m.
On Thu, 2011-05-19 at 11:31 +0100, Phil Blundell wrote:
> On Thu, 2011-05-19 at 11:15 +0100, Richard Purdie wrote:
> > We have postinstalls that require to run on the target device. If
> > they're present, we need opkg there to run them, or at least some kind
> > of script to handle that and I think both cases have a requirement on
> > that directory (the rpm backend has a suitable script).
> > 
> > So the current patches which just don't create it don't really address
> > the problem. At the very least the image generation should error if
> > these types of postinstall are present which I guess is my real concern.
> 
> Ah, I see what you mean now.  Yes, it is true that packages which expect
> to get their postinst run on the target will lose in this case.  But,
> practically speaking, this is not likely to be too much of a limitation:
> devices which have O_P_M=none tend to be the same ones that have their
> root filesystem on readonly media, so running postinsts on the target is
> likely to be futile anyway.  (Also, O_P_M=none targets will generally
> not install opkg in their rootfs in any case.)
>
> I agree it would be a good thing for the build to emit an error in this
> case rather than just causing the postinsts to silently not run.  I
> guess the ideal way of dealing with this would be:
> 
> a) introduce a new variable, TARGET_FILESYSTEM_READONLY or some such,
> and make it an error for there to be any packages left in "install ok
> unpacked" at the end of rootfs construction if this flag is set;
> 
> b) for the case where O_P_M="none" but T_F_R is not set, provide an
> additional postprocessor to distill the opkg status file down to just
> the bits that are needed for postinst running, and also provide some
> lightweight script to run the postinsts and then clean up afterwards.

Agreed. I'd like to raise one other issue which is the number of
variables we need to control these things which are effectively binary
flags. I'm leaning towards having a small number of variables containing
flags which trigger certain behaviours. In this case the obvious choices
would be:

IMAGE_FEATURES = "readonly nopackagemanager"

but there are other things which make sense as DISTRO_FEATURES (e.g.
nox11) or MACHINE_FEATURES.

We have these variables and I know Chris has some patches submitted to
improve our handling of such variables. I'm coming down in favour of
extending these into some of these use cases too though.

Thoughts?

Cheers,

Richard
Phil Blundell - May 19, 2011, 11:41 a.m.
On Thu, 2011-05-19 at 12:21 +0100, Richard Purdie wrote:
> On Thu, 2011-05-19 at 11:31 +0100, Phil Blundell wrote:
> Agreed. I'd like to raise one other issue which is the number of
> variables we need to control these things which are effectively binary
> flags. I'm leaning towards having a small number of variables containing
> flags which trigger certain behaviours. In this case the obvious choices
> would be:

That's true, though one particular variable at hand is actually a
trinary.  The behaviour that we have historically had in OE, which seems
to capture most of the significant use-cases, is:

ONLINE_PACKAGE_MANAGEMENT="full" (or not set at all) means to install
opkg or whatever and all its metadata, like oe-core does now

ONLINE_PACKAGE_MANAGEMENT="none" means to not install opkg or its
metadata on the target at all;

ONLINE_PACKAGE_MANAGEMENT="add" means to install just enough package
management infrastructure to allow new packages to be added after the
fact, but not to allow packages that were shipped in the image to be
manipulated.  I think the main use case for this was language packs and
the like.

I'm not entirely sure that O_P_M="add" has ever actually worked properly
(I don't use it myself) but it doesn't seem like an entirely worthless
proposition.

I agree that TARGET_READONLY_ROOTFS or whatever could be done just as
well as some kind of FEATURE.

p.
Richard Purdie - May 19, 2011, 12:16 p.m.
On Thu, 2011-05-19 at 12:41 +0100, Phil Blundell wrote:
> On Thu, 2011-05-19 at 12:21 +0100, Richard Purdie wrote:
> > On Thu, 2011-05-19 at 11:31 +0100, Phil Blundell wrote:
> > Agreed. I'd like to raise one other issue which is the number of
> > variables we need to control these things which are effectively binary
> > flags. I'm leaning towards having a small number of variables containing
> > flags which trigger certain behaviours. In this case the obvious choices
> > would be:
> 
> That's true, though one particular variable at hand is actually a
> trinary.  The behaviour that we have historically had in OE, which seems
> to capture most of the significant use-cases, is:
> 
> ONLINE_PACKAGE_MANAGEMENT="full" (or not set at all) means to install
> opkg or whatever and all its metadata, like oe-core does now
> 
> ONLINE_PACKAGE_MANAGEMENT="none" means to not install opkg or its
> metadata on the target at all;
> 
> ONLINE_PACKAGE_MANAGEMENT="add" means to install just enough package
> management infrastructure to allow new packages to be added after the
> fact, but not to allow packages that were shipped in the image to be
> manipulated.  I think the main use case for this was language packs and
> the like.
> 
> I'm not entirely sure that O_P_M="add" has ever actually worked properly
> (I don't use it myself) but it doesn't seem like an entirely worthless
> proposition.

This could be done as:

IMAGE_FEATURES = "packageaddonly"

with errors if two mutually exclusive flags were specified
("nopackagemanager packageaddonly").


Also, for reference in core-image.bbclass there is already this code:

   ${@base_contains("IMAGE_FEATURES", "package-management", "${ROOTFS_PKGMANAGE}", "${ROOTFS_PKGMANAGE_BOOTSTRAP}",d)} \

which means OE-Core adds the package manager if requested
(ROOTFS_PKGMANAGE) and otherwise installs ROOTFS_PKGMANAGE_BOOTSTRAP
which corresponds to enough scripting to handle the postinstalls.

So if we:

a) Only add ROOTFS_PKGMANAGE_BOOTSTRAP if postinstalls were present
b) Add the read-only-rootfs option we discussed which errors if 
   postinstalls are present

we end up a lot closer to where you want to be.

Cheers,

Richard
Phil Blundell - May 19, 2011, 3:04 p.m.
On Thu, 2011-05-19 at 13:16 +0100, Richard Purdie wrote:
> So if we:
> 
> a) Only add ROOTFS_PKGMANAGE_BOOTSTRAP if postinstalls were present
> b) Add the read-only-rootfs option we discussed which errors if 
>    postinstalls are present
> 
> we end up a lot closer to where you want to be.

Yes, sounds reasonable.  And I think we could then eliminate
remove_packaging_data_files() altogether, in favour of having the right
thing happen automatically during rootfs construction, which would
probably be a good thing too.

p.
Richard Purdie - May 19, 2011, 3:08 p.m.
On Thu, 2011-05-19 at 16:04 +0100, Phil Blundell wrote:
> On Thu, 2011-05-19 at 13:16 +0100, Richard Purdie wrote:
> > So if we:
> > 
> > a) Only add ROOTFS_PKGMANAGE_BOOTSTRAP if postinstalls were present
> > b) Add the read-only-rootfs option we discussed which errors if 
> >    postinstalls are present
> > 
> > we end up a lot closer to where you want to be.
> 
> Yes, sounds reasonable.  And I think we could then eliminate
> remove_packaging_data_files() altogether, in favour of having the right
> thing happen automatically during rootfs construction, which would
> probably be a good thing too.

Agreed, I think we have a plan :)

Cheers,

Richard
Chris Larson - May 19, 2011, 4:58 p.m.
On Thu, May 19, 2011 at 4:21 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> We have these variables and I know Chris has some patches submitted to
> improve our handling of such variables. I'm coming down in favour of
> extending these into some of these use cases too though.

This sounds like a nice idea. The bits I submitted only add support
for package groups as image features, but I always expected us to add
features other than just those in the future. This sounds like a good
candidate for that, indeed.
Phil Blundell - May 24, 2011, 1:59 p.m.
On Thu, 2011-05-19 at 16:08 +0100, Richard Purdie wrote:
> On Thu, 2011-05-19 at 16:04 +0100, Phil Blundell wrote:
> > On Thu, 2011-05-19 at 13:16 +0100, Richard Purdie wrote:
> > > So if we:
> > > 
> > > a) Only add ROOTFS_PKGMANAGE_BOOTSTRAP if postinstalls were present
> > > b) Add the read-only-rootfs option we discussed which errors if 
> > >    postinstalls are present
> > > 
> > > we end up a lot closer to where you want to be.
> > 
> > Yes, sounds reasonable.  And I think we could then eliminate
> > remove_packaging_data_files() altogether, in favour of having the right
> > thing happen automatically during rootfs construction, which would
> > probably be a good thing too.
> 
> Agreed, I think we have a plan :)

One other thing that occurred to me is that ONLINE_PACKAGE_MANAGEMENT
(in classic oe) is a DISTRO feature rather than an image one.  This is
significant because, for example, update-rc.d.bbclass doesn't include
update-rc.d in RDEPENDS if it knows that the package will never be
installed on a running target.  If we're going to make package
management into an IMAGE_FEATURE then obviously this isn't going to work
as it stands.

I guess we could work around it by letting update-rc.d add its
dependency as normal, and then adding code to the rootfs constructor to
stop it taking effect (and/or substitute a dummy update-rc.d package
with no files in) if an image with no package management is being
generated.  That doesn't seem terribly elegant but, short of going back
to a DISTRO-based selection, I can't think of any better way of fixing
it.

p.
Richard Purdie - May 24, 2011, 2:12 p.m.
On Tue, 2011-05-24 at 14:59 +0100, Phil Blundell wrote:
> On Thu, 2011-05-19 at 16:08 +0100, Richard Purdie wrote:
> > On Thu, 2011-05-19 at 16:04 +0100, Phil Blundell wrote:
> > > On Thu, 2011-05-19 at 13:16 +0100, Richard Purdie wrote:
> > > > So if we:
> > > > 
> > > > a) Only add ROOTFS_PKGMANAGE_BOOTSTRAP if postinstalls were present
> > > > b) Add the read-only-rootfs option we discussed which errors if 
> > > >    postinstalls are present
> > > > 
> > > > we end up a lot closer to where you want to be.
> > > 
> > > Yes, sounds reasonable.  And I think we could then eliminate
> > > remove_packaging_data_files() altogether, in favour of having the right
> > > thing happen automatically during rootfs construction, which would
> > > probably be a good thing too.
> > 
> > Agreed, I think we have a plan :)
> 
> One other thing that occurred to me is that ONLINE_PACKAGE_MANAGEMENT
> (in classic oe) is a DISTRO feature rather than an image one.  This is
> significant because, for example, update-rc.d.bbclass doesn't include
> update-rc.d in RDEPENDS if it knows that the package will never be
> installed on a running target.  If we're going to make package
> management into an IMAGE_FEATURE then obviously this isn't going to work
> as it stands.
> 
> I guess we could work around it by letting update-rc.d add its
> dependency as normal, and then adding code to the rootfs constructor to
> stop it taking effect (and/or substitute a dummy update-rc.d package
> with no files in) if an image with no package management is being
> generated.  That doesn't seem terribly elegant but, short of going back
> to a DISTRO-based selection, I can't think of any better way of fixing
> it.

I think allowing selection of this at image generation time is the more
powerful way to handle this. It could be we go through a step of
forcibly removing packages we don't want from the rootfs such as
update-rc.d, or we can tell the package manager to ignore the dependency
which is probably neater.

I have to admit the update-rc.d change was concerning and this does feel
like a better way to handle it.

Cheers,

Richard
Phil Blundell - May 24, 2011, 2:16 p.m.
On Tue, 2011-05-24 at 15:12 +0100, Richard Purdie wrote:
> I think allowing selection of this at image generation time is the more
> powerful way to handle this. It could be we go through a step of
> forcibly removing packages we don't want from the rootfs such as
> update-rc.d, or we can tell the package manager to ignore the dependency
> which is probably neater.
> 
> I have to admit the update-rc.d change was concerning and this does feel
> like a better way to handle it.

Yeah.  The downside to this is that it will require extra
package-manager-specific hackery in each of the rootfs backends, since
there isn't any portable way to either forcibly remove a package or to
get a dependency disregarded.  But I guess I can make it work for ipkg
easily enough and we can worry about the others later.

p.

Patch

diff --git a/meta/classes/rootfs_ipk.bbclass b/meta/classes/rootfs_ipk.bbclass
index 5727d15..c7c8325 100644
--- a/meta/classes/rootfs_ipk.bbclass
+++ b/meta/classes/rootfs_ipk.bbclass
@@ -69,8 +69,16 @@  fakeroot rootfs_ipk_do_rootfs () {
 	echo ${BUILDNAME} > ${IMAGE_ROOTFS}/${sysconfdir}/version
 
 	${ROOTFS_POSTPROCESS_COMMAND}
-	
-	rm -f ${IMAGE_ROOTFS}${opkglibdir}/lists/*
+
+	case "${ONLINE_PACKAGE_MANAGEMENT}" in
+	none)
+ 		rm -rf ${IMAGE_ROOTFS}${opkglibdir}
+		;;
+
+	*)
+		rm -f ${IMAGE_ROOTFS}${opkglibdir}/lists/*
+		;;
+	esac
 
 	log_check rootfs 	
 }