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

["Antonin Godard" <antonin.godard@bootlin.com>]

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.

>> +.. 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
>> +   :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.

>>   
>> -   See the ":ref:`bitbake-user-manual/bitbake-user-manual-fetching:fetchers`" section in the BitBake User Manual for
>> -   more information on supported BitBake Fetchers.
>> +   See the ":ref:`bitbake-user-manual/bitbake-user-manual-fetching:fetchers`"
>> +   section in the BitBake User Manual for more information about supported
>> +   BitBake Fetchers.
>>   
>
> I'm not entirely sure this is relevant here? Or maybe we should update 
> the BitBake docs to reflect which fetcher is extracting 
> packages/tarballs with which option?

Since I mentioned the fetchers above I thought it would be nice to keep, but the
link is already present in the sentence above so I'll remove this paragraph.

Thank you!
Antonin

-- 
Antonin Godard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com