[PATCH 1/2] insane: Add test for native/nativesdk inherit order

[PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Tomasz Dziendzielski]

Classes native/nativesdk should be inherited last to prevent unexpected
behaviour.

[YOCTO #5729]

Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
---
 meta/classes/insane.bbclass | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 105d2a5ce8..3597943ddd 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -27,7 +27,7 @@ WARN_QA ?= " libdir xorg-driver-abi \
             infodir build-deps src-uri-bad symlink-to-sysroot multilib \
             invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
             mime mime-xdg unlisted-pkg-lics unhandled-features-check \
-            missing-update-alternatives \
+            missing-update-alternatives native-last \
             "
 ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
             perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
@@ -1366,6 +1366,32 @@ python () {
         d.setVarFlag('do_package_qa', 'rdeptask', '')
     for i in issues:
         package_qa_handle_error("pkgvarcheck", "%s: Variable %s is set as not being package specific, please fix this." % (d.getVar("FILE"), i), d)
+
+    for native_class in ['native', 'nativesdk']:
+        if bb.data.inherits_class(native_class, d):
+
+            inherited_classes = d.getVar('__inherit_cache', False) or []
+            needle = os.path.join('classes', native_class)
+
+            bbclassextend = (d.getVar('BBCLASSEXTEND') or '').split()
+            # BBCLASSEXTEND items are always added in the end
+            skip_classes = bbclassextend
+            if bb.data.inherits_class('native', d) or 'native' in bbclassextend:
+                # native also inherits nopackages and relocatable bbclasses
+                skip_classes.extend(['nopackages', 'relocatable'])
+
+            for class_item in reversed(inherited_classes):
+                if needle not in class_item:
+                    for extend_item in skip_classes:
+                        if os.path.join('classes', '%s.bbclass' % extend_item) in class_item:
+                            break
+                    else:
+                        pn = d.getVar('PN')
+                        package_qa_handle_error("native-last", "%s: native/nativesdk class is not inherited last, this can result in unexpected behaviour. " % pn, d)
+                        break
+                else:
+                    break
+
     qa_sane = d.getVar("QA_SANE")
     if not qa_sane:
         bb.fatal("Fatal QA errors found, failing task.")
-- 
2.29.2

[PATCH 2/2] meta: Fix native inheritance order in recipes

[Tomasz Dziendzielski]

Classes native/nativesdk should be inherited last to prevent unexpected
behaviour.

See [YOCTO #5729] for details.

Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
---
 meta/recipes-core/glibc/cross-localedef-native_2.32.bb | 2 +-
 meta/recipes-devtools/meson/nativesdk-meson_0.56.2.bb  | 2 +-
 meta/recipes-devtools/qemu/qemu-native.inc             | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/meta/recipes-core/glibc/cross-localedef-native_2.32.bb b/meta/recipes-core/glibc/cross-localedef-native_2.32.bb
index 5a0abbb70f..ec59c6ba10 100644
--- a/meta/recipes-core/glibc/cross-localedef-native_2.32.bb
+++ b/meta/recipes-core/glibc/cross-localedef-native_2.32.bb
@@ -14,8 +14,8 @@ require glibc-version.inc
 #
 AUTOTOOLS_SCRIPT_PATH = "${S}/localedef"
 
-inherit native
 inherit autotools
+inherit native
 
 FILESEXTRAPATHS =. "${FILE_DIRNAME}/${PN}:${FILE_DIRNAME}/glibc:"
 
diff --git a/meta/recipes-devtools/meson/nativesdk-meson_0.56.2.bb b/meta/recipes-devtools/meson/nativesdk-meson_0.56.2.bb
index 7fdc4c86d9..8778cab124 100644
--- a/meta/recipes-devtools/meson/nativesdk-meson_0.56.2.bb
+++ b/meta/recipes-devtools/meson/nativesdk-meson_0.56.2.bb
@@ -1,7 +1,7 @@
 include meson.inc
 
-inherit nativesdk
 inherit siteinfo
+inherit nativesdk
 
 SRC_URI += "file://meson-setup.py \
             file://meson-wrapper"
diff --git a/meta/recipes-devtools/qemu/qemu-native.inc b/meta/recipes-devtools/qemu/qemu-native.inc
index aa5c9b9a72..54e49d8bc6 100644
--- a/meta/recipes-devtools/qemu/qemu-native.inc
+++ b/meta/recipes-devtools/qemu/qemu-native.inc
@@ -1,7 +1,7 @@
-inherit native
-
 require qemu.inc
 
+inherit native
+
 EXTRA_OEMAKE_append = " LD='${LD}' AR='${AR}' OBJCOPY='${OBJCOPY}' LDFLAGS='${LDFLAGS}'"
 
 LDFLAGS_append = " -fuse-ld=bfd"
-- 
2.29.2

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Martin Jansa]

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

Would it make sense to show some shortened version of inherited_classes?

The whole variable is too long, e.g. in my case with:
diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 3597943ddd..03481489c3 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -1387,7 +1387,7 @@ python () {
                             break
                     else:
                         pn = d.getVar('PN')
-                        package_qa_handle_error("native-last", "%s:
native/nativesdk class is not inherited last, this can result in unexpected
behaviour. " % pn, d)
+                        package_qa_handle_error("native-last", "%s:
native/nativesdk class is not inherited last, this can result in unexpected
behaviour. Current order: '%s'" % (pn, inherited_classes), d)
                         break
                 else:
                     break

it shows:
WARNING: /jenkins/mjansa/build/ose-github/hardknott/meta-qt5/recipes-qt/qt5/
qtbase-native_git.bb: QA Issue: qtbase-native: native/nativesdk class is
not inherited last, this can result in unexpected behaviour. Current order:
'['/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/image-artifact-names.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/base.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/patch.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/terminal.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/staging.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-webosose/meta-webos/classes/mirrors.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/utils.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/utility-tasks.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/metadata_scm.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/logging.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-virtualization/classes/sanity-meta-virt.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/own-mirrors.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/rm_work.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-updater/classes/sota.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-updater/classes/sota_sanity.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-webosose/meta-webos-raspberrypi/classes/sota_webos-rpi.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/reproducible_build_simple.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/uninative.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/package_ipk.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/package.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/packagedata.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/chrpath.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/package_pkgdata.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/insane.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/buildhistory.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/buildstats.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/image-mklibs.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/linuxloader.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-virtualization/classes/meta-virt-cfg.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-virtualization/classes/meta-virt-k8s-cfg.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-virtualization/classes/meta-virt-xen-cfg.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/debian.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/devshell.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/sstate.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/license.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/buildstats-summary.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-webosose/meta-webos/classes/webos_base.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/remove-libtool.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/image-buildinfo.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/icecc.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/blacklist.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/sanity.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-qt5/classes/qmake5_base.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-qt5/classes/qmake5_paths.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/native.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/relocatable.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/oe-core/meta/classes/nopackages.bbclass',
'/jenkins/mjansa/build/ose-github/hardknott/meta-webosose/meta-webos/classes/webos_qmake5_base.bbclass']'
[native-last]

It would make it a bit easier to find out, what was inheritted after
native/nativesdk as "bitbake -e" doesn't show it very clearly (or I don't
see it there).

Which also points to interesting case caused by
https://github.com/webosose/meta-webosose/blob/master/meta-webos/recipes-qt/qt5/qtbase-native_git.bbappend
which inherits:
https://github.com/webosose/meta-webosose/blob/master/meta-webos/classes/webos_qmake5_base.bbclass
but because it's a bbappend, it's not easy to re-order inherits to make
this QA check happy (the webos_qmake5_base.bbclass should be safe to
inherit after native(sdk) anyway).

So I guess when someone needs to inherit something for native recipe from
bbappend, then the only option is to use .inc file instead of .bbclass or
ignore this warning? Unfortunately INSANE_SKIP doesn't seem to work with
native-last as well. Is this expected?
https://bugzilla.yoctoproject.org/show_bug.cgi?id=5729 doesn't really show
what exact issues this QA check is going to prevent.

Regards,

On Sun, Jan 24, 2021 at 10:53 AM Tomasz Dziendzielski <
tomasz.dziendzielski@gmail.com> wrote:

> Classes native/nativesdk should be inherited last to prevent unexpected
> behaviour.
>
> [YOCTO #5729]
>
> Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
> ---
>  meta/classes/insane.bbclass | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 105d2a5ce8..3597943ddd 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -27,7 +27,7 @@ WARN_QA ?= " libdir xorg-driver-abi \
>              infodir build-deps src-uri-bad symlink-to-sysroot multilib \
>              invalid-packageconfig host-user-contaminated uppercase-pn
> patch-fuzz \
>              mime mime-xdg unlisted-pkg-lics unhandled-features-check \
> -            missing-update-alternatives \
> +            missing-update-alternatives native-last \
>              "
>  ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>              perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
> @@ -1366,6 +1366,32 @@ python () {
>          d.setVarFlag('do_package_qa', 'rdeptask', '')
>      for i in issues:
>          package_qa_handle_error("pkgvarcheck", "%s: Variable %s is set as
> not being package specific, please fix this." % (d.getVar("FILE"), i), d)
> +
> +    for native_class in ['native', 'nativesdk']:
> +        if bb.data.inherits_class(native_class, d):
> +
> +            inherited_classes = d.getVar('__inherit_cache', False) or []
> +            needle = os.path.join('classes', native_class)
> +
> +            bbclassextend = (d.getVar('BBCLASSEXTEND') or '').split()
> +            # BBCLASSEXTEND items are always added in the end
> +            skip_classes = bbclassextend
> +            if bb.data.inherits_class('native', d) or 'native' in
> bbclassextend:
> +                # native also inherits nopackages and relocatable
> bbclasses
> +                skip_classes.extend(['nopackages', 'relocatable'])
> +
> +            for class_item in reversed(inherited_classes):
> +                if needle not in class_item:
> +                    for extend_item in skip_classes:
> +                        if os.path.join('classes', '%s.bbclass' %
> extend_item) in class_item:
> +                            break
> +                    else:
> +                        pn = d.getVar('PN')
> +                        package_qa_handle_error("native-last", "%s:
> native/nativesdk class is not inherited last, this can result in unexpected
> behaviour. " % pn, d)
> +                        break
> +                else:
> +                    break
> +
>      qa_sane = d.getVar("QA_SANE")
>      if not qa_sane:
>          bb.fatal("Fatal QA errors found, failing task.")
> --
> 2.29.2
>
>
> 
>
>

[-- Attachment #2: Type: text/html, Size: 11455 bytes --]

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Tomasz Dziendzielski]

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

>Would it make sense to show some shortened version of inherited_classes?
Yes, it wouldn't be a problem to continue the loop until native/nativesdk
recipe is found and then just print the classes that were found.

>Unfortunately INSANE_SKIP doesn't seem to work with native-last as well.
Is this expected?
It's not expected. I wrongly assumed there is some general mechanism
checking INSANE_SKIP. I will prepare a patch with the correct INSANE_SKIP
handling.

Best regards,
Tomasz Dziendzielski

[-- Attachment #2: Type: text/html, Size: 648 bytes --]

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Tomasz Dziendzielski]

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

Hello Martin,
I have just submitted patches with fix for missing INSANE_SKIP and printing
shortlist of classes inherited after native/nativesdk.
Please check:

https://lists.openembedded.org/g/openembedded-core/message/147370

https://lists.openembedded.org/g/openembedded-core/message/147371

Sorry for inconvenience.

Best regards,
Tomasz Dziendzielski

[-- Attachment #2: Type: text/html, Size: 756 bytes --]

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Konrad Weihmann]

After this patch got merged I notice some "noise" in my builds.

For bbappends which inherit unrelated classes I get a lot of warning like

Issue: nativesdk-openssh: native/nativesdk class is not inherited last, 
this can result in unexpected behaviour. Classes inherited after 
native/nativesdk: my-custom-class.bbclass [native-last]

First it doesn't give any hint that this is caused by the bbappend and
secondly I have no idea how to fix that (if that is even possible).

So I would like to have at least an option to ignore these warnings for 
classes I'm sure don't cause any conflict - something more granular then 
just to deactivate this pretty useful check.

Thoughts?


On 24.01.21 10:55, Tomasz Dziendzielski wrote:
> Classes native/nativesdk should be inherited last to prevent unexpected
> behaviour.
> 
> [YOCTO #5729]
> 
> Signed-off-by: Tomasz Dziendzielski <tomasz.dziendzielski@gmail.com>
> ---
>   meta/classes/insane.bbclass | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 105d2a5ce8..3597943ddd 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -27,7 +27,7 @@ WARN_QA ?= " libdir xorg-driver-abi \
>               infodir build-deps src-uri-bad symlink-to-sysroot multilib \
>               invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
>               mime mime-xdg unlisted-pkg-lics unhandled-features-check \
> -            missing-update-alternatives \
> +            missing-update-alternatives native-last \
>               "
>   ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>               perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
> @@ -1366,6 +1366,32 @@ python () {
>           d.setVarFlag('do_package_qa', 'rdeptask', '')
>       for i in issues:
>           package_qa_handle_error("pkgvarcheck", "%s: Variable %s is set as not being package specific, please fix this." % (d.getVar("FILE"), i), d)
> +
> +    for native_class in ['native', 'nativesdk']:
> +        if bb.data.inherits_class(native_class, d):
> +
> +            inherited_classes = d.getVar('__inherit_cache', False) or []
> +            needle = os.path.join('classes', native_class)
> +
> +            bbclassextend = (d.getVar('BBCLASSEXTEND') or '').split()
> +            # BBCLASSEXTEND items are always added in the end
> +            skip_classes = bbclassextend
> +            if bb.data.inherits_class('native', d) or 'native' in bbclassextend:
> +                # native also inherits nopackages and relocatable bbclasses
> +                skip_classes.extend(['nopackages', 'relocatable'])
> +
> +            for class_item in reversed(inherited_classes):
> +                if needle not in class_item:
> +                    for extend_item in skip_classes:
> +                        if os.path.join('classes', '%s.bbclass' % extend_item) in class_item:
> +                            break
> +                    else:
> +                        pn = d.getVar('PN')
> +                        package_qa_handle_error("native-last", "%s: native/nativesdk class is not inherited last, this can result in unexpected behaviour. " % pn, d)
> +                        break
> +                else:
> +                    break
> +
>       qa_sane = d.getVar("QA_SANE")
>       if not qa_sane:
>           bb.fatal("Fatal QA errors found, failing task.")
> 
> 
> 
> 
> 

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Tomasz Dziendzielski]

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

>After this patch got merged I notice some "noise" in my builds.
>
>For bbappends which inherit unrelated classes I get a lot of warning like
>
>Issue: nativesdk-openssh: native/nativesdk class is not inherited last,
>this can result in unexpected behaviour. Classes inherited after
>native/nativesdk: my-custom-class.bbclass [native-last]
>
>First it doesn't give any hint that this is caused by the bbappend and
>secondly I have no idea how to fix that (if that is even possible).
>
>So I would like to have at least an option to ignore these warnings for
>classes I'm sure don't cause any conflict - something more granular then
>just to deactivate this pretty useful check.
>
>Thoughts?

I think it's a good idea to just ignore specific classes. The diff below
should be enough, with some additional note in the warning message. I'll
check if it works correctly later today.

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 53230fc667..2f53a9be09 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -1381,6 +1381,10 @@ python () {
                     # native also inherits nopackages and relocatable
bbclasses
                     skip_classes.extend(['nopackages', 'relocatable'])

+                extra_skip_classes =
(d.getVar('NATIVE_LAST_IGNORE_CLASSES') or '').split()
+                if extra_skip_classes:
+                    skip_classes.extend(extra_skip_classes)
+
                 broken_order = []
                 for class_item in reversed(inherited_classes):
                     if needle not in class_item:

Best regards,
Tomasz Dziendzielski

[-- Attachment #2: Type: text/html, Size: 2056 bytes --]

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Richard Purdie]

On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
> After this patch got merged I notice some "noise" in my builds.
> 
> For bbappends which inherit unrelated classes I get a lot of warning like
> 
> Issue: nativesdk-openssh: native/nativesdk class is not inherited last, 
> this can result in unexpected behaviour. Classes inherited after 
> native/nativesdk: my-custom-class.bbclass [native-last]
> 
> First it doesn't give any hint that this is caused by the bbappend and
> secondly I have no idea how to fix that (if that is even possible).
> 
> So I would like to have at least an option to ignore these warnings for 
> classes I'm sure don't cause any conflict - something more granular then 
> just to deactivate this pretty useful check.
> 
> Thoughts?

This shouldn't be too common as most recently use BBCLASSEXTEND which
would ensure they're last. Which recipes are you running into this
with?

You really don't want your class coming after native/nativesdk, if you
understand what that code is doing you'll realise it is rather risky :(

You could turn off that warning for a set of recipes if you really
don't care but I'd prefer not to make that too easy.

Cheers,

Richard

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Konrad Weihmann]

On 11.02.21 12:43, Richard Purdie wrote:
> On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
>> After this patch got merged I notice some "noise" in my builds.
>>
>> For bbappends which inherit unrelated classes I get a lot of warning like
>>
>> Issue: nativesdk-openssh: native/nativesdk class is not inherited last,
>> this can result in unexpected behaviour. Classes inherited after
>> native/nativesdk: my-custom-class.bbclass [native-last]
>>
>> First it doesn't give any hint that this is caused by the bbappend and
>> secondly I have no idea how to fix that (if that is even possible).
>>
>> So I would like to have at least an option to ignore these warnings for
>> classes I'm sure don't cause any conflict - something more granular then
>> just to deactivate this pretty useful check.
>>
>> Thoughts?
> 
> This shouldn't be too common as most recently use BBCLASSEXTEND which
> would ensure they're last. Which recipes are you running into this
> with?

Sure, it does only affect recipes which are manually inheriting native 
(I got a bunch of them, that's why I noticed).
A (unfortunately) common pattern I face is that ppl share variables 
between recipes using bbclasses - which to my understanding shouldn't 
cause any issues when being inherited after native.
And the same pattern is applied to recipes only being appended.

> 
> You really don't want your class coming after native/nativesdk, if you
> understand what that code is doing you'll realise it is rather risky :(

Totally, that why I'm absolutely in favor of this patch.

> 
> You could turn off that warning for a set of recipes if you really
> don't care but I'd prefer not to make that too easy.

But that would turn off the whole feature, which is bad IMO.
What I see (once that will be used by a wider audience in the wild) is 
that this will lead to questions (consuming my time as an integrator) or 
to the fact that this check will be turned off completely.
Either way not the best choice if you'd ask me.

That's why I asked for a more granular configuration, so that a person 
who's able to understand the implications of a bbclass in relation to 
native could turn it off on a distro/site config level.

And just to be clear I wouldn't want that to be used widely, or even 
being promoted.

If that's not what the project wants, I'm also fine with it, but then it 
would be really helpful to point to the bbappend as a culprit and not 
the base recipe

> 
> Cheers,
> 
> Richard
> 

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Richard Purdie]

On Thu, 2021-02-11 at 13:10 +0100, Konrad Weihmann wrote:
> 
> On 11.02.21 12:43, Richard Purdie wrote:
> > On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
> > > After this patch got merged I notice some "noise" in my builds.
> > > 
> > > For bbappends which inherit unrelated classes I get a lot of warning like
> > > 
> > > Issue: nativesdk-openssh: native/nativesdk class is not inherited last,
> > > this can result in unexpected behaviour. Classes inherited after
> > > native/nativesdk: my-custom-class.bbclass [native-last]
> > > 
> > > First it doesn't give any hint that this is caused by the bbappend and
> > > secondly I have no idea how to fix that (if that is even possible).
> > > 
> > > So I would like to have at least an option to ignore these warnings for
> > > classes I'm sure don't cause any conflict - something more granular then
> > > just to deactivate this pretty useful check.
> > > 
> > > Thoughts?
> > 
> > This shouldn't be too common as most recently use BBCLASSEXTEND which
> > would ensure they're last. Which recipes are you running into this
> > with?
> 
> Sure, it does only affect recipes which are manually inheriting native 
> (I got a bunch of them, that's why I noticed).

Any reason they don't/can't use BBCLASSEXTEND?

> A (unfortunately) common pattern I face is that ppl share variables 
> between recipes using bbclasses - which to my understanding shouldn't 
> cause any issues when being inherited after native.
> And the same pattern is applied to recipes only being appended.

You could use conf files for variables. That is something I'd like to
try and encourage more of, I've been meaning to look at that for the
core too.

> > You really don't want your class coming after native/nativesdk, if you
> > understand what that code is doing you'll realise it is rather risky :(
> 
> Totally, that why I'm absolutely in favor of this patch.
> 
> > 
> > You could turn off that warning for a set of recipes if you really
> > don't care but I'd prefer not to make that too easy.
> 
> But that would turn off the whole feature, which is bad IMO.
> What I see (once that will be used by a wider audience in the wild) is 
> that this will lead to questions (consuming my time as an integrator) or 
> to the fact that this check will be turned off completely.
> Either way not the best choice if you'd ask me.

I'm hoping that people might fix the underlying issues.

> That's why I asked for a more granular configuration, so that a person 
> who's able to understand the implications of a bbclass in relation to 
> native could turn it off on a distro/site config level.
> 
> And just to be clear I wouldn't want that to be used widely, or even 
> being promoted.

I can guarantee that making any such option available will just have
people reach for it and use it. You clearly want to here when the
changes needed to avoid it are probably not that difficult.

> If that's not what the project wants, I'm also fine with it, but then it 
> would be really helpful to point to the bbappend as a culprit and not 
> the base recipe

I understand why you'd want it, I'm just not sure if would encourage
the right behaviour :(.

Cheers,

Richard

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Konrad Weihmann]

On 11.02.21 14:04, Richard Purdie wrote:
> On Thu, 2021-02-11 at 13:10 +0100, Konrad Weihmann wrote:
>>
>> On 11.02.21 12:43, Richard Purdie wrote:
>>> On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
>>>> After this patch got merged I notice some "noise" in my builds.
>>>>
>>>> For bbappends which inherit unrelated classes I get a lot of warning like
>>>>
>>>> Issue: nativesdk-openssh: native/nativesdk class is not inherited last,
>>>> this can result in unexpected behaviour. Classes inherited after
>>>> native/nativesdk: my-custom-class.bbclass [native-last]
>>>>
>>>> First it doesn't give any hint that this is caused by the bbappend and
>>>> secondly I have no idea how to fix that (if that is even possible).
>>>>
>>>> So I would like to have at least an option to ignore these warnings for
>>>> classes I'm sure don't cause any conflict - something more granular then
>>>> just to deactivate this pretty useful check.
>>>>
>>>> Thoughts?
>>>
>>> This shouldn't be too common as most recently use BBCLASSEXTEND which
>>> would ensure they're last. Which recipes are you running into this
>>> with?
>>
>> Sure, it does only affect recipes which are manually inheriting native
>> (I got a bunch of them, that's why I noticed).
> 
> Any reason they don't/can't use BBCLASSEXTEND?
> 
>> A (unfortunately) common pattern I face is that ppl share variables
>> between recipes using bbclasses - which to my understanding shouldn't
>> cause any issues when being inherited after native.
>> And the same pattern is applied to recipes only being appended.
> 
> You could use conf files for variables. That is something I'd like to
> try and encourage more of, I've been meaning to look at that for the
> core too.
> 
>>> You really don't want your class coming after native/nativesdk, if you
>>> understand what that code is doing you'll realise it is rather risky :(
>>
>> Totally, that why I'm absolutely in favor of this patch.
>>
>>>
>>> You could turn off that warning for a set of recipes if you really
>>> don't care but I'd prefer not to make that too easy.
>>
>> But that would turn off the whole feature, which is bad IMO.
>> What I see (once that will be used by a wider audience in the wild) is
>> that this will lead to questions (consuming my time as an integrator) or
>> to the fact that this check will be turned off completely.
>> Either way not the best choice if you'd ask me.
> 
> I'm hoping that people might fix the underlying issues.
> 
>> That's why I asked for a more granular configuration, so that a person
>> who's able to understand the implications of a bbclass in relation to
>> native could turn it off on a distro/site config level.
>>
>> And just to be clear I wouldn't want that to be used widely, or even
>> being promoted.
> 
> I can guarantee that making any such option available will just have
> people reach for it and use it. You clearly want to here when the
> changes needed to avoid it are probably not that difficult.
> 
>> If that's not what the project wants, I'm also fine with it, but then it
>> would be really helpful to point to the bbappend as a culprit and not
>> the base recipe
> 
> I understand why you'd want it, I'm just not sure if would encourage
> the right behaviour :(.

Fair enough - guess I will go with the educating ppl (including myself) 
path then

> 
> Cheers,
> 
> Richard
> 

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Richard Purdie]

On Thu, 2021-02-11 at 15:47 +0100, Konrad Weihmann wrote:
> 
> On 11.02.21 14:04, Richard Purdie wrote:
> > On Thu, 2021-02-11 at 13:10 +0100, Konrad Weihmann wrote:
> > > 
> > > On 11.02.21 12:43, Richard Purdie wrote:
> > > > On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
> > > > > After this patch got merged I notice some "noise" in my builds.
> > 
> > I can guarantee that making any such option available will just have
> > people reach for it and use it. You clearly want to here when the
> > changes needed to avoid it are probably not that difficult.
> > 
> > > If that's not what the project wants, I'm also fine with it, but then it
> > > would be really helpful to point to the bbappend as a culprit and not
> > > the base recipe
> > 
> > I understand why you'd want it, I'm just not sure if would encourage
> > the right behaviour :(.
> 
> Fair enough - guess I will go with the educating ppl (including myself) 
> path then

One thing to remember is that you can change ERROR_QA and WARN_QA on a
per recipe basis, so in theory you can disable this per recipe, or
reduce it to a warning...

Its not neat code to do it, but it isn't really meant to be!

Cheers,

Richard

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Konrad Weihmann]

On 11.02.21 15:59, Richard Purdie wrote:
> On Thu, 2021-02-11 at 15:47 +0100, Konrad Weihmann wrote:
>>
>> On 11.02.21 14:04, Richard Purdie wrote:
>>> On Thu, 2021-02-11 at 13:10 +0100, Konrad Weihmann wrote:
>>>>
>>>> On 11.02.21 12:43, Richard Purdie wrote:
>>>>> On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
>>>>>> After this patch got merged I notice some "noise" in my builds.
>>>
>>> I can guarantee that making any such option available will just have
>>> people reach for it and use it. You clearly want to here when the
>>> changes needed to avoid it are probably not that difficult.
>>>
>>>> If that's not what the project wants, I'm also fine with it, but then it
>>>> would be really helpful to point to the bbappend as a culprit and not
>>>> the base recipe
>>>
>>> I understand why you'd want it, I'm just not sure if would encourage
>>> the right behaviour :(.
>>
>> Fair enough - guess I will go with the educating ppl (including myself)
>> path then
> 
> One thing to remember is that you can change ERROR_QA and WARN_QA on a
> per recipe basis, so in theory you can disable this per recipe, or
> reduce it to a warning...
> 
> Its not neat code to do it, but it isn't really meant to be!

Yeah, unfortunately all the CI tooling I use, will even reflect that 
back to the user - nvm I will figure out something

> 
> Cheers,
> 
> Richard
> 

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Peter Kjellerstedt]

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

Since the order of inheriting native/nativesdk is important, rather than adding warnings about when it is done in the wrong order, which conflicts with bbappends that want to add inherit statements for native recipes, wouldn’t it make sense to instead handle this properly in the code so that native/nativesdk are always inherited last. An idea to solve this would be to rename native.bbclass and nativesdk.bbclass to, e.g., native-internal.bbclass and nativesdk-internal.bbclass, and then make the original classes only set a special variable (similar to BBCLASSEXTEND), and then later when it is appropriate check this variable and import the native-internal/nativesdk-internal class.

//Peter

From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Tomasz Dziendzielski
Sent: den 11 februari 2021 11:30
To: Konrad Weihmann <kweihmann@outlook.com>
Cc: openembedded-core <openembedded-core@lists.openembedded.org>
Subject: Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

>After this patch got merged I notice some "noise" in my builds.
>
>For bbappends which inherit unrelated classes I get a lot of warning like
>
>Issue: nativesdk-openssh: native/nativesdk class is not inherited last,
>this can result in unexpected behaviour. Classes inherited after
>native/nativesdk: my-custom-class.bbclass [native-last]
>
>First it doesn't give any hint that this is caused by the bbappend and
>secondly I have no idea how to fix that (if that is even possible).
>
>So I would like to have at least an option to ignore these warnings for
>classes I'm sure don't cause any conflict - something more granular then
>just to deactivate this pretty useful check.
>
>Thoughts?

I think it's a good idea to just ignore specific classes. The diff below should be enough, with some additional note in the warning message. I'll check if it works correctly later today.

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 53230fc667..2f53a9be09 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -1381,6 +1381,10 @@ python () {
                     # native also inherits nopackages and relocatable bbclasses
                     skip_classes.extend(['nopackages', 'relocatable'])

+                extra_skip_classes = (d.getVar('NATIVE_LAST_IGNORE_CLASSES') or '').split()
+                if extra_skip_classes:
+                    skip_classes.extend(extra_skip_classes)
+
                 broken_order = []
                 for class_item in reversed(inherited_classes):
                     if needle not in class_item:

Best regards,
Tomasz Dziendzielski

[-- Attachment #2: Type: text/html, Size: 6143 bytes --]

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Richard Purdie]

On Fri, 2021-02-12 at 18:26 +0000, Peter Kjellerstedt wrote:
> Since the order of inheriting native/nativesdk is important, rather
> than adding warnings about when it is done in the wrong order, which
> conflicts with bbappends that want to add inherit statements for
> native recipes, wouldn’t it make sense to instead handle this
> properly in the code so that native/nativesdk are always inherited
> last. An idea to solve this would be to rename native.bbclass and
> nativesdk.bbclass to, e.g., native-internal.bbclass and nativesdk-
> internal.bbclass, and then make the original classes only set a
> special variable (similar to BBCLASSEXTEND), and then later when it
> is appropriate check this variable and import the native-
> internal/nativesdk-internal class.

We have code to do this properly, it's called BBCLASSEXTEND. It
automatically adds the inherit at the end of the recipe.

I will also state publicly and clearly than any proposal that involves
"-internal" renaming of class files or variables is unlikely to be
accepted by me, I have a strong dislike of even the idea of them.

Cheers,

Richard

Re: [OE-core] [PATCH 1/2] insane: Add test for native/nativesdk inherit order

[Konrad Weihmann]

Another point where I encounter this warning is when using devtool, as 
externalsrc is injected via bbappend.
After browsing through the code here [1] I was wondering if all that is 
like it is supposed to be - if I understand the code correctly [2] the 
bbappends should be applied before the BBCLASSEXTEND will be computed, 
so the inherit order should be correct - just want to be sure that there 
isn't something more subtle hidden in all that

[1] 
http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/bitbake/lib/bb/parse/ast.py#n387
[2] 
http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/bitbake/lib/bb/parse/ast.py#n393


On 11.02.21 16:12, Konrad Weihmann wrote:
> 
> 
> On 11.02.21 15:59, Richard Purdie wrote:
>> On Thu, 2021-02-11 at 15:47 +0100, Konrad Weihmann wrote:
>>>
>>> On 11.02.21 14:04, Richard Purdie wrote:
>>>> On Thu, 2021-02-11 at 13:10 +0100, Konrad Weihmann wrote:
>>>>>
>>>>> On 11.02.21 12:43, Richard Purdie wrote:
>>>>>> On Thu, 2021-02-11 at 08:21 +0100, Konrad Weihmann wrote:
>>>>>>> After this patch got merged I notice some "noise" in my builds.
>>>>
>>>> I can guarantee that making any such option available will just have
>>>> people reach for it and use it. You clearly want to here when the
>>>> changes needed to avoid it are probably not that difficult.
>>>>
>>>>> If that's not what the project wants, I'm also fine with it, but 
>>>>> then it
>>>>> would be really helpful to point to the bbappend as a culprit and not
>>>>> the base recipe
>>>>
>>>> I understand why you'd want it, I'm just not sure if would encourage
>>>> the right behaviour :(.
>>>
>>> Fair enough - guess I will go with the educating ppl (including myself)
>>> path then
>>
>> One thing to remember is that you can change ERROR_QA and WARN_QA on a
>> per recipe basis, so in theory you can disable this per recipe, or
>> reduce it to a warning...
>>
>> Its not neat code to do it, but it isn't really meant to be!
> 
> Yeah, unfortunately all the CI tooling I use, will even reflect that 
> back to the user - nvm I will figure out something
> 
>>
>> Cheers,
>>
>> Richard
>>
> 
> 
> 
>