[PATCH 1/2] fetch2: local files only in DL_DIR becomes fatal error

[PATCH 1/2] fetch2: local files only in DL_DIR becomes fatal error

[Paulo Neves]

When trying to checksum local files, if a given file is not found
anywhere else than the DL_DIR then this means that the the build is
inconsistent, and unreproducible.

This also means that if the DL_DIR is removed or not available the
build does not know how to fetch the file and will fail. With this
commit we fail earlier and consistently on this condition.

Signed-off-by: Paulo Neves <ptsneves@gmail.com>
---
 lib/bb/fetch2/__init__.py | 4 +++-
 lib/bb/tests/fetch.py     | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index ac557176..5f05278a 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1233,7 +1233,9 @@ def get_checksum_file_list(d):
                 if f.startswith(dl_dir):
                     # The local fetcher's behaviour is to return a path under DL_DIR if it couldn't find the file anywhere else
                     if os.path.exists(f):
-                        bb.warn("Getting checksum for %s SRC_URI entry %s: file not found except in DL_DIR" % (d.getVar('PN'), os.path.basename(f)))
+                        bb.fatal(("Getting checksum for %s SRC_URI entry %s: file not found except in DL_DIR."
+                            " This means there is no way to get the file except for an orphaned copy"
+                            " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
                     else:
                         bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
                 filelist.append(f + ":" + str(os.path.exists(f)))
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index ee41bff4..3ebd9fd7 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -693,6 +693,13 @@ class FetcherLocalTest(FetcherTest):
         flst.sort()
         return flst
 
+    def test_local_checksum_fails_if_only_in_dldir(self):
+        with open(os.path.join(self.dldir, "on_dl_dir"), "wb"):
+            pass
+        self.d.setVar("SRC_URI", "file://on_dl_dir")
+        with self.assertRaises(bb.BBHandledException):
+            bb.fetch.get_checksum_file_list(self.d)
+
     def test_local(self):
         tree = self.fetchUnpack(['file://a', 'file://dir/c'])
         self.assertEqual(tree, ['a', 'dir/c'])
-- 
2.25.1

[PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Paulo Neves]

If the local fetcher was not able to find the file anywhere but it
was included in the SRC_URI for checksumming just make it a fatal
error.
---
 lib/bb/fetch2/__init__.py | 2 +-
 lib/bb/tests/fetch.py     | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 5f05278a..8184b55e 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1237,7 +1237,7 @@ def get_checksum_file_list(d):
                             " This means there is no way to get the file except for an orphaned copy"
                             " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
                     else:
-                        bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
+                        bb.fatal("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
                 filelist.append(f + ":" + str(os.path.exists(f)))
 
     return " ".join(filelist)
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 3ebd9fd7..5b577b06 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -700,6 +700,11 @@ class FetcherLocalTest(FetcherTest):
         with self.assertRaises(bb.BBHandledException):
             bb.fetch.get_checksum_file_list(self.d)
 
+    def test_local_checksum_fails_no_file(self):
+        self.d.setVar("SRC_URI", "file://404")
+        with self.assertRaises(bb.BBHandledException):
+            bb.fetch.get_checksum_file_list(self.d)
+
     def test_local(self):
         tree = self.fetchUnpack(['file://a', 'file://dir/c'])
         self.assertEqual(tree, ['a', 'dir/c'])
-- 
2.25.1

Re: [bitbake-devel] [PATCH 1/2] fetch2: local files only in DL_DIR becomes fatal error

[Richard Purdie]

On Fri, 2022-07-08 at 22:54 +0200, Paulo Neves wrote:
> When trying to checksum local files, if a given file is not found
> anywhere else than the DL_DIR then this means that the the build is
> inconsistent, and unreproducible.
> 
> This also means that if the DL_DIR is removed or not available the
> build does not know how to fetch the file and will fail. With this
> commit we fail earlier and consistently on this condition.
> 
> Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> ---
>  lib/bb/fetch2/__init__.py | 4 +++-
>  lib/bb/tests/fetch.py     | 7 +++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index ac557176..5f05278a 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -1233,7 +1233,9 @@ def get_checksum_file_list(d):
>                  if f.startswith(dl_dir):
>                      # The local fetcher's behaviour is to return a path under DL_DIR if it couldn't find the file anywhere else
>                      if os.path.exists(f):
> -                        bb.warn("Getting checksum for %s SRC_URI entry %s: file not found except in DL_DIR" % (d.getVar('PN'), os.path.basename(f)))
> +                        bb.fatal(("Getting checksum for %s SRC_URI entry %s: file not found except in DL_DIR."
> +                            " This means there is no way to get the file except for an orphaned copy"
> +                            " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
>                      else:
>                          bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
>                  filelist.append(f + ":" + str(os.path.exists(f)))

Did you manage to trigger that error in a real world use case?

I've just been looking at this code and it is horribly old and
outdated. I can't help wonder if we shouldn't do something a bit more
invasive to clean it up a bit more. I suspect it does some of these
things for old/obsolete reasons...

I ask since I'm wondering if anyone ever hits these code paths in a
valid usecase.

Thanks for working on it, we do need to improve some of these things.

Cheers,

Richard

Re: [bitbake-devel] [PATCH 1/2] fetch2: local files only in DL_DIR becomes fatal error

[Paulo Neves]

[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]

On Sat, Jul 9, 2022, 08:52 Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Fri, 2022-07-08 at 22:54 +0200, Paulo Neves wrote:
> > When trying to checksum local files, if a given file is not found
> > anywhere else than the DL_DIR then this means that the the build is
> > inconsistent, and unreproducible.
> >
> > This also means that if the DL_DIR is removed or not available the
> > build does not know how to fetch the file and will fail. With this
> > commit we fail earlier and consistently on this condition.
> >
> > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> > ---
> >  lib/bb/fetch2/__init__.py | 4 +++-
> >  lib/bb/tests/fetch.py     | 7 +++++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> > index ac557176..5f05278a 100644
> > --- a/lib/bb/fetch2/__init__.py
> > +++ b/lib/bb/fetch2/__init__.py
> > @@ -1233,7 +1233,9 @@ def get_checksum_file_list(d):
> >                  if f.startswith(dl_dir):
> >                      # The local fetcher's behaviour is to return a path
> under DL_DIR if it couldn't find the file anywhere else
> >                      if os.path.exists(f):
> > -                        bb.warn("Getting checksum for %s SRC_URI entry
> %s: file not found except in DL_DIR" % (d.getVar('PN'),
> os.path.basename(f)))
> > +                        bb.fatal(("Getting checksum for %s SRC_URI
> entry %s: file not found except in DL_DIR."
> > +                            " This means there is no way to get the
> file except for an orphaned copy"
> > +                            " in DL_DIR.") % (d.getVar('PN'),
> os.path.basename(f)))
> >                      else:
> >                          bb.warn("Unable to get checksum for %s SRC_URI
> entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
> >                  filelist.append(f + ":" + str(os.path.exists(f)))
>
> Did you manage to trigger that error in a real world use case?
>
> I've just been looking at this code and it is horribly old and
> outdated. I can't help wonder if we shouldn't do something a bit more
> invasive to clean it up a bit more. I suspect it does some of these
> things for old/obsolete reasons...
>
> I ask since I'm wondering if anyone ever hits these code paths in a
> valid usecase.
>
> Thanks for working on it, we do need to improve some of these things.
>
> Cheers,
>
> Richard
>

Hey Richard,

I think these codepaths are hit on mostly on bugs(bad uri parsing) or bad
files(bad permissions).

The use case i added on the tests is also clearly bad, but I have seen
builds going on for very long in the state of warning, which becomes fatal
with the patch, because the dl_dir was stable and used by all users. So
expect this patch to increase the reports of latent issues people just
ignored.

Yesterday I also hit this warning organically exactly on the situation of
the test: a directory mentioned on the SRC_URI which did not exist in any
FILESPATHS but somehow existed on the dl_dir. So it is real.

I will investigate the whole default to dl_dir if not found and remove it
if I cannot find a good reason.

Paulo Neves

>

[-- Attachment #2: Type: text/html, Size: 4447 bytes --]

Re: [bitbake-devel] [PATCH 1/2] fetch2: local files only in DL_DIR becomes fatal error

[Richard Purdie]

On Sat, 2022-07-09 at 13:12 +0200, Paulo Neves wrote:
>  
>  
> On 7/9/22 09:19, Paulo Neves wrote:
>  
> >  
> > 
> >  
> > On Sat, Jul 9, 2022, 08:52 Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> >  
> > > On Fri, 2022-07-08 at 22:54 +0200, Paulo Neves wrote:
> > >  > When trying to checksum local files, if a given file is not
> > > found
> > >  > anywhere else than the DL_DIR then this means that the the
> > > build is
> > >  > inconsistent, and unreproducible.
> > >  > 
> > >  > This also means that if the DL_DIR is removed or not available
> > > the
> > >  > build does not know how to fetch the file and will fail. With
> > > this
> > >  > commit we fail earlier and consistently on this condition.
> > >  > 
> > >  > Signed-off-by: Paulo Neves <ptsneves@gmail.com>
> > >  > ---
> > >  >  lib/bb/fetch2/__init__.py | 4 +++-
> > >  >  lib/bb/tests/fetch.py     | 7 +++++++
> > >  >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >  > 
> > >  > diff --git a/lib/bb/fetch2/__init__.py
> > > b/lib/bb/fetch2/__init__.py
> > >  > index ac557176..5f05278a 100644
> > >  > --- a/lib/bb/fetch2/__init__.py
> > >  > +++ b/lib/bb/fetch2/__init__.py
> > >  > @@ -1233,7 +1233,9 @@ def get_checksum_file_list(d):
> > >  >                  if f.startswith(dl_dir):
> > >  >                      # The local fetcher's behaviour is to
> > > return a path under DL_DIR if it couldn't find the file anywhere
> > > else
> > >  >                      if os.path.exists(f):
> > >  > -                        bb.warn("Getting checksum for %s
> > > SRC_URI entry %s: file not found except in DL_DIR" %
> > > (d.getVar('PN'), os.path.basename(f)))
> > >  > +                        bb.fatal(("Getting checksum for %s
> > > SRC_URI entry %s: file not found except in DL_DIR."
> > >  > +                            " This means there is no way to
> > > get the file except for an orphaned copy"
> > >  > +                            " in DL_DIR.") % (d.getVar('PN'),
> > > os.path.basename(f)))
> > >  >                      else:
> > >  >                          bb.warn("Unable to get checksum for
> > > %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'),
> > > os.path.basename(f)))
> > >  >                  filelist.append(f + ":" +
> > > str(os.path.exists(f)))
> > >  
> > >  Did you manage to trigger that error in a real world use case?
> > >  
> > >  I've just been looking at this code and it is horribly old and
> > >  outdated. I can't help wonder if we shouldn't do something a bit
> > > more
> > >  invasive to clean it up a bit more. I suspect it does some of
> > > these
> > >  things for old/obsolete reasons...
> > >  
> > >  I ask since I'm wondering if anyone ever hits these code paths
> > > in a
> > >  valid usecase.
> > >  
> > >  Thanks for working on it, we do need to improve some of these
> > > things.
> > >  
> > >  Cheers,
> > >  
> > >  Richard
> > > 
> > 
> > Hey Richard, 
> > 
> > I think these codepaths are hit on mostly on bugs(bad uri parsing)
> > or bad files(bad permissions).
> > 
> > The use case i added on the tests is also clearly bad, but I have
> > seen builds going on for very long in the state of warning, which
> > becomes fatal with the patch, because the dl_dir was stable and
> > used by all users. So expect this patch to increase the reports of
> > latent issues people just ignored. 
> > 
> > Yesterday I also hit this warning organically exactly on the
> > situation of the test: a directory mentioned on the SRC_URI which
> > did not exist in any FILESPATHS but somehow existed on the dl_dir.
> > So it is real. 
> > 
> > I will investigate the whole default to dl_dir if not found and
> > remove it if I cannot find a good reason.
> > 
> > Paulo Neves 
>  
>  I did some archeology trying to find why DL_DIR has anything to do
> with the local fetcher and the reason probably comes from this[1] and
> this[2]. The commit is about sstate cache and i think it considers
> DL_DIR as valid mirror directory in agreement with
> lib/bb/fetch2/README[2]. 
>  
>  The problem is that [3] from the same README "spec", is either
> contradictory with with [2], senseless, or accurate depending on the
> usage of the local file:// fetcher. For a SRC_URI file, what it is
> senseless to search DL_DIR if we have the FILEPATHS? If it is not at
> hand and only in the DL_DIR then it is irreproducible and contradicts
> [3] (the case this patch argues for).

sstate.bbclass sets DL_DIR to SSTATE_DIR in the context of sstate
downloads. It also puts SSTATE_DIR in FILESPATH which I suspect it did
not back in 2011. I therefore suspect we now have a better fix for the
issue as FILESPATH is set and the change from 2011 is now obsolete for
sstate. Or I hope so anyway!

Cheers,

Richard

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Alexandre Belloni]

Hello,

I believe this is the cause of the following failure:

2022-07-13 08:32:51,511 - oe-selftest - INFO - bbtests.BitbakeTests.test_invalid_recipe_src_uri (subunit.RemotedTestCase)
2022-07-13 08:32:51,512 - oe-selftest - INFO -  ... FAIL
2022-07-13 08:32:51,512 - oe-selftest - INFO - 3: 18/58 119/493 (3.15s) (bbtests.BitbakeTests.test_invalid_recipe_src_uri)
2022-07-13 08:32:51,512 - oe-selftest - INFO - testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/selftest/cases/bbtests.py", line 148, in test_invalid_recipe_src_uri
    bitbake('-ccleanall man-db')
  File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/utils/commands.py", line 229, in bitbake
    return runCmd(cmd, ignore_status, timeout, output_log=output_log, **options)
  File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/utils/commands.py", line 207, in runCmd
    raise AssertionError("Command '%s' returned non-zero exit status %d:\n%s" % (command, result.status, exc_output))
AssertionError: Command 'bitbake  -ccleanall man-db' returned non-zero exit status 1:
/usr/lib64/python3.6/importlib/_bootstrap.py:219: ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__
  return f(*args, **kwds)
NOTE: Reconnecting to bitbake server...
Loading cache...done.
Loaded 1718 entries from dependency cache.
Parsing recipes...ERROR: /home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/recipes-extended/man-db/man-db_2.10.2.bb: Unable to get checksum for man-db SRC_URI entry invalid: file could not be found
ERROR: Parsing halted due to errors, see error messages above

I guess you have to update
bbtests.BitbakeTests.test_invalid_recipe_src_uri too

On 08/07/2022 22:54:07+0200, Paulo Neves wrote:
> If the local fetcher was not able to find the file anywhere but it
> was included in the SRC_URI for checksumming just make it a fatal
> error.
> ---
>  lib/bb/fetch2/__init__.py | 2 +-
>  lib/bb/tests/fetch.py     | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 5f05278a..8184b55e 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -1237,7 +1237,7 @@ def get_checksum_file_list(d):
>                              " This means there is no way to get the file except for an orphaned copy"
>                              " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
>                      else:
> -                        bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
> +                        bb.fatal("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
>                  filelist.append(f + ":" + str(os.path.exists(f)))
>  
>      return " ".join(filelist)
> diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
> index 3ebd9fd7..5b577b06 100644
> --- a/lib/bb/tests/fetch.py
> +++ b/lib/bb/tests/fetch.py
> @@ -700,6 +700,11 @@ class FetcherLocalTest(FetcherTest):
>          with self.assertRaises(bb.BBHandledException):
>              bb.fetch.get_checksum_file_list(self.d)
>  
> +    def test_local_checksum_fails_no_file(self):
> +        self.d.setVar("SRC_URI", "file://404")
> +        with self.assertRaises(bb.BBHandledException):
> +            bb.fetch.get_checksum_file_list(self.d)
> +
>      def test_local(self):
>          tree = self.fetchUnpack(['file://a', 'file://dir/c'])
>          self.assertEqual(tree, ['a', 'dir/c'])
> -- 
> 2.25.1
> 

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#13813): https://lists.openembedded.org/g/bitbake-devel/message/13813
> Mute This Topic: https://lists.openembedded.org/mt/92260795/3617179
> Group Owner: bitbake-devel+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [alexandre.belloni@bootlin.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Paulo Neves]

Hello,

Well this illustrates the kind of issue I wanted to catch, but this 
test's condition will now not be reachable at all.
Previously, the checksum failed with only a warning, while the failure 
occurred in the download stage of the local fetcher.
Now as the checksum fails and it is immediately fatal there is no way to 
reach a download state at all.

Should this test be removed, then? In my patch i added the appropriate 
test and it is in the bitbake self test which does not depend on poky or 
openembedded core. Let me know what you think.

Paulo Neves


On 7/13/22 11:48, Alexandre Belloni wrote:
> Hello,
>
> I believe this is the cause of the following failure:
>
> 2022-07-13 08:32:51,511 - oe-selftest - INFO - bbtests.BitbakeTests.test_invalid_recipe_src_uri (subunit.RemotedTestCase)
> 2022-07-13 08:32:51,512 - oe-selftest - INFO -  ... FAIL
> 2022-07-13 08:32:51,512 - oe-selftest - INFO - 3: 18/58 119/493 (3.15s) (bbtests.BitbakeTests.test_invalid_recipe_src_uri)
> 2022-07-13 08:32:51,512 - oe-selftest - INFO - testtools.testresult.real._StringException: Traceback (most recent call last):
>    File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/selftest/cases/bbtests.py", line 148, in test_invalid_recipe_src_uri
>      bitbake('-ccleanall man-db')
>    File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/utils/commands.py", line 229, in bitbake
>      return runCmd(cmd, ignore_status, timeout, output_log=output_log, **options)
>    File "/home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/lib/oeqa/utils/commands.py", line 207, in runCmd
>      raise AssertionError("Command '%s' returned non-zero exit status %d:\n%s" % (command, result.status, exc_output))
> AssertionError: Command 'bitbake  -ccleanall man-db' returned non-zero exit status 1:
> /usr/lib64/python3.6/importlib/_bootstrap.py:219: ImportWarning: can't resolve package from __spec__ or __package__, falling back on __name__ and __path__
>    return f(*args, **kwds)
> NOTE: Reconnecting to bitbake server...
> Loading cache...done.
> Loaded 1718 entries from dependency cache.
> Parsing recipes...ERROR: /home/pokybuild/yocto-worker/oe-selftest-centos/build/meta/recipes-extended/man-db/man-db_2.10.2.bb: Unable to get checksum for man-db SRC_URI entry invalid: file could not be found
> ERROR: Parsing halted due to errors, see error messages above
>
> I guess you have to update
> bbtests.BitbakeTests.test_invalid_recipe_src_uri too
>
> On 08/07/2022 22:54:07+0200, Paulo Neves wrote:
>> If the local fetcher was not able to find the file anywhere but it
>> was included in the SRC_URI for checksumming just make it a fatal
>> error.
>> ---
>>   lib/bb/fetch2/__init__.py | 2 +-
>>   lib/bb/tests/fetch.py     | 5 +++++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
>> index 5f05278a..8184b55e 100644
>> --- a/lib/bb/fetch2/__init__.py
>> +++ b/lib/bb/fetch2/__init__.py
>> @@ -1237,7 +1237,7 @@ def get_checksum_file_list(d):
>>                               " This means there is no way to get the file except for an orphaned copy"
>>                               " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
>>                       else:
>> -                        bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
>> +                        bb.fatal("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
>>                   filelist.append(f + ":" + str(os.path.exists(f)))
>>   
>>       return " ".join(filelist)
>> diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
>> index 3ebd9fd7..5b577b06 100644
>> --- a/lib/bb/tests/fetch.py
>> +++ b/lib/bb/tests/fetch.py
>> @@ -700,6 +700,11 @@ class FetcherLocalTest(FetcherTest):
>>           with self.assertRaises(bb.BBHandledException):
>>               bb.fetch.get_checksum_file_list(self.d)
>>   
>> +    def test_local_checksum_fails_no_file(self):
>> +        self.d.setVar("SRC_URI", "file://404")
>> +        with self.assertRaises(bb.BBHandledException):
>> +            bb.fetch.get_checksum_file_list(self.d)
>> +
>>       def test_local(self):
>>           tree = self.fetchUnpack(['file://a', 'file://dir/c'])
>>           self.assertEqual(tree, ['a', 'dir/c'])
>> -- 
>> 2.25.1
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#13813): https://lists.openembedded.org/g/bitbake-devel/message/13813
>> Mute This Topic: https://lists.openembedded.org/mt/92260795/3617179
>> Group Owner: bitbake-devel+owner@lists.openembedded.org
>> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [alexandre.belloni@bootlin.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>
>

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Richard Purdie]

On Wed, 2022-07-13 at 12:10 +0200, Paulo Neves wrote:
> Hello,
> 
> Well this illustrates the kind of issue I wanted to catch, but this 
> test's condition will now not be reachable at all.
> Previously, the checksum failed with only a warning, while the failure 
> occurred in the download stage of the local fetcher.
> Now as the checksum fails and it is immediately fatal there is no way to 
> reach a download state at all.
> 
> Should this test be removed, then? In my patch i added the appropriate 
> test and it is in the bitbake self test which does not depend on poky or 
> openembedded core. Let me know what you think.

The test makes sense, we probably just need to remove the        
bitbake('-ccleanall man-db') lines since the issue changed from runtime
to parsing.

Cheers,

Richard

Re: [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

On Fri, Jul 08, 2022 at 10:54:07PM +0200, Paulo Neves wrote:
> If the local fetcher was not able to find the file anywhere but it
> was included in the SRC_URI for checksumming just make it a fatal
> error.
> ---
>  lib/bb/fetch2/__init__.py | 2 +-
>  lib/bb/tests/fetch.py     | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 5f05278a..8184b55e 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -1237,7 +1237,7 @@ def get_checksum_file_list(d):
>                              " This means there is no way to get the file except for an orphaned copy"
>                              " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
>                      else:
> -                        bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
> +                        bb.fatal("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))

Now that we've picked up this change, it seems to have caused a bunch of
irritating hard failures where we use to just get irritating warnings.
Our recipes probably aren't 100% ideal, but we had a number of recipes
which are not pulled into all our machine configs, and could end up with
broken SRC_URIs on machine configs they are not intended to be used on.

There are some more complex examples, but one easy example is a recipe
which had `file://${MACHINE}/eeprom.h` in its SRC_URI[1].  Any machine
which didn't provide this file, even if it never intended to use the
recipe, now fails when we picked up this change.

I know we've been ignoring the warning(s) for a while on these kinds of
failures, so it is our own fault, but it is kind of annoying the new
behavior.  We now have to make sure every recipe not only parses validly
on all machine configs but it also has to have fully populated SRC_URIs
even when the recipe is never used on that machine config.

1. https://github.com/facebook/openbmc/blob/f0d9511ad2fbd563a6b793093cdac557c5ef2a89/meta-facebook/recipes-utils/mac-util/mac-util_0.1.bb#L12
-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Alexander Kanavin]

On Tue, 26 Jul 2022 at 06:09, Patrick Williams <patrick@stwcx.xyz> wrote:
> There are some more complex examples, but one easy example is a recipe
> which had `file://${MACHINE}/eeprom.h` in its SRC_URI[1].  Any machine
> which didn't provide this file, even if it never intended to use the
> recipe, now fails when we picked up this change.

This is not how machine-specific versions of the same file are
supposed to be listed. Just use `file://eeprom.h`, and bitbake will
look for it in subdirectories that include $MACHINE, and if not found
there, a default version will be picked (which you can make bogus, so
builds will fail at recipe build stage instead of parsing stage).

Example:
https://git.yoctoproject.org/poky/tree/meta/recipes-graphics/xorg-xserver/xserver-xf86-config?h=master-next

> I know we've been ignoring the warning(s) for a while on these kinds of
> failures, so it is our own fault, but it is kind of annoying the new
> behavior.  We now have to make sure every recipe not only parses validly
> on all machine configs but it also has to have fully populated SRC_URIs
> even when the recipe is never used on that machine config.
>
> 1. https://github.com/facebook/openbmc/blob/f0d9511ad2fbd563a6b793093cdac557c5ef2a89/meta-facebook/recipes-utils/mac-util/mac-util_0.1.bb#L12

Look, this is bleeding edge. And bleeding edge sometimes bleeds and
breaks your builds. You signed up for it; if you don't like it there,
use stable branches, or use controlled transitions to milestone tags.

Alex

Re: [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Richard Purdie]

On Mon, 2022-07-25 at 23:09 -0500, Patrick Williams wrote:
> On Fri, Jul 08, 2022 at 10:54:07PM +0200, Paulo Neves wrote:
> > If the local fetcher was not able to find the file anywhere but it
> > was included in the SRC_URI for checksumming just make it a fatal
> > error.
> > ---
> >  lib/bb/fetch2/__init__.py | 2 +-
> >  lib/bb/tests/fetch.py     | 5 +++++
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> > index 5f05278a..8184b55e 100644
> > --- a/lib/bb/fetch2/__init__.py
> > +++ b/lib/bb/fetch2/__init__.py
> > @@ -1237,7 +1237,7 @@ def get_checksum_file_list(d):
> >                              " This means there is no way to get the file except for an orphaned copy"
> >                              " in DL_DIR.") % (d.getVar('PN'), os.path.basename(f)))
> >                      else:
> > -                        bb.warn("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
> > +                        bb.fatal("Unable to get checksum for %s SRC_URI entry %s: file could not be found" % (d.getVar('PN'), os.path.basename(f)))
> 
> Now that we've picked up this change, it seems to have caused a bunch of
> irritating hard failures where we use to just get irritating warnings.
> Our recipes probably aren't 100% ideal, but we had a number of recipes
> which are not pulled into all our machine configs, and could end up with
> broken SRC_URIs on machine configs they are not intended to be used on.
> 
> There are some more complex examples, but one easy example is a recipe
> which had `file://${MACHINE}/eeprom.h` in its SRC_URI[1].  Any machine
> which didn't provide this file, even if it never intended to use the
> recipe, now fails when we picked up this change.
> 
> I know we've been ignoring the warning(s) for a while on these kinds of
> failures, so it is our own fault, but it is kind of annoying the new
> behavior.  We now have to make sure every recipe not only parses validly
> on all machine configs but it also has to have fully populated SRC_URIs
> even when the recipe is never used on that machine config.
> 
> 1. https://github.com/facebook/openbmc/blob/f0d9511ad2fbd563a6b793093cdac557c5ef2a89/meta-facebook/recipes-utils/mac-util/mac-util_0.1.bb#L12

I'm just going from memory but it might help to set COMPATIBLE_MACHINE
in the recipe so that it isn't fully parsed for the machines it isn't
intended for.

Cheers,

Richard

Re: [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Paulo Neves]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

That is what I do. If the recipe is machine specific it should be marked as
such.

On Tue, 26 Jul 2022 at 08:39, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Mon, 2022-07-25 at 23:09 -0500, Patrick Williams wrote:
> > On Fri, Jul 08, 2022 at 10:54:07PM +0200, Paulo Neves wrote:
> > > If the local fetcher was not able to find the file anywhere but it
> > > was included in the SRC_URI for checksumming just make it a fatal
> > > error.
> > > ---
> > >  lib/bb/fetch2/__init__.py | 2 +-
> > >  lib/bb/tests/fetch.py     | 5 +++++
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> > > index 5f05278a..8184b55e 100644
> > > --- a/lib/bb/fetch2/__init__.py
> > > +++ b/lib/bb/fetch2/__init__.py
> > > @@ -1237,7 +1237,7 @@ def get_checksum_file_list(d):
> > >                              " This means there is no way to get the
> file except for an orphaned copy"
> > >                              " in DL_DIR.") % (d.getVar('PN'),
> os.path.basename(f)))
> > >                      else:
> > > -                        bb.warn("Unable to get checksum for %s
> SRC_URI entry %s: file could not be found" % (d.getVar('PN'),
> os.path.basename(f)))
> > > +                        bb.fatal("Unable to get checksum for %s
> SRC_URI entry %s: file could not be found" % (d.getVar('PN'),
> os.path.basename(f)))
> >
> > Now that we've picked up this change, it seems to have caused a bunch of
> > irritating hard failures where we use to just get irritating warnings.
> > Our recipes probably aren't 100% ideal, but we had a number of recipes
> > which are not pulled into all our machine configs, and could end up with
> > broken SRC_URIs on machine configs they are not intended to be used on.
> >
> > There are some more complex examples, but one easy example is a recipe
> > which had `file://${MACHINE}/eeprom.h` in its SRC_URI[1].  Any machine
> > which didn't provide this file, even if it never intended to use the
> > recipe, now fails when we picked up this change.
> >
> > I know we've been ignoring the warning(s) for a while on these kinds of
> > failures, so it is our own fault, but it is kind of annoying the new
> > behavior.  We now have to make sure every recipe not only parses validly
> > on all machine configs but it also has to have fully populated SRC_URIs
> > even when the recipe is never used on that machine config.
> >
> > 1.
> https://github.com/facebook/openbmc/blob/f0d9511ad2fbd563a6b793093cdac557c5ef2a89/meta-facebook/recipes-utils/mac-util/mac-util_0.1.bb#L12
>
> I'm just going from memory but it might help to set COMPATIBLE_MACHINE
> in the recipe so that it isn't fully parsed for the machines it isn't
> intended for.
>
> Cheers,
>
> Richard
>
>
>

[-- Attachment #2: Type: text/html, Size: 3736 bytes --]

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 3149 bytes --]

On Tue, Jul 26, 2022 at 07:35:21AM +0200, Alexander Kanavin wrote:
> On Tue, 26 Jul 2022 at 06:09, Patrick Williams <patrick@stwcx.xyz> wrote:
> > There are some more complex examples, but one easy example is a recipe
> > which had `file://${MACHINE}/eeprom.h` in its SRC_URI[1].  Any machine
> > which didn't provide this file, even if it never intended to use the
> > recipe, now fails when we picked up this change.
> 
> This is not how machine-specific versions of the same file are
> supposed to be listed. Just use `file://eeprom.h`, and bitbake will
> look for it in subdirectories that include $MACHINE, and if not found
> there, a default version will be picked (which you can make bogus, so
> builds will fail at recipe build stage instead of parsing stage).
> 
> Example:
> https://git.yoctoproject.org/poky/tree/meta/recipes-graphics/xorg-xserver/xserver-xf86-config?h=master-next

I agree.  The person who originally wrote the recipe didn't know about
the automatic addition of overrides with the FILESEXTRAPATHS.  As I
wrote this was just one easy example.  I fixed this one to both use the
override implicitly (as you also suggested) and put a default 'eeprom.h'
with a `#error`[1].

A seriously more complex example we have is that we have our own u-boot
recipes and used the same `u-boot-foo.inc` file names as are present in
`meta/`.  It happens that the recipes in `meta/recipes-bsp/u-boot` end
up, even though we'd never use them, using our `u-boot-foo.inc` files
and ending up with totally bogus SRC_URIs in the parsing phase (and
thus failing).  Essentially, whenever there is a new version of u-boot
in `meta` we're going to end up having to make some bogus empty
files[2].

> 
> > I know we've been ignoring the warning(s) for a while on these kinds of
> > failures, so it is our own fault, but it is kind of annoying the new
> > behavior.  We now have to make sure every recipe not only parses validly
> > on all machine configs but it also has to have fully populated SRC_URIs
> > even when the recipe is never used on that machine config.
> >
> > 1. https://github.com/facebook/openbmc/blob/f0d9511ad2fbd563a6b793093cdac557c5ef2a89/meta-facebook/recipes-utils/mac-util/mac-util_0.1.bb#L12
> 
> Look, this is bleeding edge. And bleeding edge sometimes bleeds and
> breaks your builds. You signed up for it; if you don't like it there,
> use stable branches, or use controlled transitions to milestone tags.

I agree we signed up for the bleeding edge, partially to be more aware
of changes like this.  Hopefully this wasn't worded too poorly, but I am
not complaining that "things are currently broken for me".  I was trying
to raise awareness that this change isn't as pleasant to downstream layer
maintainers as it might seem on the surface.  If there is an improvement
or revert, great; if there isn't, we'll deal with it.

1. https://github.com/facebook/openbmc/commit/c4e6ffacb73a567c83458f6a1b7e5bb0a032b770
2. https://github.com/facebook/openbmc/tree/c4e6ffacb73a567c83458f6a1b7e5bb0a032b770/meta-aspeed/recipes-bsp/u-boot/files/openbmc-fb
-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Tue, Jul 26, 2022 at 09:01:31AM +0200, Paulo Neves wrote:

> > I'm just going from memory but it might help to set COMPATIBLE_MACHINE
> > in the recipe so that it isn't fully parsed for the machines it isn't
> > intended for.
> That is what I do. If the recipe is machine specific it should be marked as
> such.
> 

Thanks for the info.  At first blush it doesn't sound pleasant to
maintain a list like this for each recipe (since we have over 30
different machines), but we might be able to take a tactical approach on
specific problematic recipes and/or leverage some overrides to better
classify the machine feature that causes the recipe to be necessary.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Alexander Kanavin]

On Tue, 26 Jul 2022 at 17:57, Patrick Williams <patrick@stwcx.xyz> wrote:
> A seriously more complex example we have is that we have our own u-boot
> recipes and used the same `u-boot-foo.inc` file names as are present in
> `meta/`.  It happens that the recipes in `meta/recipes-bsp/u-boot` end
> up, even though we'd never use them, using our `u-boot-foo.inc` files
> and ending up with totally bogus SRC_URIs in the parsing phase (and
> thus failing).  Essentially, whenever there is a new version of u-boot
> in `meta` we're going to end up having to make some bogus empty
> files[2].

Reusing the same filenames to me seems like asking for trouble. It's
really better to name your own u-boot items in a way that is
guaranteed not to get in the way with what items in meta/ look for.
u-boot-openbmc-* or similar would do.

Alex

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Quentin Schulz]

