more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Robert P. J. Day]

  more annoying nitpickery ... here:

https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28

it's not clear why someone would go to the trouble of conditionally
including run-ptest in SRC_URI as i would have thought the only
benefit would be to avoid the triviality of copying run-ptest into
WORKDIR, but beyond that, i don't see what the point is, and that's
the only recipe i've run across that bothers to do that.

  is there something more subtle happening here?

rday

Re: more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Robert P. J. Day]

On Mon, 12 Feb 2024, Robert P. J. Day wrote:

>
>   more annoying nitpickery ... here:
>
> https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
>
> it's not clear why someone would go to the trouble of conditionally
> including run-ptest in SRC_URI as i would have thought the only
> benefit would be to avoid the triviality of copying run-ptest into
> WORKDIR, but beyond that, i don't see what the point is, and that's
> the only recipe i've run across that bothers to do that.
>
>   is there something more subtle happening here?

  just for fun, tripped over a number of recipes in meta-openembedded
that do the same thing, like this:

https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-support/re2/re2_2023.03.01.bb#n10
https://git.openembedded.org/meta-openembedded/tree/meta-networking/recipes-protocols/freediameter/freediameter_1.4.0.bb#n21

and more. i'm *guessing* that's not necessary, but it doesn't hurt.

rday

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Alexander Kanavin]

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

Don’t assume intent where mindless copy paste would suffice Robert. :)

Alex

On Mon 12. Feb 2024 at 7.48, Robert P. J. Day <rpjday@crashcourse.ca> wrote:

> On Mon, 12 Feb 2024, Robert P. J. Day wrote:
>
> >
> >   more annoying nitpickery ... here:
> >
> >
> https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> >
> > it's not clear why someone would go to the trouble of conditionally
> > including run-ptest in SRC_URI as i would have thought the only
> > benefit would be to avoid the triviality of copying run-ptest into
> > WORKDIR, but beyond that, i don't see what the point is, and that's
> > the only recipe i've run across that bothers to do that.
> >
> >   is there something more subtle happening here?
>
>   just for fun, tripped over a number of recipes in meta-openembedded
> that do the same thing, like this:
>
>
> https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-support/re2/re2_2023.03.01.bb#n10
>
> https://git.openembedded.org/meta-openembedded/tree/meta-networking/recipes-protocols/freediameter/freediameter_1.4.0.bb#n21
>
> and more. i'm *guessing* that's not necessary, but it doesn't hurt.
>
> rday
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#195301):
> https://lists.openembedded.org/g/openembedded-core/message/195301
> Mute This Topic: https://lists.openembedded.org/mt/104307351/1686489
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

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

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Ross Burton]

On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
>  more annoying nitpickery ... here:
> 
> https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> 
> it's not clear why someone would go to the trouble of conditionally
> including run-ptest in SRC_URI as i would have thought the only
> benefit would be to avoid the triviality of copying run-ptest into
> WORKDIR, but beyond that, i don't see what the point is, and that's
> the only recipe i've run across that bothers to do that.
> 
>  is there something more subtle happening here?

No good reason, and I’d approve a patch that removed the conditionals on the grounds of cleaning up the recipe and ensuring that there’s less conditional paths through the recipe.

Ross

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Robert P. J. Day]

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

On Mon, 12 Feb 2024, Ross Burton wrote:

> On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
> >  more annoying nitpickery ... here:
> >
> > https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> >
> > it's not clear why someone would go to the trouble of conditionally
> > including run-ptest in SRC_URI as i would have thought the only
> > benefit would be to avoid the triviality of copying run-ptest into
> > WORKDIR, but beyond that, i don't see what the point is, and that's
> > the only recipe i've run across that bothers to do that.
> >
> >  is there something more subtle happening here?
>
> No good reason, and I’d approve a patch that removed the
> conditionals on the grounds of cleaning up the recipe and ensuring
> that there’s less conditional paths through the recipe.

  it gets even stranger when you have a conditional to apply a test
patch, like here:

https://git.openembedded.org/meta-openembedded/tree/meta-networking/recipes-protocols/freediameter/freediameter_1.4.0.bb#n21

  i am also assuming that it's cleaner to conditionally check for
