Re: [PATCH v5] fetch/local: verify checksums for file:// urls

[Paul Barker <paul@pbarker.dev>]

Hi Savvas,

Some review comments inline.

On Mon, 2026-09-28 at 22:59 +0200, Savvas Etairidis wrote:
> [YOCTO #9993]
> 
> Local.urldata_init() always cleared needdonestamp, so a checksum given
> in a file:// url (e.g. UNINATIVE_CHECKSUM when UNINATIVE_URL points at
> a local file://) was never verified. Keep the donestamp when the url
> carries a checksum so verify_checksum runs, and make
> rename_bad_checksum() a FetchMethod method that Local overrides as a
> no-op, so a mismatch doesn't rename the user's source file.

This commit message is incredibly terse, and it has changed from v3
without any explanation as to why.

> 
> Based on a patch by Jhonata Poma-Hansen.
> 
> Signed-off-by: Savvas Etairidis <setairidis@gmail.com>
> 
> ---
> changes in v5:
> - Rebased on top of master, added changelog entry
> 
> changes in v4:
> - Rebased on top of master
> 
> changes in v3:
> - Cherry-picked the patch from Jhonata Poma-Hansen
> 
> ---
> ---
>  lib/bb/fetch/__init__.py | 36 ++++++++++++++++++------------------
>  lib/bb/fetch/local.py    | 18 ++++++++++++++++--
>  lib/bb/tests/fetch.py    | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/bb/fetch/__init__.py b/lib/bb/fetch/__init__.py
> index 55ab710f3..a9f0add78 100644
> --- a/lib/bb/fetch/__init__.py
> +++ b/lib/bb/fetch/__init__.py
> @@ -742,7 +742,7 @@ def verify_donestamp(ud, d, origud=None):
>          logger.warning("Checksum mismatch for local file %s\n"
>                         "Cleaning and trying again." % ud.localpath)
>          if os.path.exists(ud.localpath):
> -            rename_bad_checksum(ud, e.checksum)
> +            ud.method.rename_bad_checksum(ud, e.checksum)
>          bb.utils.remove(ud.donestamp)
>      return False
>  
> @@ -775,7 +775,7 @@ def update_stamp(ud, d):
>              logger.warning("Checksum mismatch for local file %s\n"
>                             "Cleaning and trying again." % ud.localpath)
>              if os.path.exists(ud.localpath):
> -                rename_bad_checksum(ud, e.checksum)
> +                ud.method.rename_bad_checksum(ud, e.checksum)
>              bb.utils.remove(ud.donestamp)
>              raise
>  
> @@ -1079,20 +1079,6 @@ def build_mirroruris(origud, mirrors, ld):
>  
>      return uris, uds
>  
> -def rename_bad_checksum(ud, suffix):
> -    """
> -    Renames files to have suffix from parameter
> -    """
> -
> -    if ud.localpath is None:
> -        return
> -
> -    new_localpath = "%s_bad-checksum_%s" % (ud.localpath, suffix)
> -    bb.warn("Renaming %s to %s" % (ud.localpath, new_localpath))
> -    if not bb.utils.movefile(ud.localpath, new_localpath):
> -        bb.warn("Renaming %s to %s failed, grep movefile in log.do_fetch to see why" % (ud.localpath, new_localpath))
> -
> -
>  def try_mirror_url(fetch, origud, ud, ld, check = False):
>      # Return of None or a value means we're finished
>      # False means try another url
> @@ -1163,7 +1149,7 @@ def try_mirror_url(fetch, origud, ud, ld, check = False):
>              logger.warning("Mirror checksum failure for url %s (original url: %s)\nCleaning and trying again." % (ud.url, origud.url))
>              logger.warning(str(e))
>              if os.path.exists(ud.localpath):
> -                rename_bad_checksum(ud, e.checksum)
> +                ud.method.rename_bad_checksum(ud, e.checksum)
>          elif isinstance(e, NoChecksumError):
>              raise
>          else:
> @@ -1479,6 +1465,20 @@ class FetchMethod(object):
>          """
>          return True
>  
> +    def rename_bad_checksum(self, ud, suffix):
> +        """
> +        Rename ud.localpath with the given suffix on checksum mismatch.
> +        Local overrides this to a no-op since its localpath points at the
> +        user's source tree.
> +        """

The docstring for a method explaining how another class overrides this
function is an AI-generated code smell. It makes no sense to document
this here.

> +        if ud.localpath is None:
> +            return
> +
> +        new_localpath = "%s_bad-checksum_%s" % (ud.localpath, suffix)
> +        bb.warn("Renaming %s to %s" % (ud.localpath, new_localpath))
> +        if not bb.utils.movefile(ud.localpath, new_localpath):
> +            bb.warn("Renaming %s to %s failed, grep movefile in log.do_fetch to see why" % (ud.localpath, new_localpath))
> +
>      def verify_donestamp(self, ud, d):
>          """
>          Verify the donestamp file
> @@ -1943,7 +1943,7 @@ class Fetch(object):
>                              logger.warning("Checksum failure encountered with download of %s - will attempt other sources if available" % u)
>                              logger.debug(str(e))
>                              if os.path.exists(ud.localpath):
> -                                rename_bad_checksum(ud, e.checksum)
> +                                ud.method.rename_bad_checksum(ud, e.checksum)
>                          elif isinstance(e, NoChecksumError):
>                              raise
>                          else:
> diff --git a/lib/bb/fetch/local.py b/lib/bb/fetch/local.py
> index dfc5b564c..a92c0d267 100644
> --- a/lib/bb/fetch/local.py
> +++ b/lib/bb/fetch/local.py
> @@ -17,7 +17,7 @@ import os
>  import urllib.request, urllib.parse, urllib.error
>  import bb
>  import bb.utils
> -from   bb.fetch import FetchMethod, FetchError, ParameterError
> +from   bb.fetch import CHECKSUM_LIST, FetchMethod, FetchError, ParameterError
>  from   bb.fetch import logger
>  
>  class Local(FetchMethod):
> @@ -31,11 +31,25 @@ class Local(FetchMethod):
>          # We don't set localfile as for this fetcher the file is already local!
>          ud.basename = os.path.basename(ud.path)
>          ud.basepath = ud.path
> -        ud.needdonestamp = False
> +        # Honor explicit checksum params (UNINATIVE_CHECKSUM, recipe
> +        # ;sha256sum=) by leaving needdonestamp at FetchData's default so
> +        # verify_checksum runs.
> +        name = ud.parm.get("name")
> +        ud.needdonestamp = any(
> +            (name and "%s.%ssum" % (name, a) in ud.parm)
> +            or "%ssum" % a in ud.parm
> +            for a in CHECKSUM_LIST
> +        )

This expression is a bit confusing, can we instead check for the
presence of %s_name attributes set by configure_checksum() calls in
FetchData.__init__()?

The comment can also be simpler, perhaps something like "We need a done
stamp file only if a checksum is provided". We should also separate
things with a blank line if we need a comment before the
ud.needdonestamp assignment.

>          if "*" in ud.path:
>              raise bb.fetch.ParameterError("file:// urls using globbing are no longer supported. Please place the files in a directory and reference that instead.", ud.url)
>          return
>  
> +    def rename_bad_checksum(self, ud, suffix):
> +        # file:// urls point at the user's source tree (recipe-shipped files
> +        # under FILESPATH or an absolute path the user passed in); skip the
> +        # rename so a ChecksumError surfaces without mutating their files.
> +        return

Again the comment can be simpler, perhaps "Don't rename the user's
files".

> +
>      def localpath(self, urldata, d):
>          """
>          Return the local filename of a given url assuming a successful fetch.
> diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
> index 71b0a63fe..60d356fc6 100644
> --- a/lib/bb/tests/fetch.py
> +++ b/lib/bb/tests/fetch.py
> @@ -876,6 +876,36 @@ class FetcherLocalTest(FetcherTest):
>              with self.subTest(striplevel=repr(value)):
>                  self.assertInvalidStriplevel(value)
>  
> +    def test_local_checksum_match(self):
> +        # Correct sha256 must run verify_checksum to completion; donestamp
> +        # lands in DL_DIR.

This comment is also an AI-generated code smell. Please ensure that
you're following our AI-assisted contribution policy [1]. If the
original patch from Jhonata was AI-generated then you need to document
this.

[1]: https://docs.yoctoproject.org/contributor-guide/submit-changes.html#acceptance-of-ai-generated-code

> +        import hashlib
> +        content = b"file:// checksum match test\n"
> +        with open(os.path.join(self.localsrcdir, 'sumfile'), 'wb') as f:
> +            f.write(content)
> +        good = hashlib.sha256(content).hexdigest()
> +        fetcher = bb.fetch.Fetch(['file://sumfile;sha256sum=' + good], self.d)
> +        ud = fetcher.ud[fetcher.urls[0]]
> +        self.assertTrue(ud.needdonestamp)
> +        fetcher.download()
> +        self.assertTrue(os.path.exists(ud.donestamp))

We shouldn't need either of the assertTrue() calls here. The checksum
mismatch case will catch failure to check the checksum, and
fetcher.download() will raise an error if it fails.

> +
> +    def test_local_checksum_mismatch(self):
> +        # Bad sha256 raises ChecksumError without renaming the user's
> +        # source file to the <localpath>_bad-checksum_<sha> sibling.
> +        content = b"file:// checksum mismatch test\n"
> +        srcpath = os.path.join(self.localsrcdir, 'sumfile')
> +        with open(srcpath, 'wb') as f:
> +            f.write(content)
> +        bad = "0" * 64
> +        fetcher = bb.fetch.Fetch(['file://sumfile;sha256sum=' + bad], self.d)
> +        with self.assertRaises(bb.fetch.FetchError):
> +            fetcher.download()
> +        self.assertTrue(os.path.exists(srcpath))
> +        with open(srcpath, 'rb') as f:
> +            self.assertEqual(f.read(), content)

Why are we checking the content of the file? We're just looking for a rename.

> +        self.assertFalse(os.path.exists(srcpath + '_bad-checksum_' + bad))
> +
>      def dummyGitTest(self, suffix):
>          # Create dummy local Git repo
>          src_dir = tempfile.mkdtemp(dir=self.tempdir,

Best regards,

-- 
Paul Barker