Re: [OE-core][PATCHv2 1/2] classes/ptest-python-pytest: add a new class to consolidate pytest ptest functionality

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Derek,

On 12/18/24 7:04 PM, Derek Straka wrote:
> V3 to be sent shortly based on inputs from you and Alex.
> 
> On Wed, Dec 18, 2024 at 7:32 AM Quentin Schulz <quentin.schulz@cherry.de>
> wrote:
> 
>> Hi Derek,
>>
> Hi Quentin,
> 
> Thanks for your note.
> 
>>
>> On 12/18/24 12:12 AM, Derek Straka via lists.openembedded.org wrote:
>>> A large number of python packages leverage the pytest unit test
>>> framework for their ptest functionality.  Currently, many of the tests
>>> have duplicate code for:
>>>       1. Installing pytest files
>>>       2. Declaring ptest dependencies
>>>       3. Script for executing tests (run-ptes)
>>>
>>> To simplify adding common pytest based ptests, added a new class
>>> enabling base functionality.  Users can also override the location of
>>> the pytest files in addition to using their own version of run-ptest
>>>
>>> Signed-off-by: Derek Straka <derek@asterius.io>
>>> ---
>>>    .../ptest-python-pytest.bbclass               | 42 +++++++++++++++++++
>>>    meta/files/ptest-python-pytest/run-ptest      |  3 ++
>>>    2 files changed, 45 insertions(+)
>>>    create mode 100644 meta/classes-recipe/ptest-python-pytest.bbclass
>>>    create mode 100755 meta/files/ptest-python-pytest/run-ptest
>>>
>>> diff --git a/meta/classes-recipe/ptest-python-pytest.bbclass
>> b/meta/classes-recipe/ptest-python-pytest.bbclass
>>> new file mode 100644
>>> index 0000000000..89ff10c335
>>> --- /dev/null
>>> +++ b/meta/classes-recipe/ptest-python-pytest.bbclass
>>> @@ -0,0 +1,42 @@
>>> +#
>>> +# Copyright OpenEmbedded Contributors
>>> +#
>>> +# SPDX-License-Identifier: MIT
>>> +#
>>> +
>>> +inherit ptest
>>> +
>>> +FILESEXTRAPATHS:prepend := "${COREBASE}/meta/files:"
>>
>> I think it'd make sense to use :append here as we don't want to override
>> anything, just provide a fallback in case the
>> ptest-python-pytest/run-ptest isn't find anywhere else. This would also
>> allow to easily override this file by just adding the file next to the
>> recipe, instead of having to add a FILESEXTRAPATHS:prepend :=
>> "${THISDIR}/${PN}:" in the recipe after the inherit for example.
>>
> I was able to override in oe-core without adding any prepends in the
> recipes.  My understanding is the EXTRA paths are searched as a last
> resort, so recipes having their own run-ptest get theirs included first.
> Overall, the class was adapted from ptest-perl and ptest-cargo.
>

FILESEXTRAPATHS:prepend is what we use in bbappends to search in the 
directory next to the bbappend **first** and then from the original 
recipe second.

It's a bit convoluted, but FILESPATH stores a list of paths that is 
constructed from FILESEXTRAPATHS + <original-recipe-dir>/${BP} + 
<original-recipe-dir>/${BPN} + <original-recipe-dir>/files (c.f. 
meta/classes-global/base.bbclass line 59, which calls base_set_filespath 
from meta/classes-global/utils.bbclass). FILESPATH is then read from 
left to right and the first path to have the listed file "wins".

Can you please provide the code you used to validate overriding 
run-ptest from the recipe? If that is the case, then I would like to 
understand how that happens and we possibly have hit a corner case we 
should fix (or at least be aware of).

>>> +
>>> +SRC_URI += "file://ptest-python-pytest/run-ptest"
>>> +
>>
>> We use append/prepend everywhere but here, should we?
>>
>> The code was borrowed from ptest-perl.bbclass.  I'll update the python
> class as suggested.
> 
>>> +# Overridable configuration for the directory within the source tree
>>> +# containing the pytest files
>>> +PTEST_PYTEST_DIR ?= "/tests"
>>> +
>>> +do_install_ptest_python_pytest() {
>>> +     if [ ! -f ${D}${PTEST_PATH}/run-ptest ]; then
>>> +             install -m 0755 ${UNPACKDIR}/ptest-python-pytest/run-ptest
>> ${D}${PTEST_PATH}
>>> +     fi
>>
>> This means if we ever built the recipe and don't clean its WORKDIR, but
>> update the file, it will never be updated. I don't think that's what we
>> want. Can you explain what you're trying to prevent here by having a
>> check for NOT installing the file?
>>
>>> +    if [ -d "${S}/${PTEST_PYTEST_DIR}" ]; then
>>
>> Shouldn't we use UNPACKDIR for file:// SRC_URI in master?
>>
> To my knowledge, the pypi recipes aren't putting the sources into UNPACKDIR
> for better or worse.  I looked at several packages, and they're using `S =
> "${WORKDIR}/${PYPI_PACKAGE}-${PV}"` (See pypi.bbclass)
> 

I'm not familiar with UNPACKDIR and its use as I'm still on Scarthgap.

Cheers,
Quentin