ptest assignment for stuff like SRC_URI and DEPENDS, not with:

  ${@bb.utils.contains('DISTRO_FEATURES', 'ptest' ...

but with the more intuitive:

  ${@bb.utils.contains('PTEST_ENABLED', '1', ...

rday

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Alex Kiernan]

On Mon, Feb 12, 2024 at 12:17 PM Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
> On Mon, 12 Feb 2024, Ross Burton wrote:
>
> > On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
> > >  more annoying nitpickery ... here:
> > >
> > > https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> > >
> > > it's not clear why someone would go to the trouble of conditionally
> > > including run-ptest in SRC_URI as i would have thought the only
> > > benefit would be to avoid the triviality of copying run-ptest into
> > > WORKDIR, but beyond that, i don't see what the point is, and that's
> > > the only recipe i've run across that bothers to do that.
> > >
> > >  is there something more subtle happening here?
> >
> > No good reason, and I’d approve a patch that removed the
> > conditionals on the grounds of cleaning up the recipe and ensuring
> > that there’s less conditional paths through the recipe.
>
>   it gets even stranger when you have a conditional to apply a test
> patch, like here:
>
> https://git.openembedded.org/meta-openembedded/tree/meta-networking/recipes-protocols/freediameter/freediameter_1.4.0.bb#n21
>
>   i am also assuming that it's cleaner to conditionally check for
> ptest assignment for stuff like SRC_URI and DEPENDS, not with:
>
>   ${@bb.utils.contains('DISTRO_FEATURES', 'ptest' ...
>
> but with the more intuitive:
>
>   ${@bb.utils.contains('PTEST_ENABLED', '1', ...
>

The PTEST_ENABLED version has the advantage that disabling ptest for a
single recipe is much easier. I guess you could manipulate
DISTRO_FEATURES on a per-recipe basis, but that feels awful.

--
Alex Kiernan

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Robert P. J. Day]

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

On Mon, 12 Feb 2024, Alex Kiernan wrote:

> On Mon, Feb 12, 2024 at 12:17 PM Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> >
> > On Mon, 12 Feb 2024, Ross Burton wrote:
> >
> > > On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
> > > >  more annoying nitpickery ... here:
> > > >
> > > > https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> > > >
> > > > it's not clear why someone would go to the trouble of conditionally
> > > > including run-ptest in SRC_URI as i would have thought the only
> > > > benefit would be to avoid the triviality of copying run-ptest into
> > > > WORKDIR, but beyond that, i don't see what the point is, and that's
> > > > the only recipe i've run across that bothers to do that.
> > > >
> > > >  is there something more subtle happening here?
> > >
> > > No good reason, and I’d approve a patch that removed the
> > > conditionals on the grounds of cleaning up the recipe and ensuring
> > > that there’s less conditional paths through the recipe.
> >
> >   it gets even stranger when you have a conditional to apply a test
> > patch, like here:
> >
> > https://git.openembedded.org/meta-openembedded/tree/meta-networking/recipes-protocols/freediameter/freediameter_1.4.0.bb#n21
> >
> >   i am also assuming that it's cleaner to conditionally check for
> > ptest assignment for stuff like SRC_URI and DEPENDS, not with:
> >
> >   ${@bb.utils.contains('DISTRO_FEATURES', 'ptest' ...
> >
> > but with the more intuitive:
> >
> >   ${@bb.utils.contains('PTEST_ENABLED', '1', ...
> >
>
> The PTEST_ENABLED version has the advantage that disabling ptest for a
> single recipe is much easier. I guess you could manipulate
> DISTRO_FEATURES on a per-recipe basis, but that feels awful.

  so you're confirming that the second form is equivalent, and more
intuitive? i just wanted to make sure (again) that there was no
subtlety i was overlooking.

rday

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Alex Kiernan]

On Mon, Feb 12, 2024 at 2:46 PM Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
> On Mon, 12 Feb 2024, Alex Kiernan wrote:
>
> > On Mon, Feb 12, 2024 at 12:17 PM Robert P. J. Day <rpjday@crashcourse.ca> wrote:
> > >
> > > On Mon, 12 Feb 2024, Ross Burton wrote:
> > >
> > > > On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
> > > > >  more annoying nitpickery ... here:
> > > > >
> > > > > https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> > > > >
> > > > > it's not clear why someone would go to the trouble of conditionally
> > > > > including run-ptest in SRC_URI as i would have thought the only
> > > > > benefit would be to avoid the triviality of copying run-ptest into
> > > > > WORKDIR, but beyond that, i don't see what the point is, and that's
> > > > > the only recipe i've run across that bothers to do that.
> > > > >
> > > > >  is there something more subtle happening here?
> > > >
> > > > No good reason, and I’d approve a patch that removed the
> > > > conditionals on the grounds of cleaning up the recipe and ensuring
> > > > that there’s less conditional paths through the recipe.
> > >
> > >   it gets even stranger when you have a conditional to apply a test
> > > patch, like here:
> > >
> > > https://git.openembedded.org/meta-openembedded/tree/meta-networking/recipes-protocols/freediameter/freediameter_1.4.0.bb#n21
> > >
> > >   i am also assuming that it's cleaner to conditionally check for
> > > ptest assignment for stuff like SRC_URI and DEPENDS, not with:
> > >
> > >   ${@bb.utils.contains('DISTRO_FEATURES', 'ptest' ...
> > >
> > > but with the more intuitive:
> > >
> > >   ${@bb.utils.contains('PTEST_ENABLED', '1', ...
> > >
> >
> > The PTEST_ENABLED version has the advantage that disabling ptest for a
> > single recipe is much easier. I guess you could manipulate
> > DISTRO_FEATURES on a per-recipe basis, but that feels awful.
>
>   so you're confirming that the second form is equivalent, and more
> intuitive? i just wanted to make sure (again) that there was no
> subtlety i was overlooking.
>

PTEST_ENABLED is just set in ptest.bbclass:

PTEST_ENABLED = "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', '1', '0', d)}"
PTEST_ENABLED:class-native = ""
PTEST_ENABLED:class-nativesdk = ""
PTEST_ENABLED:class-cross-canadian = ""

I'd argue checking PTEST_ENABLED is the right way.

-- 
Alex Kiernan

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Ross Burton]

On 12 Feb 2024, at 15:00, Alex Kiernan <alex.kiernan@gmail.com> wrote:
> PTEST_ENABLED is just set in ptest.bbclass:
> 
> PTEST_ENABLED = "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', '1', '0', d)}"
> PTEST_ENABLED:class-native = ""
> PTEST_ENABLED:class-nativesdk = ""
> PTEST_ENABLED:class-cross-canadian = ""
> 
> I'd argue checking PTEST_ENABLED is the right way.

Agreed, I’ve done exactly what you said before: ptest globally is off, but I can turn it on for specific recipes.  Or turn it on globally but then off for specific recipes.

Ross

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Robert P. J. Day]

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


On Mon, 12 Feb 2024, Ross Burton wrote:

> On 12 Feb 2024, at 15:00, Alex Kiernan <alex.kiernan@gmail.com> wrote:
> > PTEST_ENABLED is just set in ptest.bbclass:
> >
> > PTEST_ENABLED = "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', '1', '0', d)}"
> > PTEST_ENABLED:class-native = ""
> > PTEST_ENABLED:class-nativesdk = ""
> > PTEST_ENABLED:class-cross-canadian = ""
> >
> > I'd argue checking PTEST_ENABLED is the right way.
>
> Agreed, I’ve done exactly what you said before: ptest globally is
> off, but I can turn it on for specific recipes.  Or turn it on
> globally but then off for specific recipes.
>
> Ross

  here's just a first pass through oe-core, want to make sure this is
the first stage of cleanup people are talking about. if so, will
submit officially (straightforward replacement):

diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
index 828f618cda..e6240430db 100644
--- a/meta/classes-global/insane.bbclass
+++ b/meta/classes-global/insane.bbclass
@@ -1371,7 +1371,7 @@ python do_qa_patch() {
         return False

     srcdir = d.getVar('S')
-    if not bb.utils.contains('DISTRO_FEATURES', 'ptest', True, False, d):
+    if not bb.utils.contains('PTEST_ENABLED', '1', True, False, d):
         pass
     elif bb.data.inherits_class('ptest', d):
         bb.note("Package %s QA: skipping unimplemented-ptest: ptest implementation detected" % d.getVar('PN'))
diff --git a/meta/classes-recipe/packagegroup.bbclass b/meta/classes-recipe/packagegroup.bbclass
index cf6fc354a8..742fb99953 100644
--- a/meta/classes-recipe/packagegroup.bbclass
+++ b/meta/classes-recipe/packagegroup.bbclass
@@ -32,7 +32,7 @@ python () {
     packages = d.getVar('PACKAGES').split()
     if d.getVar('PACKAGEGROUP_DISABLE_COMPLEMENTARY') != '1':
         types = ['', '-dbg', '-dev']
-        if bb.utils.contains('DISTRO_FEATURES', 'ptest', True, False, d):
+        if bb.utils.contains('PTEST_ENABLED', '1', True, False, d):
             types.append('-ptest')
         packages = [pkg + suffix for pkg in packages
                     for suffix in types]
diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
index 281de4784f..3834b9b9bc 100644
--- a/meta/classes-recipe/testimage.bbclass
+++ b/meta/classes-recipe/testimage.bbclass
@@ -28,7 +28,7 @@ TESTIMAGE_FAILED_QA_ARTIFACTS = "\
     ${sysconfdir}/os-release"

 # If some ptests are run and fail, retrieve corresponding directories
-TESTIMAGE_FAILED_QA_ARTIFACTS += "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', '${libdir}/${MCNAME}/ptest', '', d)}"
+TESTIMAGE_FAILED_QA_ARTIFACTS += "${@bb.utils.contains('PTEST_ENABLED', '1', '${libdir}/${MCNAME}/ptest', '', d)}"

 # You can set (or append to) TEST_SUITES in local.conf to select the tests
 # which you want to run for your target.
diff --git a/meta/recipes-devtools/lua/lua_5.4.6.bb b/meta/recipes-devtools/lua/lua_5.4.6.bb
index eabfc89575..6420b2231b 100644
--- a/meta/recipes-devtools/lua/lua_5.4.6.bb
+++ b/meta/recipes-devtools/lua/lua_5.4.6.bb
@@ -6,7 +6,7 @@ HOMEPAGE = "http://www.lua.org/"

 SRC_URI = "http://www.lua.org/ftp/lua-${PV}.tar.gz;name=tarballsrc \
            file://lua.pc.in \
-           ${@bb.utils.contains('DISTRO_FEATURES', 'ptest', 'http://www.lua.org/tests/lua-${PV_testsuites}-tests.tar.gz;name=tarballtest file://run-ptest ', '', d)} \
+           ${@bb.utils.contains('PTEST_ENABLED', '1', 'http://www.lua.org/tests/lua-${PV_testsuites}-tests.tar.gz;name=tarballtest file://run-ptest ', '', d)} \
            "

 # if no test suite matches PV release of Lua exactly, download the suite for the closest Lua release.
diff --git a/meta/recipes-devtools/perl/perl-ptest.inc b/meta/recipes-devtools/perl/perl-ptest.inc
index e07355d3f5..98d7b1c2a2 100644
--- a/meta/recipes-devtools/perl/perl-ptest.inc
+++ b/meta/recipes-devtools/perl/perl-ptest.inc
@@ -51,7 +51,7 @@ python populate_packages:prepend() {
     # Put all *.t files from the lib dir in the ptest package
     # do_split_packages requires a pair of () in the regex, but we have nothing
     # to match, so use an empty pair.
-    if bb.utils.contains('DISTRO_FEATURES', 'ptest', True, False, d):
+    if bb.utils.contains('PTEST_ENABLED', '1', True, False, d):
         do_split_packages(d, d.expand('${libdir}/perl/${PV}'), r'.*\.t()',
             '${PN}-ptest%s', '%s', recursive=True, match_path=True)
 }
diff --git a/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb b/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb
index 860f291476..f7a184be9a 100644
--- a/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb
+++ b/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb
@@ -4,7 +4,7 @@ LIC_FILES_CHKSUM = "file://LICENSE;md5=8f7bb094c7232b058c7e9f2e431f389c"
 HOMEPAGE = "https://pypi.org/project/bcrypt/"

 DEPENDS += "${PYTHON_PN}-cffi-native"
-LDFLAGS:append = "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', ' -fuse-ld=bfd', '', d)}"
+LDFLAGS:append = "${@bb.utils.contains('PTEST_ENABLED', '1', ' -fuse-ld=bfd', '', d)}"

 SRC_URI[sha256sum] = "33313a1200a3ae90b75587ceac502b048b840fc69e7f7a0905b5f87fac7a1258"

diff --git a/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb b/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb
index 67a197c509..47a873df31 100644
--- a/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb
+++ b/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb
@@ -9,7 +9,7 @@ LIC_FILES_CHKSUM = "file://COPYING;md5=b234ee4d69f5fce4486a80fdaf4a4263 \
                     file://COPYING.DOCS;md5=24ea4c7092233849b4394699333b5c56"

 DEPENDS = " \
-           ${@bb.utils.contains('DISTRO_FEATURES', 'ptest', 'boost', '', d)} \
+           ${@bb.utils.contains('PTEST_ENABLED', '1', 'boost', '', d)} \
         "

 SRC_URI = "https://sourceware.org/pub/valgrind/valgrind-${PV}.tar.bz2 \
diff --git a/meta/recipes-kernel/linux/linux-yocto-dev.bb b/meta/recipes-kernel/linux/linux-yocto-dev.bb
index c91aa073e8..6490a95c67 100644
--- a/meta/recipes-kernel/linux/linux-yocto-dev.bb
+++ b/meta/recipes-kernel/linux/linux-yocto-dev.bb
@@ -60,7 +60,7 @@ KERNEL_FEATURES:append:qemuall=" cfg/virtio.scc features/drm-bochs/drm-bochs.scc
 KERNEL_FEATURES:append:qemux86=" cfg/sound.scc cfg/paravirt_kvm.scc"
 KERNEL_FEATURES:append:qemux86-64=" cfg/sound.scc cfg/paravirt_kvm.scc"
 KERNEL_FEATURES:append = " ${@bb.utils.contains("TUNE_FEATURES", "mx32", " cfg/x32.scc", "", d)}"
-KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/scsi/scsi-debug.scc", "", d)}"
-KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/gpio/mockup.scc", "", d)}"
+KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/scsi/scsi-debug.scc", "", d)}"
+KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/gpio/mockup.scc", "", d)}"

 KERNEL_VERSION_SANITY_SKIP = "1"
diff --git a/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb b/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb
index af8027c51a..441415f9de 100644
--- a/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb
+++ b/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb
@@ -44,5 +44,5 @@ KERNEL_FEATURES:append = " ${KERNEL_EXTRA_FEATURES}"
 KERNEL_FEATURES:append:qemuall=" cfg/virtio.scc features/drm-bochs/drm-bochs.scc"
 KERNEL_FEATURES:append:qemux86=" cfg/sound.scc cfg/paravirt_kvm.scc"
 KERNEL_FEATURES:append:qemux86-64=" cfg/sound.scc cfg/paravirt_kvm.scc"
-KERNEL_FEATURES:append = "${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/scsi/scsi-debug.scc", "", d)}"
-KERNEL_FEATURES:append = "${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/gpio/mockup.scc", "", d)}"
+KERNEL_FEATURES:append = "${@bb.utils.contains("PTEST_ENABLED", "1", " features/scsi/scsi-debug.scc", "", d)}"
+KERNEL_FEATURES:append = "${@bb.utils.contains("PTEST_ENABLED", "1", " features/gpio/mockup.scc", "", d)}"
diff --git a/meta/recipes-kernel/linux/linux-yocto_6.6.bb b/meta/recipes-kernel/linux/linux-yocto_6.6.bb
index 7a47bac057..154cf37bf3 100644
--- a/meta/recipes-kernel/linux/linux-yocto_6.6.bb
+++ b/meta/recipes-kernel/linux/linux-yocto_6.6.bb
@@ -62,8 +62,8 @@ KERNEL_FEATURES:append:qemuall=" cfg/virtio.scc features/drm-bochs/drm-bochs.scc
 KERNEL_FEATURES:append:qemux86=" cfg/sound.scc cfg/paravirt_kvm.scc"
 KERNEL_FEATURES:append:qemux86-64=" cfg/sound.scc cfg/paravirt_kvm.scc"
 KERNEL_FEATURES:append = " ${@bb.utils.contains("TUNE_FEATURES", "mx32", " cfg/x32.scc", "", d)}"
-KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/scsi/scsi-debug.scc", "", d)}"
-KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/gpio/mockup.scc", "", d)}"
+KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/scsi/scsi-debug.scc", "", d)}"
+KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/gpio/mockup.scc", "", d)}"
 KERNEL_FEATURES:append:powerpc =" arch/powerpc/powerpc-debug.scc"
 KERNEL_FEATURES:append:powerpc64 =" arch/powerpc/powerpc-debug.scc"
 KERNEL_FEATURES:append:powerpc64le =" arch/powerpc/powerpc-debug.scc"

rday

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Robert P. J. Day]

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

On Mon, 12 Feb 2024, Ross Burton wrote:

> On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
> >  more annoying nitpickery ... here:
> >
> > https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> >
> > it's not clear why someone would go to the trouble of conditionally
> > including run-ptest in SRC_URI as i would have thought the only
> > benefit would be to avoid the triviality of copying run-ptest into
> > WORKDIR, but beyond that, i don't see what the point is, and that's
> > the only recipe i've run across that bothers to do that.
> >
> >  is there something more subtle happening here?
>
> No good reason, and I’d approve a patch that removed the
> conditionals on the grounds of cleaning up the recipe and ensuring
> that there’s less conditional paths through the recipe.

  additional note in that, if a recipe inherits ptext, then it's silly
to conditionally include "run-ptest" in SRC_URI. but it's trickier if
something else in SRC_URI is included conditionally:

https://git.openembedded.org/openembedded-core/tree/meta/recipes-devtools/flex/flex_2.6.4.bb#n18

without knowing what that patch does, it seems like you kind of have
to leave it as conditional. maybe other examples are more obvious.

rday

RE: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Robert P. J. Day
> Sent: den 13 februari 2024 12:35
> To: Ross Burton <Ross.Burton@arm.com>
> Cc: OE Core mailing list <openembedded-core@lists.openembedded.org>
> Subject: Re: [OE-core] more ptest pedantry: why conditionally include
> "run-ptest" in SRC_URI?
> 
> On Mon, 12 Feb 2024, Ross Burton wrote:
> 
> > On 12 Feb 2024, at 06:39, Robert P. J. Day via lists.openembedded.org
> <rpjday=crashcourse.ca@lists.openembedded.org> wrote:
> > >  more annoying nitpickery ... here:
> > >
> > > https://git.openembedded.org/openembedded-core/tree/meta/recipes-extended/xz/xz_5.4.5.bb#n28
> > >
> > > it's not clear why someone would go to the trouble of conditionally
> > > including run-ptest in SRC_URI as i would have thought the only
> > > benefit would be to avoid the triviality of copying run-ptest into
> > > WORKDIR, but beyond that, i don't see what the point is, and that's
> > > the only recipe i've run across that bothers to do that.
> > >
> > >  is there something more subtle happening here?
> >
> > No good reason, and I’d approve a patch that removed the
> > conditionals on the grounds of cleaning up the recipe and ensuring
> > that there’s less conditional paths through the recipe.
> 
>   additional note in that, if a recipe inherits ptext, then it's silly
> to conditionally include "run-ptest" in SRC_URI. but it's trickier if
> something else in SRC_URI is included conditionally:
> 
> https://git.openembedded.org/openembedded-core/tree/meta/recipes-devtools/flex/flex_2.6.4.bb#n18
> 
> without knowing what that patch does, it seems like you kind of have
> to leave it as conditional. maybe other examples are more obvious.
> 
> rday

Looking at the patch, it states that the reason to apply it when ptest is 
not enabled is to avoid unnecessary dependencies on bison-native and 
flex-native.

//Peter

Re: [OE-core] more ptest pedantry: why conditionally include "run-ptest" in SRC_URI?

[Alex Kiernan]

On Tue, Feb 13, 2024 at 11:21 AM Robert P. J. Day <rpjday@crashcourse.ca> wrote:
>
>
> On Mon, 12 Feb 2024, Ross Burton wrote:
>
> > On 12 Feb 2024, at 15:00, Alex Kiernan <alex.kiernan@gmail.com> wrote:
> > > PTEST_ENABLED is just set in ptest.bbclass:
> > >
> > > PTEST_ENABLED = "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', '1', '0', d)}"
> > > PTEST_ENABLED:class-native = ""
> > > PTEST_ENABLED:class-nativesdk = ""
> > > PTEST_ENABLED:class-cross-canadian = ""
> > >
> > > I'd argue checking PTEST_ENABLED is the right way.
> >
> > Agreed, I’ve done exactly what you said before: ptest globally is
> > off, but I can turn it on for specific recipes.  Or turn it on
> > globally but then off for specific recipes.
> >
> > Ross
>
>   here's just a first pass through oe-core, want to make sure this is
> the first stage of cleanup people are talking about. if so, will
> submit officially (straightforward replacement):
>
> diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
> index 828f618cda..e6240430db 100644
> --- a/meta/classes-global/insane.bbclass
> +++ b/meta/classes-global/insane.bbclass
> @@ -1371,7 +1371,7 @@ python do_qa_patch() {
>          return False
>
>      srcdir = d.getVar('S')
> -    if not bb.utils.contains('DISTRO_FEATURES', 'ptest', True, False, d):
> +    if not bb.utils.contains('PTEST_ENABLED', '1', True, False, d):
>          pass
>      elif bb.data.inherits_class('ptest', d):
>          bb.note("Package %s QA: skipping unimplemented-ptest: ptest implementation detected" % d.getVar('PN'))
> diff --git a/meta/classes-recipe/packagegroup.bbclass b/meta/classes-recipe/packagegroup.bbclass
> index cf6fc354a8..742fb99953 100644
> --- a/meta/classes-recipe/packagegroup.bbclass
> +++ b/meta/classes-recipe/packagegroup.bbclass
> @@ -32,7 +32,7 @@ python () {
>      packages = d.getVar('PACKAGES').split()
>      if d.getVar('PACKAGEGROUP_DISABLE_COMPLEMENTARY') != '1':
>          types = ['', '-dbg', '-dev']
> -        if bb.utils.contains('DISTRO_FEATURES', 'ptest', True, False, d):
> +        if bb.utils.contains('PTEST_ENABLED', '1', True, False, d):
>              types.append('-ptest')
>          packages = [pkg + suffix for pkg in packages
>                      for suffix in types]
> diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
> index 281de4784f..3834b9b9bc 100644
> --- a/meta/classes-recipe/testimage.bbclass
> +++ b/meta/classes-recipe/testimage.bbclass
> @@ -28,7 +28,7 @@ TESTIMAGE_FAILED_QA_ARTIFACTS = "\
>      ${sysconfdir}/os-release"
>
>  # If some ptests are run and fail, retrieve corresponding directories
> -TESTIMAGE_FAILED_QA_ARTIFACTS += "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', '${libdir}/${MCNAME}/ptest', '', d)}"
> +TESTIMAGE_FAILED_QA_ARTIFACTS += "${@bb.utils.contains('PTEST_ENABLED', '1', '${libdir}/${MCNAME}/ptest', '', d)}"
>
>  # You can set (or append to) TEST_SUITES in local.conf to select the tests
>  # which you want to run for your target.

Just reading over the classes, I wonder if this change makes sense for
them, or if the DISTRO_FEATURES dance is more appropriate.

> diff --git a/meta/recipes-devtools/lua/lua_5.4.6.bb b/meta/recipes-devtools/lua/lua_5.4.6.bb
> index eabfc89575..6420b2231b 100644
> --- a/meta/recipes-devtools/lua/lua_5.4.6.bb
> +++ b/meta/recipes-devtools/lua/lua_5.4.6.bb
> @@ -6,7 +6,7 @@ HOMEPAGE = "http://www.lua.org/"
>
>  SRC_URI = "http://www.lua.org/ftp/lua-${PV}.tar.gz;name=tarballsrc \
>             file://lua.pc.in \
> -           ${@bb.utils.contains('DISTRO_FEATURES', 'ptest', 'http://www.lua.org/tests/lua-${PV_testsuites}-tests.tar.gz;name=tarballtest file://run-ptest ', '', d)} \
> +           ${@bb.utils.contains('PTEST_ENABLED', '1', 'http://www.lua.org/tests/lua-${PV_testsuites}-tests.tar.gz;name=tarballtest file://run-ptest ', '', d)} \
>             "
>
>  # if no test suite matches PV release of Lua exactly, download the suite for the closest Lua release.
> diff --git a/meta/recipes-devtools/perl/perl-ptest.inc b/meta/recipes-devtools/perl/perl-ptest.inc
> index e07355d3f5..98d7b1c2a2 100644
> --- a/meta/recipes-devtools/perl/perl-ptest.inc
> +++ b/meta/recipes-devtools/perl/perl-ptest.inc
> @@ -51,7 +51,7 @@ python populate_packages:prepend() {
>      # Put all *.t files from the lib dir in the ptest package
>      # do_split_packages requires a pair of () in the regex, but we have nothing
>      # to match, so use an empty pair.
> -    if bb.utils.contains('DISTRO_FEATURES', 'ptest', True, False, d):
> +    if bb.utils.contains('PTEST_ENABLED', '1', True, False, d):
>          do_split_packages(d, d.expand('${libdir}/perl/${PV}'), r'.*\.t()',
>              '${PN}-ptest%s', '%s', recursive=True, match_path=True)
>  }
> diff --git a/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb b/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb
> index 860f291476..f7a184be9a 100644
> --- a/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb
> +++ b/meta/recipes-devtools/python/python3-bcrypt_4.1.2.bb
> @@ -4,7 +4,7 @@ LIC_FILES_CHKSUM = "file://LICENSE;md5=8f7bb094c7232b058c7e9f2e431f389c"
>  HOMEPAGE = "https://pypi.org/project/bcrypt/"
>
>  DEPENDS += "${PYTHON_PN}-cffi-native"
> -LDFLAGS:append = "${@bb.utils.contains('DISTRO_FEATURES', 'ptest', ' -fuse-ld=bfd', '', d)}"
> +LDFLAGS:append = "${@bb.utils.contains('PTEST_ENABLED', '1', ' -fuse-ld=bfd', '', d)}"
>
>  SRC_URI[sha256sum] = "33313a1200a3ae90b75587ceac502b048b840fc69e7f7a0905b5f87fac7a1258"
>
> diff --git a/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb b/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb
> index 67a197c509..47a873df31 100644
> --- a/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb
> +++ b/meta/recipes-devtools/valgrind/valgrind_3.21.0.bb
> @@ -9,7 +9,7 @@ LIC_FILES_CHKSUM = "file://COPYING;md5=b234ee4d69f5fce4486a80fdaf4a4263 \
>                      file://COPYING.DOCS;md5=24ea4c7092233849b4394699333b5c56"
>
>  DEPENDS = " \
> -           ${@bb.utils.contains('DISTRO_FEATURES', 'ptest', 'boost', '', d)} \
> +           ${@bb.utils.contains('PTEST_ENABLED', '1', 'boost', '', d)} \
>          "
>
>  SRC_URI = "https://sourceware.org/pub/valgrind/valgrind-${PV}.tar.bz2 \
> diff --git a/meta/recipes-kernel/linux/linux-yocto-dev.bb b/meta/recipes-kernel/linux/linux-yocto-dev.bb
> index c91aa073e8..6490a95c67 100644
> --- a/meta/recipes-kernel/linux/linux-yocto-dev.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto-dev.bb
> @@ -60,7 +60,7 @@ KERNEL_FEATURES:append:qemuall=" cfg/virtio.scc features/drm-bochs/drm-bochs.scc
>  KERNEL_FEATURES:append:qemux86=" cfg/sound.scc cfg/paravirt_kvm.scc"
>  KERNEL_FEATURES:append:qemux86-64=" cfg/sound.scc cfg/paravirt_kvm.scc"
>  KERNEL_FEATURES:append = " ${@bb.utils.contains("TUNE_FEATURES", "mx32", " cfg/x32.scc", "", d)}"
> -KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/scsi/scsi-debug.scc", "", d)}"
> -KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/gpio/mockup.scc", "", d)}"
> +KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/scsi/scsi-debug.scc", "", d)}"
> +KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/gpio/mockup.scc", "", d)}"
>
>  KERNEL_VERSION_SANITY_SKIP = "1"
> diff --git a/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb b/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb
> index af8027c51a..441415f9de 100644
> --- a/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto-rt_6.6.bb
> @@ -44,5 +44,5 @@ KERNEL_FEATURES:append = " ${KERNEL_EXTRA_FEATURES}"
>  KERNEL_FEATURES:append:qemuall=" cfg/virtio.scc features/drm-bochs/drm-bochs.scc"
>  KERNEL_FEATURES:append:qemux86=" cfg/sound.scc cfg/paravirt_kvm.scc"
>  KERNEL_FEATURES:append:qemux86-64=" cfg/sound.scc cfg/paravirt_kvm.scc"
> -KERNEL_FEATURES:append = "${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/scsi/scsi-debug.scc", "", d)}"
> -KERNEL_FEATURES:append = "${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/gpio/mockup.scc", "", d)}"
> +KERNEL_FEATURES:append = "${@bb.utils.contains("PTEST_ENABLED", "1", " features/scsi/scsi-debug.scc", "", d)}"
> +KERNEL_FEATURES:append = "${@bb.utils.contains("PTEST_ENABLED", "1", " features/gpio/mockup.scc", "", d)}"
> diff --git a/meta/recipes-kernel/linux/linux-yocto_6.6.bb b/meta/recipes-kernel/linux/linux-yocto_6.6.bb
> index 7a47bac057..154cf37bf3 100644
> --- a/meta/recipes-kernel/linux/linux-yocto_6.6.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto_6.6.bb
> @@ -62,8 +62,8 @@ KERNEL_FEATURES:append:qemuall=" cfg/virtio.scc features/drm-bochs/drm-bochs.scc
>  KERNEL_FEATURES:append:qemux86=" cfg/sound.scc cfg/paravirt_kvm.scc"
>  KERNEL_FEATURES:append:qemux86-64=" cfg/sound.scc cfg/paravirt_kvm.scc"
>  KERNEL_FEATURES:append = " ${@bb.utils.contains("TUNE_FEATURES", "mx32", " cfg/x32.scc", "", d)}"
> -KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/scsi/scsi-debug.scc", "", d)}"
> -KERNEL_FEATURES:append = " ${@bb.utils.contains("DISTRO_FEATURES", "ptest", " features/gpio/mockup.scc", "", d)}"
> +KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/scsi/scsi-debug.scc", "", d)}"
> +KERNEL_FEATURES:append = " ${@bb.utils.contains("PTEST_ENABLED", "1", " features/gpio/mockup.scc", "", d)}"
>  KERNEL_FEATURES:append:powerpc =" arch/powerpc/powerpc-debug.scc"
>  KERNEL_FEATURES:append:powerpc64 =" arch/powerpc/powerpc-debug.scc"
>  KERNEL_FEATURES:append:powerpc64le =" arch/powerpc/powerpc-debug.scc"
>
> rday



--
Alex Kiernan