Patchwork [CONSOLIDATED,PULL,012/113] base.bbclass: Allow buildstats to be optionally supplied

login
register
mail settings
Submitter Saul Wold
Date Jan. 3, 2012, 6:18 a.m.
Message ID <09b1dc8bd886c8cd2a5d4085d8bb4b73ece1f5b0.1325571069.git.sgw@linux.intel.com>
Download mbox | patch
Permalink /patch/18011/
State New
Headers show

Comments

Saul Wold - Jan. 3, 2012, 6:18 a.m.
From: Mark Hatle <mark.hatle@windriver.com>

Buildstats should be allowed to be optionally enabled.  It's
recommended that it be enabled via the USER_CLASSES setting.

Alternatively it could be enabled via the INHERIT_DISTRO or
similar mechanism.

Signed-off-by: Mark Hatle <mark.hatle@windriver.com>
---
 meta/classes/base.bbclass   |    1 -
 meta/conf/local.conf.sample |    3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)
Phil Blundell - Jan. 3, 2012, 12:14 p.m.
On Mon, 2012-01-02 at 22:18 -0800, Saul Wold wrote:
> From: Mark Hatle <mark.hatle@windriver.com>
> 
> Buildstats should be allowed to be optionally enabled.  It's
> recommended that it be enabled via the USER_CLASSES setting.
> 
> Alternatively it could be enabled via the INHERIT_DISTRO or
> similar mechanism.
> 
> Signed-off-by: Mark Hatle <mark.hatle@windriver.com>

I don't think the short summary of this patch gives a very clear
indication of what it's doing.  The terminology "optionally supplied"
makes it sound as though you're talking about some sort of add-on data
file which can be enabled.  Whereas, what the patch seems actually to be
doing is removing the unconditional inherit of buildstats (i.e. turning
it off for almost everyone who has it on today) and adding a suggestion
in local.conf.sample as to how it might be turned on again.

(That said, I do think the intent of this patch is a good one; it's been
a long-standing source of irritation to me that disabling buildstats is
so awkward at present.)

p.
Mark Hatle - Jan. 3, 2012, 6:06 p.m.
On 1/3/12 6:14 AM, Phil Blundell wrote:
> On Mon, 2012-01-02 at 22:18 -0800, Saul Wold wrote:
>> From: Mark Hatle<mark.hatle@windriver.com>
>>
>> Buildstats should be allowed to be optionally enabled.  It's
>> recommended that it be enabled via the USER_CLASSES setting.
>>
>> Alternatively it could be enabled via the INHERIT_DISTRO or
>> similar mechanism.
>>
>> Signed-off-by: Mark Hatle<mark.hatle@windriver.com>
>
> I don't think the short summary of this patch gives a very clear
> indication of what it's doing.  The terminology "optionally supplied"
> makes it sound as though you're talking about some sort of add-on data
> file which can be enabled.  Whereas, what the patch seems actually to be
> doing is removing the unconditional inherit of buildstats (i.e. turning
> it off for almost everyone who has it on today) and adding a suggestion
> in local.conf.sample as to how it might be turned on again.

Perhaps enabled is a better word then "supplied" in this case?

I didn't comment that it was removing the unconditional inherit as I thought 
that was obvious..

Saul -- can you change the short summary or should I resend it?

> (That said, I do think the intent of this patch is a good one; it's been
> a long-standing source of irritation to me that disabling buildstats is
> so awkward at present.)

This was causing a problem for me as well, thus the solution.  Seemed simply 
enough and beneficial.

--Mark

> p.
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Saul Wold - Jan. 4, 2012, 12:45 a.m.
On 01/03/2012 10:06 AM, Mark Hatle wrote:
> On 1/3/12 6:14 AM, Phil Blundell wrote:
>> On Mon, 2012-01-02 at 22:18 -0800, Saul Wold wrote:
>>> From: Mark Hatle<mark.hatle@windriver.com>
>>>
>>> Buildstats should be allowed to be optionally enabled. It's
>>> recommended that it be enabled via the USER_CLASSES setting.
>>>
>>> Alternatively it could be enabled via the INHERIT_DISTRO or
>>> similar mechanism.
>>>
>>> Signed-off-by: Mark Hatle<mark.hatle@windriver.com>
>>
>> I don't think the short summary of this patch gives a very clear
>> indication of what it's doing. The terminology "optionally supplied"
>> makes it sound as though you're talking about some sort of add-on data
>> file which can be enabled. Whereas, what the patch seems actually to be
>> doing is removing the unconditional inherit of buildstats (i.e. turning
>> it off for almost everyone who has it on today) and adding a suggestion
>> in local.conf.sample as to how it might be turned on again.
>
> Perhaps enabled is a better word then "supplied" in this case?
>
> I didn't comment that it was removing the unconditional inherit as I
> thought that was obvious..
>
> Saul -- can you change the short summary or should I resend it?
>
Already merged by the time this email came in!  Sorry.

Sau!

>> (That said, I do think the intent of this patch is a good one; it's been
>> a long-standing source of irritation to me that disabling buildstats is
>> so awkward at present.)
>
> This was causing a problem for me as well, thus the solution. Seemed
> simply enough and beneficial.
>
> --Mark
>
>> p.
>>
>>
>>
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>

Patch

diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass
index fbcaefb..e65a722 100644
--- a/meta/classes/base.bbclass
+++ b/meta/classes/base.bbclass
@@ -7,7 +7,6 @@  inherit mirrors
 inherit utils
 inherit utility-tasks
 inherit metadata_scm
-inherit buildstats
 inherit logging
 
 OE_IMPORTS += "os sys time oe.path oe.utils oe.data oe.packagegroup"
diff --git a/meta/conf/local.conf.sample b/meta/conf/local.conf.sample
index 73ab9f8..7d52d0e 100644
--- a/meta/conf/local.conf.sample
+++ b/meta/conf/local.conf.sample
@@ -134,12 +134,13 @@  EXTRA_IMAGE_FEATURES = "debug-tweaks"
 # The following is a list of additional classes to use when building images which
 # enable extra features. Some available options which can be included in this variable 
 # are:
+#   - 'buildstats' collect build statistics
 #   - 'image-mklibs' to reduce shared library files size for an image
 #   - 'image-prelink' in order to prelink the filesystem image
 #   - 'image-swab' to perform host system intrusion detection
 # NOTE: if listing mklibs & prelink both, then make sure mklibs is before prelink
 # NOTE: mklibs also needs to be explicitly enabled for a given image, see local.conf.extended
-USER_CLASSES ?= "image-mklibs image-prelink"
+USER_CLASSES ?= "buildstats image-mklibs image-prelink"
 
 #
 # Runtime testing of images