file checksums versus SRC_URI globs

file checksums versus SRC_URI globs

[Rasmus Villemoes]

Hi,

We have a recipe that uses

  SRC_URI += "file://somedir/*"

and we have noticed that changing one of the files inside somedir does
not cause the recipe to be rebuilt - in fact, no task hashes are
affected; we discovered this because a clean build worked correctly,
while one that uses an sstate mirror would end up with an old version of
those files on the target.

Digging around, it _seems_ that this should be supported, e.g.

commit bb66113bde5361b869dce2bdaece5b938f077ea8
Author: Richard Purdie <richard.purdie@linuxfoundation.org>
Date:   Sun Apr 6 11:08:22 2014 +0100

    bitbake: fetch2: Fix bug in file checksum generation

However, it also seems that something since then has broken the
checksumming of files specified via a glob pattern. At least if I take
the recipe mentioned in that commit and run "bitbake-dumpsig -t
connman-gnome do_fetch", I get

Variable SRC_URI value is
git://github.com/connectivity/connman-gnome.git
file://0001-Removed-icon-from-connman-gnome-about-applet.patch
 file://null_check_for_ipv4_config.patch            file://images/*
      file://connman-gnome-fix-dbus-interface-name.patch
file://0001-Port-to-Gtk3.patch
...
This task depends on the checksums of files:
[('0001-Removed-icon-from-connman-gnome-about-applet.patch',
'4c1b4d89f92949f34d5c232d85398bb4'),
('connman-gnome-fix-dbus-interface-name.patch',
'51f2ebbc91ec35184876dfdf8aec35fd'),
('null_check_for_ipv4_config.patch',
'a23271e41c9fe81551244d875106af10'), ('0001-Port-to-Gtk3.patch',
'fa8ffcd77c330fd5063c7a7c62dd7c69')]

i.e., none of the files in images/ are mentioned, contrary to what was
apparently the case back in 2014.

Rasmus

Re: [OE-core] file checksums versus SRC_URI globs

[Quentin Schulz]

Hi Rasmus,

On Wed, Jul 01, 2020 at 03:51:19PM +0200, Rasmus Villemoes wrote:
> Hi,
> 
> We have a recipe that uses
> 
>   SRC_URI += "file://somedir/*"
> 

Glob aren't supported. Use "file://somedir/" instead.

Quentin

Re: [OE-core] file checksums versus SRC_URI globs

[Rasmus Villemoes]

On 01/07/2020 16.03, Quentin Schulz wrote:
> Hi Rasmus,
> 
> On Wed, Jul 01, 2020 at 03:51:19PM +0200, Rasmus Villemoes wrote:
>> Hi,
>>
>> We have a recipe that uses
>>
>>   SRC_URI += "file://somedir/*"
>>
> 
> Glob aren't supported. Use "file://somedir/" instead.