Hi Patrick,

On 7/26/22 17:57, Patrick Williams wrote:
> On Tue, Jul 26, 2022 at 07:35:21AM +0200, Alexander Kanavin wrote:
>> On Tue, 26 Jul 2022 at 06:09, Patrick Williams <patrick@stwcx.xyz> wrote:
>>> There are some more complex examples, but one easy example is a recipe
>>> which had `file://${MACHINE}/eeprom.h` in its SRC_URI[1].  Any machine
>>> which didn't provide this file, even if it never intended to use the
>>> recipe, now fails when we picked up this change.
>>
>> This is not how machine-specific versions of the same file are
>> supposed to be listed. Just use `file://eeprom.h`, and bitbake will
>> look for it in subdirectories that include $MACHINE, and if not found
>> there, a default version will be picked (which you can make bogus, so
>> builds will fail at recipe build stage instead of parsing stage).
>>
>> Example:
>> https://git.yoctoproject.org/poky/tree/meta/recipes-graphics/xorg-xserver/xserver-xf86-config?h=master-next
> 
> I agree.  The person who originally wrote the recipe didn't know about
> the automatic addition of overrides with the FILESEXTRAPATHS.  As I
> wrote this was just one easy example.  I fixed this one to both use the
> override implicitly (as you also suggested) and put a default 'eeprom.h'
> with a `#error`[1].
> 
> A seriously more complex example we have is that we have our own u-boot
> recipes and used the same `u-boot-foo.inc` file names as are present in
> `meta/`.  It happens that the recipes in `meta/recipes-bsp/u-boot` end
> up, even though we'd never use them, using our `u-boot-foo.inc` files
> and ending up with totally bogus SRC_URIs in the parsing phase (and
> thus failing).  Essentially, whenever there is a new version of u-boot
> in `meta` we're going to end up having to make some bogus empty
> files[2].
> 

