* [RFC][PATCH 2/2] Don't inherit 'features_check' in recipes that don't utilize it
2020-06-09 14:11 [RFC][PATCH 1/2] features_check: Warn if not used Jacob Kroon
@ 2020-06-09 14:11 ` Jacob Kroon
2020-06-09 14:31 ` ✗ patchtest: failure for "[RFC] features_check: Warn if ..." and 1 more Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Jacob Kroon @ 2020-06-09 14:11 UTC (permalink / raw)
To: openembedded-core
Signed-off-by: Jacob Kroon <jacob.kroon@gmail.com>
---
meta/recipes-bsp/usbutils/usbutils_012.bb | 2 +-
meta/recipes-core/libxml/libxml2_2.9.10.bb | 2 +-
meta/recipes-devtools/mtools/mtools_4.0.24.bb | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/meta/recipes-bsp/usbutils/usbutils_012.bb b/meta/recipes-bsp/usbutils/usbutils_012.bb
index b670fa4ab6..28031e72a7 100644
--- a/meta/recipes-bsp/usbutils/usbutils_012.bb
+++ b/meta/recipes-bsp/usbutils/usbutils_012.bb
@@ -15,7 +15,7 @@ SRC_URI = "${KERNELORG_MIRROR}/linux/utils/usb/usbutils/usbutils-${PV}.tar.gz \
SRC_URI[md5sum] = "7484445cbcf04b3eacac892fe58f8d9f"
SRC_URI[sha256sum] = "ae2e10aad530d95839b6f4d46cd41715eae6f0f1789310d793e9be21b3e7ae20"
-inherit autotools pkgconfig features_check update-alternatives
+inherit autotools pkgconfig update-alternatives
ALTERNATIVE_${PN} = "lsusb"
ALTERNATIVE_PRIORITY = "100"
diff --git a/meta/recipes-core/libxml/libxml2_2.9.10.bb b/meta/recipes-core/libxml/libxml2_2.9.10.bb
index 097aceb2c0..d11b083e8b 100644
--- a/meta/recipes-core/libxml/libxml2_2.9.10.bb
+++ b/meta/recipes-core/libxml/libxml2_2.9.10.bb
@@ -37,7 +37,7 @@ PACKAGECONFIG ??= "python \
PACKAGECONFIG[python] = "--with-python=${PYTHON},--without-python,python3"
PACKAGECONFIG[ipv6] = "--enable-ipv6,--disable-ipv6,"
-inherit autotools pkgconfig binconfig-disabled ptest features_check
+inherit autotools pkgconfig binconfig-disabled ptest
inherit ${@bb.utils.contains('PACKAGECONFIG', 'python', 'python3native', '', d)}
diff --git a/meta/recipes-devtools/mtools/mtools_4.0.24.bb b/meta/recipes-devtools/mtools/mtools_4.0.24.bb
index d7cc72d172..f11cdad37a 100644
--- a/meta/recipes-devtools/mtools/mtools_4.0.24.bb
+++ b/meta/recipes-devtools/mtools/mtools_4.0.24.bb
@@ -35,7 +35,7 @@ SRC_URI = "${GNU_MIRROR}/mtools/mtools-${PV}.tar.bz2 \
SRC_URI_append_class-native = " file://disable-hardcoded-configs.patch"
-inherit autotools texinfo features_check
+inherit autotools texinfo
EXTRA_OECONF = "--without-x"
^ permalink raw reply related [flat|nested] 7+ messages in thread* ✗ patchtest: failure for "[RFC] features_check: Warn if ..." and 1 more
2020-06-09 14:11 [RFC][PATCH 1/2] features_check: Warn if not used Jacob Kroon
2020-06-09 14:11 ` [RFC][PATCH 2/2] Don't inherit 'features_check' in recipes that don't utilize it Jacob Kroon
@ 2020-06-09 14:31 ` Patchwork
2020-06-09 14:34 ` [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used Martin Jansa
2020-06-09 15:44 ` Richard Purdie
3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2020-06-09 14:31 UTC (permalink / raw)
To: Jacob Kroon; +Cc: openembedded-core
== Series Details ==
Series: "[RFC] features_check: Warn if ..." and 1 more
Revision: 1
URL : https://patchwork.openembedded.org/series/24558/
State : failure
== Summary ==
Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:
* Patch [RFC, 2/2] Don't inherit 'features_check' in recipes that don't utilize it
Issue Shortlog does not follow expected format [test_shortlog_format]
Suggested fix Commit shortlog (first line of commit message) should follow the format "<target>: <summary>"
If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).
---
Guidelines: https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used
2020-06-09 14:11 [RFC][PATCH 1/2] features_check: Warn if not used Jacob Kroon
2020-06-09 14:11 ` [RFC][PATCH 2/2] Don't inherit 'features_check' in recipes that don't utilize it Jacob Kroon
2020-06-09 14:31 ` ✗ patchtest: failure for "[RFC] features_check: Warn if ..." and 1 more Patchwork
@ 2020-06-09 14:34 ` Martin Jansa
2020-06-09 15:54 ` Jacob Kroon
2020-06-09 15:44 ` Richard Purdie
3 siblings, 1 reply; 7+ messages in thread
From: Martin Jansa @ 2020-06-09 14:34 UTC (permalink / raw)
To: Jacob Kroon; +Cc: Patches and discussions about the oe-core layer
[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]
Seeing how many cases like this exist(ed), this look very useful, but it
would be even better if we can catch the opposite case when e.g.
REQUIRED_DISTRO_FEATURES are set in recipe, but doesn't do anything because
of missing features_check inherit.
But I understand that such check might be more controversial as it would
need to be checked globally (outside this bbclass), maybe something in
insane.bbclass catching both cases as QA issue?
On Tue, Jun 9, 2020 at 4:11 PM Jacob Kroon <jacob.kroon@gmail.com> wrote:
> Signed-off-by: Jacob Kroon <jacob.kroon@gmail.com>
> ---
> meta/classes/features_check.bbclass | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/meta/classes/features_check.bbclass
> b/meta/classes/features_check.bbclass
> index 4ba827d4ab..31ce600374 100644
> --- a/meta/classes/features_check.bbclass
> +++ b/meta/classes/features_check.bbclass
> @@ -12,11 +12,25 @@
> # Copyright 2019 (C) Texas Instruments Inc.
> # Copyright 2013 (C) O.S. Systems Software LTDA.
>
> +FEATURES_CHECK_INCLUDE_STACK := "${BBINCLUDESTACK}"
> +
> python () {
> if d.getVar('PARSE_ALL_RECIPES', False):
> return
>
> + unused = True
> +
> for kind in ['DISTRO', 'MACHINE', 'COMBINED']:
> + if d.getVar('ANY_OF_' + kind + '_FEATURES') is None and \
> + d.overridedata.get('ANY_OF_' + kind + '_FEATURES') is None and
> \
> + d.getVar('REQUIRED_' + kind + '_FEATURES') is None and \
> + d.overridedata.get('REQUIRED_' + kind + '_FEATURES') is None
> and \
> + d.getVar('CONFLICT_' + kind + '_FEATURES') is None and \
> + d.overridedata.get('CONFLICT_' + kind + '_FEATURES') is None:
> + continue
> +
> + unused = False
> +
> # Assume at least one var is set.
> features = set((d.getVar(kind + '_FEATURES') or '').split())
>
> @@ -39,4 +53,10 @@ python () {
> if conflicts:
> raise bb.parse.SkipRecipe("conflicting %s feature%s '%s'
> (in %s_FEATURES)"
> % (kind.lower(), 's' if len(conflicts) > 1 else '', '
> '.join(conflicts), kind))
> +
> + # Only warn if inherited directly in a .bb-file; if the class is
> inherited
> + # via an .inc/.bbclass it could be a false positive
> + inherited_from_bb =
> d.getVar('FEATURES_CHECK_INCLUDE_STACK').split()[-1].endswith('.bb')
> + if unused and inherited_from_bb:
> + bb.warn("Recipe inherits features_check but doesn't use it")
> }
>
>
[-- Attachment #2: Type: text/html, Size: 3344 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used
2020-06-09 14:34 ` [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used Martin Jansa
@ 2020-06-09 15:54 ` Jacob Kroon
0 siblings, 0 replies; 7+ messages in thread
From: Jacob Kroon @ 2020-06-09 15:54 UTC (permalink / raw)
To: Martin Jansa; +Cc: Patches and discussions about the oe-core layer
On 6/9/20 4:34 PM, Martin Jansa wrote:
> Seeing how many cases like this exist(ed), this look very useful, but it
> would be even better if we can catch the opposite case when e.g.
> REQUIRED_DISTRO_FEATURES are set in recipe, but doesn't do anything
> because of missing features_check inherit.
>
Yes, that would be a good test for insane.
> But I understand that such check might be more controversial as it would
> need to be checked globally (outside this bbclass), maybe something in
> insane.bbclass catching both cases as QA issue?
I didn't want to add this check to insane since I like that you only pay
for the check if you actually use the class.
Not sure if that is worth pushing for, but I think I recall someone
saying that a large part of the parsing time is spent running the
anonyous python(?).
Jacob
>
> On Tue, Jun 9, 2020 at 4:11 PM Jacob Kroon <jacob.kroon@gmail.com
> <mailto:jacob.kroon@gmail.com>> wrote:
>
> Signed-off-by: Jacob Kroon <jacob.kroon@gmail.com
> <mailto:jacob.kroon@gmail.com>>
> ---
> meta/classes/features_check.bbclass | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/meta/classes/features_check.bbclass
> b/meta/classes/features_check.bbclass
> index 4ba827d4ab..31ce600374 100644
> --- a/meta/classes/features_check.bbclass
> +++ b/meta/classes/features_check.bbclass
> @@ -12,11 +12,25 @@
> # Copyright 2019 (C) Texas Instruments Inc.
> # Copyright 2013 (C) O.S. Systems Software LTDA.
>
> +FEATURES_CHECK_INCLUDE_STACK := "${BBINCLUDESTACK}"
> +
> python () {
> if d.getVar('PARSE_ALL_RECIPES', False):
> return
>
> + unused = True
> +
> for kind in ['DISTRO', 'MACHINE', 'COMBINED']:
> + if d.getVar('ANY_OF_' + kind + '_FEATURES') is None and \
> + d.overridedata.get('ANY_OF_' + kind + '_FEATURES') is
> None and \
> + d.getVar('REQUIRED_' + kind + '_FEATURES') is None and \
> + d.overridedata.get('REQUIRED_' + kind + '_FEATURES') is
> None and \
> + d.getVar('CONFLICT_' + kind + '_FEATURES') is None and \
> + d.overridedata.get('CONFLICT_' + kind + '_FEATURES') is
> None:
> + continue
> +
> + unused = False
> +
> # Assume at least one var is set.
> features = set((d.getVar(kind + '_FEATURES') or '').split())
>
> @@ -39,4 +53,10 @@ python () {
> if conflicts:
> raise bb.parse.SkipRecipe("conflicting %s
> feature%s '%s' (in %s_FEATURES)"
> % (kind.lower(), 's' if len(conflicts) > 1
> else '', ' '.join(conflicts), kind))
> +
> + # Only warn if inherited directly in a .bb-file; if the class
> is inherited
> + # via an .inc/.bbclass it could be a false positive
> + inherited_from_bb =
> d.getVar('FEATURES_CHECK_INCLUDE_STACK').split()[-1].endswith('.bb')
> + if unused and inherited_from_bb:
> + bb.warn("Recipe inherits features_check but doesn't use it")
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used
2020-06-09 14:11 [RFC][PATCH 1/2] features_check: Warn if not used Jacob Kroon
` (2 preceding siblings ...)
2020-06-09 14:34 ` [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used Martin Jansa
@ 2020-06-09 15:44 ` Richard Purdie
2020-06-09 16:00 ` Jacob Kroon
3 siblings, 1 reply; 7+ messages in thread
From: Richard Purdie @ 2020-06-09 15:44 UTC (permalink / raw)
To: Jacob Kroon, openembedded-core
On Tue, 2020-06-09 at 16:11 +0200, Jacob Kroon wrote:
> Signed-off-by: Jacob Kroon <jacob.kroon@gmail.com>
> ---
> meta/classes/features_check.bbclass | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/meta/classes/features_check.bbclass
> b/meta/classes/features_check.bbclass
> index 4ba827d4ab..31ce600374 100644
> --- a/meta/classes/features_check.bbclass
> +++ b/meta/classes/features_check.bbclass
> @@ -12,11 +12,25 @@
> # Copyright 2019 (C) Texas Instruments Inc.
> # Copyright 2013 (C) O.S. Systems Software LTDA.
>
> +FEATURES_CHECK_INCLUDE_STACK := "${BBINCLUDESTACK}"
This looks like a rather heavy hit to want to take on every recipe
being parsed everywhere (I also really don't like seeing := needing to
be used).
I'd much rather this were some kind of test we could run periodically,
maybe on the infrastructure rather than the hit being taken all the
time.
Could we add something which just ran these tests? Maybe a class which
the autobuilder could inherit to enable these kinds of tests?
Cheers,
Richard
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [OE-core] [RFC][PATCH 1/2] features_check: Warn if not used
2020-06-09 15:44 ` Richard Purdie
@ 2020-06-09 16:00 ` Jacob Kroon
0 siblings, 0 replies; 7+ messages in thread
From: Jacob Kroon @ 2020-06-09 16:00 UTC (permalink / raw)
To: Richard Purdie, openembedded-core
On 6/9/20 5:44 PM, Richard Purdie wrote:
> On Tue, 2020-06-09 at 16:11 +0200, Jacob Kroon wrote:
>> Signed-off-by: Jacob Kroon <jacob.kroon@gmail.com>
>> ---
>> meta/classes/features_check.bbclass | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/meta/classes/features_check.bbclass
>> b/meta/classes/features_check.bbclass
>> index 4ba827d4ab..31ce600374 100644
>> --- a/meta/classes/features_check.bbclass
>> +++ b/meta/classes/features_check.bbclass
>> @@ -12,11 +12,25 @@
>> # Copyright 2019 (C) Texas Instruments Inc.
>> # Copyright 2013 (C) O.S. Systems Software LTDA.
>>
>> +FEATURES_CHECK_INCLUDE_STACK := "${BBINCLUDESTACK}"
>
> This looks like a rather heavy hit to want to take on every recipe
> being parsed everywhere (I also really don't like seeing := needing to
> be used).
>
Maybe I misunderstood you, but the check is only done in recipes that
inherit the class. If you are referring to the accompanied change in
BitBake for keeping track of the include stack, yeah that might be worse.
The direct assignment is needed in order to suppress false positives,
for instance when an .inc/.bbclass contains the inherit directive. Not
sure how to work around that without this.
> I'd much rather this were some kind of test we could run periodically,
> maybe on the infrastructure rather than the hit being taken all the
> time.
>
> Could we add something which just ran these tests? Maybe a class which
> the autobuilder could inherit to enable these kinds of tests?
>
Maybe that is a better approach. autobuilder could send a summary report
periodically to the ml so that the warnings would be visible.
Jacob
^ permalink raw reply [flat|nested] 7+ messages in thread