Re: [docs] [yocto-docs][PATCH v3] ref-manual: classes: fix bin_package description

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

Hi Antonin,

On 11/27/24 2:37 PM, Antonin Godard wrote:
> Hi Quentin,
> 
> On Wed Nov 27, 2024 at 1:36 PM CET, Quentin Schulz via lists.yoctoproject.org wrote:
>> Hi Antonin,
>>
>> On 11/27/24 12:36 PM, Antonin Godard wrote:
>>> The previous bin_package description was confusing: it would instruct to
>>> use the git fetcher to extract the content of an RPM package using the
>>> `subpath` option - but that's not possible as the git fetcher can be
>>> used to clone a repository but not to do the extraction.
>>>
>>> Update the description by telling what it really does and what it
>>> doesn't do, and by giving an HTTPS+RPM example.
>>>
>>> Signed-off-by: Antonin Godard <antonin.godard@bootlin.com>
>>> ---
>>> Changes in v3:
>>> - Actually explain what bin_package does and what it doesn't do.
>>> - Link to v2: https://lore.kernel.org/r/20241120-fix-bin-package-v2-1-917a5c2745d2@bootlin.com
>>>
>>> Changes in v2:
>>> - Instead of updating the example, update the description of the class
>>>     with a more common (and working) example usage of the class.
>>> - Link to v1: https://lore.kernel.org/r/20241118-fix-bin-package-v1-1-906f0148fdaa@bootlin.com
>>> ---
>>>    documentation/ref-manual/classes.rst | 42 +++++++++++++++++++++---------------
>>>    1 file changed, 25 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/documentation/ref-manual/classes.rst b/documentation/ref-manual/classes.rst
>>> index b92f4e4f20ea8f702c90f4e3d29251b2461d07d0..ff9979d1f1ac0afb0ee59fcd193dec2c1323e1fe 100644
>>> --- a/documentation/ref-manual/classes.rst
>>> +++ b/documentation/ref-manual/classes.rst
>>> @@ -159,27 +159,35 @@ software that includes bash-completion data.
>>>    ``bin_package``
>>>    ===============
>>>    
>>> -The :ref:`ref-classes-bin-package` class is a helper class for recipes that extract the
>>> -contents of a binary package (e.g. an RPM) and install those contents
>>> -rather than building the binary from source. The binary package is
>>> -extracted and new packages in the configured output package format are
>>> -created. Extraction and installation of proprietary binaries is a good
>>> -example use for this class.
>>> +The :ref:`ref-classes-bin-package` class is a helper class for recipes that
>>
>> Would put a comma after recipes so people don't think it's for recipes
>> that extract the contents[...] but rather that the class does it for the
>> recipes inheriting it.
> 
> +1
> 
>>> +disables the :ref:`ref-tasks-configure` and :ref:`ref-tasks-compile` tasks and
>>> +copies the content of the :term:`S` directory into the :term:`D` directory. This
>>> +is useful for installing binary packages (e.g. RPM packages) by passing the
>>> +package in the :term:`SRC_URI` variable and inheriting this class.
>>>    
>>
>> I would also provide tarballs as another example of "binary packages",
>> not sure how to word this though.
> 
> I'll add another example afterwards.
> 
>>> -.. note::
>>> +For RPMs and other packages that do not contain a subdirectory, you should set
>>> +the :term:`SRC_URI` option ``subdir`` to :term:`BP` so that the contents are
>>> +extracted to the directory expected by the default value of :term:`S`. For
>>> +example::
>>> +
>>> +   SRC_URI = "https://example.com/downloads/somepackage.rpm;subdir=${BP}"
>>>    
>>> -   For RPMs and other packages that do not contain a subdirectory, you
>>> -   should specify an appropriate fetcher parameter to point to the
>>> -   subdirectory. For example, if BitBake is using the Git fetcher (``git://``),
>>> -   the "subpath" parameter limits the checkout to a specific subpath
>>> -   of the tree. Here is an example where ``${BP}`` is used so that the files
>>> -   are extracted into the subdirectory expected by the default value of
>>> -   :term:`S`::
>>> +This class assumes that the content of :term:`S` already contains the
>>
>> """
>> This class assumes that the content of the package as installed in
>> :term:`S` mirrors the expected layout once installed on the target,
>> which is generally[...]
>> """
>>
>> ?
> 
> I agree, better wording
> 
>>> +installation paths on the target , which is generally the case for binary
>>
>> Spurious whitespace before comma.
> 
> +1
> 
>>> +packages. For example, a RPM package containing a library should already contain
>>
>> s/a RPM/an RPM/
>>
>> s/containing a library/for a library/ # to avoid redundant "contain" verb
>>
>> s/should already contain/would usually contain/
> 
> +1
> 
>>> +the ``usr/lib`` directories, and should be extracted as
>>
>> s/as/to/?
> 
> +1
> 
>>> +``${S}/usr/lib/<library>.so`` to be installed in :term:`D` correctly.
>>> +
>>
>> It'd be nice to encourage people to use versioned shared libraries even
>> for binary packages but not sure how to word this here.
> 
> Replace by ``${S}/usr/lib/<library>.<version>.so``? and implicitly encourage it.
> Not sure this is the place to give written advice on that.
> 

It's actually ``${S}/usr/lib/<library>.so.<version>`` :) But would work 
for me. Though considering FILES:${PN} = "/" once inheriting this class, 
whether versioned or not doesn't matter to the packaging step (unlike 
the default values, where .so goes to -dev package).

>>> +.. note::
>>>    
>>> -      SRC_URI = "git://example.com/downloads/somepackage.rpm;branch=main;subpath=${BP}"
>>> +   The extraction of the package passed in :term:`SRC_URI` is not handled by the
>>> +   :ref:`ref-classes-bin-package` class, and is done by the appropriate

s/, and is done/but rather/ ?

>>> +   :ref:`fetcher <bitbake-user-manual/bitbake-user-manual-fetching:fetchers>`
>>> +   depending on the nature of the package.
>>
>> What do you mean by "depending on the nature of the package"?
> 
> I meant the type, like RPM, tarball, deb… I can replace by "type", and add a
> "(RPM, Deb, tarball, …)" in parenthesis afterwards.
> 

I would simply remove "depending on the nature of the package", that's 
already implied? Ah, or we can say depending on the file extension which 
I believe is how the fetcher is actually figuring out how to extract them?

Cheers,
Quentin