Thanks, that actually works for one of the cases we have (there are
others that use globs which cannot be solved quite that simply, but for
now I'm just listing files explicitly instead).

However, I'm not sure that "globs aren't supported". The commit I
referenced clearly tried to make that work (better), it also "works" in
the sense of unpacking the expected things when building from scratch -
there's even

    def test_local_wildcard(self):
        tree = self.fetchUnpack(['file://a', 'file://dir/*'])
        self.assertEqual(tree, ['a',  'dir/c', 'dir/d', 'dir/subdir/e'])

in bitbake/lib/bb/tests/fetch.py. And the two upstream recipes
connman-gnome_0.7.bb and matchbox-desktop_2.2.bb both use that exact
pattern.

So either

- this is a plain bug in the signature computation,
- documentation lacks a big fat warning that using globs in file://
should be avoided unless one has a fetish for tracking down bugs due to
confused sstate, or
- all code that makes glob patterns work should be ripped out so that it
is indeed not supported, thus eliminating this footgun.

Rasmus

Re: [OE-core] file checksums versus SRC_URI globs

[Rasmus Villemoes]

On 02/07/2020 08.42, Rasmus Villemoes via lists.openembedded.org wrote:
> On 01/07/2020 16.03, Quentin Schulz wrote:
>> Hi Rasmus,
>>
>> On Wed, Jul 01, 2020 at 03:51:19PM +0200, Rasmus Villemoes wrote:
>>> Hi,
>>>
>>> We have a recipe that uses
>>>
>>>   SRC_URI += "file://somedir/*"
>>>
>>
>> Glob aren't supported. Use "file://somedir/" instead.
> 
> Thanks, that actually works for one of the cases we have (there are
> others that use globs which cannot be solved quite that simply, but for
> now I'm just listing files explicitly instead).
> 
> However, I'm not sure that "globs aren't supported". The commit I
> referenced clearly tried to make that work (better), it also "works" in
> the sense of unpacking the expected things when building from scratch -
> there's even
> 
>     def test_local_wildcard(self):
>         tree = self.fetchUnpack(['file://a', 'file://dir/*'])
>         self.assertEqual(tree, ['a',  'dir/c', 'dir/d', 'dir/subdir/e'])
> 
> in bitbake/lib/bb/tests/fetch.py. And the two upstream recipes
> connman-gnome_0.7.bb and matchbox-desktop_2.2.bb both use that exact
> pattern.
> 
> So either
> 
> - this is a plain bug in the signature computation,

I'm guessing the culprit is

commit 6c0706a28d72c591f1b75b6e3f3b645859387c7e
Author: Richard Purdie <richard.purdie@linuxfoundation.org>
Date:   Mon Dec 8 21:25:23 2014 +0000

    cache/fetch2/siggen: Ensure we track include history for file checksums

which was the one that introduced the ":True: or ":False" suffixing via

+                filelist.append(f + ":" + str(os.path.exists(f)))

and then also did

@@ -981,6 +980,10 @@ def get_file_checksums(filelist, pn):

     checksums = []
     for pth in filelist.split():
+        exist = pth.split(":")[1]
+        if exist == "False":
+            continue
+        pth = pth.split(":")[0]
         if '*' in pth:
             # Handle globs
             for f in glob.glob(pth):

which practically guaranteed that the "if '*' in pth" would be dead code.

Richard, do you agree that this is a bug in the signature computation?

As I wrote previously, there's no warning anywhere that using globs in
SRC_URI will fail to take the contents of the referenced file into
account in hashes, but there's obviously still a some code that makes
file://*.c work wrt. unpacking (and a test case for that).

Rasmus

Re: [OE-core] file checksums versus SRC_URI globs

[Richard Purdie]

On Thu, 2020-07-09 at 21:31 +0200, Rasmus Villemoes wrote:
> On 02/07/2020 08.42, Rasmus Villemoes via lists.openembedded.org wrote:
> > On 01/07/2020 16.03, Quentin Schulz wrote:
> > > Hi Rasmus,
> > > 
> > > On Wed, Jul 01, 2020 at 03:51:19PM +0200, Rasmus Villemoes wrote:
> > > > Hi,
> > > > 
> > > > We have a recipe that uses
> > > > 
> > > >   SRC_URI += "file://somedir/*"
> > > > 
> > > 
> > > Glob aren't supported. Use "file://somedir/" instead.
> > 
> > Thanks, that actually works for one of the cases we have (there are
> > others that use globs which cannot be solved quite that simply, but for
> > now I'm just listing files explicitly instead).
> > 
> > However, I'm not sure that "globs aren't supported". The commit I
> > referenced clearly tried to make that work (better), it also "works" in
> > the sense of unpacking the expected things when building from scratch -
> > there's even
> > 
> >     def test_local_wildcard(self):
> >         tree = self.fetchUnpack(['file://a', 'file://dir/*'])
> >         self.assertEqual(tree, ['a',  'dir/c', 'dir/d', 'dir/subdir/e'])
> > 
> > in bitbake/lib/bb/tests/fetch.py. And the two upstream recipes
> > connman-gnome_0.7.bb and matchbox-desktop_2.2.bb both use that exact
> > pattern.
> > 
> > So either
> > 
> > - this is a plain bug in the signature computation,
> 
> I'm guessing the culprit is
> 
> commit 6c0706a28d72c591f1b75b6e3f3b645859387c7e
> Author: Richard Purdie <richard.purdie@linuxfoundation.org>
> Date:   Mon Dec 8 21:25:23 2014 +0000
> 
>     cache/fetch2/siggen: Ensure we track include history for file checksums
> 
> which was the one that introduced the ":True: or ":False" suffixing via
> 
> +                filelist.append(f + ":" + str(os.path.exists(f)))
> 
> and then also did
> 
> @@ -981,6 +980,10 @@ def get_file_checksums(filelist, pn):
> 
>      checksums = []
>      for pth in filelist.split():
> +        exist = pth.split(":")[1]
> +        if exist == "False":
> +            continue
> +        pth = pth.split(":")[0]
>          if '*' in pth:
>              # Handle globs
>              for f in glob.glob(pth):
> 
> which practically guaranteed that the "if '*' in pth" would be dead code.
> 
> Richard, do you agree that this is a bug in the signature computation?
> 
> As I wrote previously, there's no warning anywhere that using globs in
> SRC_URI will fail to take the contents of the referenced file into
> account in hashes, but there's obviously still a some code that makes
> file://*.c work wrt. unpacking (and a test case for that).

Sorry about the delay in replying to this, it was flagged in my inbox,
I've just lacked time to investigate and resolve it.

I agree its a bug but my patch above didn't break globing support for
file checksums, its simply never worked even before that change. After
a lot of thought, I've also concluded that I don't think it can work in
a sane way (which is why it was never implemented).

The checksum code needs to know about the files that were searched that
didn't exist as well as those that existed and were checksumed. For the
simple file://xxx/* url that is possible but for more imaginative
globs, its hard. In the file://xxx/* case it may as well be file://xxx/
 in the url which works and is already checksumed correctly.

I've therefore sent out patches to remove the globbing support and
clean up the couple of existing references in OE-Core which are
trivially adapted.

I appreciate this isn't the answer you want but I can't realistically
see how to make this work properly in a sane way that scales. I do
agree the pretence it works is bad though so want to fix that.

Cheers,

Richard

Re: [OE-core] file checksums versus SRC_URI globs

[Rasmus Villemoes]

On 09/07/2020 21.31, Rasmus Villemoes via lists.openembedded.org wrote:
> On 02/07/2020 08.42, Rasmus Villemoes via lists.openembedded.org wrote:
>> On 01/07/2020 16.03, Quentin Schulz wrote:
>>> Hi Rasmus,
>>>
>>> On Wed, Jul 01, 2020 at 03:51:19PM +0200, Rasmus Villemoes wrote:
>>>> Hi,
>>>>
>>>> We have a recipe that uses
>>>>
>>>>   SRC_URI += "file://somedir/*"
>>>>
>>>
>>> Glob aren't supported. Use "file://somedir/" instead.
>>
>> Thanks, that actually works for one of the cases we have (there are
>> others that use globs which cannot be solved quite that simply, but for
>> now I'm just listing files explicitly instead).
>>
>> However, I'm not sure that "globs aren't supported". The commit I
>> referenced clearly tried to make that work (better), it also "works" in
>> the sense of unpacking the expected things when building from scratch -
>> there's even
>>
>>     def test_local_wildcard(self):
>>         tree = self.fetchUnpack(['file://a', 'file://dir/*'])
>>         self.assertEqual(tree, ['a',  'dir/c', 'dir/d', 'dir/subdir/e'])
>>
>> in bitbake/lib/bb/tests/fetch.py. And the two upstream recipes
>> connman-gnome_0.7.bb and matchbox-desktop_2.2.bb both use that exact
>> pattern.
>>
>> So either
>>
>> - this is a plain bug in the signature computation,
> 
> I'm guessing the culprit is
> 
> commit 6c0706a28d72c591f1b75b6e3f3b645859387c7e
> Author: Richard Purdie <richard.purdie@linuxfoundation.org>
> Date:   Mon Dec 8 21:25:23 2014 +0000
> 
>     cache/fetch2/siggen: Ensure we track include history for file checksums
> 
> which was the one that introduced the ":True: or ":False" suffixing via
> 
> +                filelist.append(f + ":" + str(os.path.exists(f)))
> 
> and then also did
> 
> @@ -981,6 +980,10 @@ def get_file_checksums(filelist, pn):
> 
>      checksums = []
>      for pth in filelist.split():
> +        exist = pth.split(":")[1]
> +        if exist == "False":
> +            continue
> +        pth = pth.split(":")[0]
>          if '*' in pth:
>              # Handle globs
>              for f in glob.glob(pth):
> 
> which practically guaranteed that the "if '*' in pth" would be dead code.
> 
> Richard, do you agree that this is a bug in the signature computation?

Richard, ping?

Rasmus