Re: [bitbake-devel] [PATCH] upstream source tracing: base process (patch 1/3)

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Alberto,

thanks for your patches!

I'm not going into the details of the implementation but I have a few
comments on formal aspects.

First, I would recommend to remove the "(patch 1/3)" from the subject,
or it will be part of the final commit message when applying the patch
using 'git am'. This is something that won't make sense later on, when
one will look at the git history. 'git format-patch origin/master' will
generate patches with an appropriate header for you, and for patch 3
which is special you can do a manual handling but still keep the
standard format: "[PATCH 3/3] ...commit title..."

On Thu, 20 Apr 2023 08:20:24 +0200
"Alberto Pianon" <alberto@pianon.eu> wrote:

> From: Alberto Pianon <alberto@pianon.eu>
> 
> License compliance, SBoM generation and CVE checking require to be able
> to trace each source file back to its corresponding upstream source. The
> current implementation of bb.fetch2 makes it difficult, especially when
> multiple sources are combined together.
> 
> This series of patches provides a solution to the issue by implementing
> a process that unpacks each SRC_URI element into a temporary directory,
> collects relevant provenance metadata on each source file, moves
> everything to the recipe rootdir, and saves metadata in a JSON file.
> 
> The proposed solution is split into a series of patches, with the first
> patch containing required modifications to fetchers' code and a
> TraceUnpackBase class that implements the process, and the second patch
> implementing the data collecting logic in a separate TraceUnpack
> subclass. The final patch includes test data and test cases to
> demonstrate the solution's efficacy.

Your comment is identical for patches 1 and 2. Instead a commit message
should describe what the single patch/commit does. Being this a pretty
complex patch set you certainly can include a couple lines giving the
big picture, but the bulk of the text should really be about _why_ we
need the patch, and also about _what_ it does in case it is not obvious
from the diff.

> 
> Signed-off-by: Alberto Pianon <alberto@pianon.eu>
> ---
>  bin/bitbake-selftest        |   1 +
>  lib/bb/fetch2/__init__.py   |  57 +++++++-
>  lib/bb/fetch2/crate.py      |   2 +
>  lib/bb/fetch2/gitsm.py      |  24 +++-
>  lib/bb/fetch2/hg.py         |   1 +
>  lib/bb/fetch2/npm.py        |   1 +
>  lib/bb/fetch2/npmsw.py      |  26 +++-
>  lib/bb/fetch2/trace_base.py | 254 ++++++++++++++++++++++++++++++++++++
>  lib/bb/tests/trace_base.py  | 227 ++++++++++++++++++++++++++++++++
>  9 files changed, 584 insertions(+), 9 deletions(-)
>  create mode 100644 lib/bb/fetch2/trace_base.py
>  create mode 100644 lib/bb/tests/trace_base.py
> 
> diff --git a/bin/bitbake-selftest b/bin/bitbake-selftest
> index f25f23b1..6d60a5d2 100755
> --- a/bin/bitbake-selftest
> +++ b/bin/bitbake-selftest
> @@ -31,6 +31,7 @@ tests = ["bb.tests.codeparser",
>           "bb.tests.runqueue",
>           "bb.tests.siggen",
>           "bb.tests.utils",
> +         "bb.tests.trace_base",
>           "bb.tests.compression",
>           "hashserv.tests",
>           "layerindexlib.tests.layerindexobj",
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 1a86d8fd..96a7c7b0 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -27,6 +27,10 @@ import bb.persist_data, bb.utils
>  import bb.checksum
>  import bb.process
>  import bb.event
> +try:
> +    from .trace import TraceUnpack

The trace module is introduced in a later patch, which is not very good
practice as the git history would not be bisectable. Commits should
first introduce basic components, and later commits should start using
them, and in case there is a good reason to not do so, they should be
merged in a single commit.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com