bitbake-devel.lists.openembedded.org archive mirror
 help / color / mirror / Atom feed
From: Alberto Pianon <alberto@pianon.eu>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc: bitbake-devel@lists.openembedded.org,
	richard.purdie@linuxfoundation.org, jpewhacker@gmail.com,
	carlo@piana.eu
Subject: Re: [bitbake-devel] [PATCH] upstream source tracing: base process (patch 1/3)
Date: Thu, 20 Apr 2023 12:30:40 +0200	[thread overview]
Message-ID: <c0728e2088b793ba5438c4cdde8833f7@pianon.eu> (raw)
In-Reply-To: <20230420105624.12f67d72@booty>

Hello Luca,
thanks for your suggestions, I will definitely try to follow them, 
re-submitting the patches.
Best,
Alberto


Il 2023-04-20 10:56 Luca Ceresoli ha scritto:
> 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


  reply	other threads:[~2023-04-20 10:30 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-20  6:20 [PATCH] upstream source tracing: base process (patch 1/3) alberto
2023-04-20  8:56 ` [bitbake-devel] " Luca Ceresoli
2023-04-20 10:30   ` Alberto Pianon [this message]
2023-04-21  7:15 ` Luca Ceresoli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c0728e2088b793ba5438c4cdde8833f7@pianon.eu \
    --to=alberto@pianon.eu \
    --cc=bitbake-devel@lists.openembedded.org \
    --cc=carlo@piana.eu \
    --cc=jpewhacker@gmail.com \
    --cc=luca.ceresoli@bootlin.com \
    --cc=richard.purdie@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).