Alexander already suggested to use a slightly different recipe name (and 
then you can pick which recipe you want from a configuration file with 
PREFERRED_PROVIDER_u-boot = "" IIRC), but I'm actually surprised by the 
behavior you mention seeing.

The original u-boot recipe uses "require" with just the filename, c.f. 
https://cgit.openembedded.org/openembedded-core/tree/meta/recipes-bsp/u-boot/u-boot_2022.07.bb?h=master, 
etc... there's not a single recipes-bsp/u-boot returned by git grep in 
openembedded-core... My understanding of the require mechanism so far is 
that if you pass it a filename, it'll only look into the current 
directory (where the recipe is located) and if it has a path, it'll look 
for that path in all layers (well, BBPATH, which in the majority of 
cases should be LAYERDIR, the root of the layer). Since there is no path 
in require for the u-boot recipes in openmebedded-core, I don't 
understand what you're saying happens happens. c.f. 
https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#require-directive 
for the doc on require directive.

This highlights one of the following:
  - a rogue bbappend or some weird configuration in one the layers in use,
  - a bug/corner-case in require directive,

Maybe there's something to fix or document, hence why I'm writing this 
mail, it'd be good to actually be able to reproduce this and understand 
why it happens.

Cheers,
Quentin

Re: [bitbake-devel] [PATCH 2/2] fetch: bb.fatal when trying to checksum non-existing files.

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Wed, Jul 27, 2022 at 04:50:37PM +0200, Quentin Schulz wrote:
> On 7/26/22 17:57, Patrick Williams wrote:
> > On Tue, Jul 26, 2022 at 07:35:21AM +0200, Alexander Kanavin wrote:
> >> On Tue, 26 Jul 2022 at 06:09, Patrick Williams <patrick@stwcx.xyz> wrote:

> The original u-boot recipe uses "require" with just the filename, c.f. 
> https://cgit.openembedded.org/openembedded-core/tree/meta/recipes-bsp/u-boot/u-boot_2022.07.bb?h=master, 
> etc... there's not a single recipes-bsp/u-boot returned by git grep in 
> openembedded-core... My understanding of the require mechanism so far is 
> that if you pass it a filename, it'll only look into the current 
> directory (where the recipe is located) and if it has a path, it'll look 
> for that path in all layers (well, BBPATH, which in the majority of 
> cases should be LAYERDIR, the root of the layer). Since there is no path 
> in require for the u-boot recipes in openmebedded-core, I don't 
> understand what you're saying happens happens. c.f. 
> https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#require-directive 
> for the doc on require directive.
> 
> This highlights one of the following:
>   - a rogue bbappend or some weird configuration in one the layers in use,

I dug into it a little more.  It is a rogue bbappend in the u-boot case
and not an issue with the require.

We have an internal tree that contains all the u-boot code along side
the recipes, along with a u-boot%.bbappend to replace the external
SRC_URI with an internal ("file://"-based) one.  This gets applied to
the `meta/recipes-bsp` versions even though we don't have copies of
their code.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]