[PATCH] classes: decode output data to text

[PATCH] classes: decode output data to text

[Pawel Zalewski]

The default return value from subprocess.check_output is an encoded byte.
The applied fix will decode the value to a string.

Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
---
 meta/classes/fs-uuid.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
index 9b53dfba7a..731ea575bd 100644
--- a/meta/classes/fs-uuid.bbclass
+++ b/meta/classes/fs-uuid.bbclass
@@ -4,7 +4,7 @@
 def get_rootfs_uuid(d):
     import subprocess
     rootfs = d.getVar('ROOTFS')
-    output = subprocess.check_output(['tune2fs', '-l', rootfs])
+    output = subprocess.check_output(['tune2fs', '-l', rootfs], text=True)
     for line in output.split('\n'):
         if line.startswith('Filesystem UUID:'):
             uuid = line.split()[-1]
-- 
2.34.1

Re: [OE-core] [PATCH] classes: decode output data to text

[Richard Purdie]

On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
> The default return value from subprocess.check_output is an encoded byte.
> The applied fix will decode the value to a string.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  meta/classes/fs-uuid.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
> index 9b53dfba7a..731ea575bd 100644
> --- a/meta/classes/fs-uuid.bbclass
> +++ b/meta/classes/fs-uuid.bbclass
> @@ -4,7 +4,7 @@
>  def get_rootfs_uuid(d):
>      import subprocess
>      rootfs = d.getVar('ROOTFS')
> -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
> +    output = subprocess.check_output(['tune2fs', '-l', rootfs], text=True)
>      for line in output.split('\n'):
>          if line.startswith('Filesystem UUID:'):
>              uuid = line.split()[-1]


That looks reasonable, I just wonder how this has worked until now? Why
aren't we seeing errors due to this?

Does it mean we don't have some test coverage? or was there silent
breakage of some kind this fixes?

Cheers,

Richard

Re: [OE-core] [PATCH] classes: decode output data to text

[Pawel Zalewski]

It is not actually being used by default in Yocto ?
This was found in kirkstone.
But regardless, it is wrong and will drop an error.

Kind regards,
Pawel

On Fri, 6 Jan 2023 at 12:06, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
> > The default return value from subprocess.check_output is an encoded byte.
> > The applied fix will decode the value to a string.
> >
> > Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> > ---
> >  meta/classes/fs-uuid.bbclass | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
> > index 9b53dfba7a..731ea575bd 100644
> > --- a/meta/classes/fs-uuid.bbclass
> > +++ b/meta/classes/fs-uuid.bbclass
> > @@ -4,7 +4,7 @@
> >  def get_rootfs_uuid(d):
> >      import subprocess
> >      rootfs = d.getVar('ROOTFS')
> > -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
> > +    output = subprocess.check_output(['tune2fs', '-l', rootfs], text=True)
> >      for line in output.split('\n'):
> >          if line.startswith('Filesystem UUID:'):
> >              uuid = line.split()[-1]
>
>
> That looks reasonable, I just wonder how this has worked until now? Why
> aren't we seeing errors due to this?
>
> Does it mean we don't have some test coverage? or was there silent
> breakage of some kind this fixes?
>
> Cheers,
>
> Richard

Re: [OE-core] [PATCH] classes: decode output data to text

[Alexander Kanavin]

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

Can you please describe steps to reproduce?

Alex

On Fri 6. Jan 2023 at 13.40, Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
wrote:

> It is not actually being used by default in Yocto ?
> This was found in kirkstone.
> But regardless, it is wrong and will drop an error.
>
> Kind regards,
> Pawel
>
> On Fri, 6 Jan 2023 at 12:06, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> >
> > On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
> > > The default return value from subprocess.check_output is an encoded
> byte.
> > > The applied fix will decode the value to a string.
> > >
> > > Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> > > ---
> > >  meta/classes/fs-uuid.bbclass | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/meta/classes/fs-uuid.bbclass
> b/meta/classes/fs-uuid.bbclass
> > > index 9b53dfba7a..731ea575bd 100644
> > > --- a/meta/classes/fs-uuid.bbclass
> > > +++ b/meta/classes/fs-uuid.bbclass
> > > @@ -4,7 +4,7 @@
> > >  def get_rootfs_uuid(d):
> > >      import subprocess
> > >      rootfs = d.getVar('ROOTFS')
> > > -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
> > > +    output = subprocess.check_output(['tune2fs', '-l', rootfs],
> text=True)
> > >      for line in output.split('\n'):
> > >          if line.startswith('Filesystem UUID:'):
> > >              uuid = line.split()[-1]
> >
> >
> > That looks reasonable, I just wonder how this has worked until now? Why
> > aren't we seeing errors due to this?
> >
> > Does it mean we don't have some test coverage? or was there silent
> > breakage of some kind this fixes?
> >
> > Cheers,
> >
> > Richard
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#175583):
> https://lists.openembedded.org/g/openembedded-core/message/175583
> Mute This Topic: https://lists.openembedded.org/mt/95823853/1686489
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

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

Re: [OE-core] [PATCH] classes: decode output data to text

[Pawel Zalewski]

1. inherit fs-uuid in some-recipe
2. set ROOTFS to some-existing-rootfs
3. make call to get_rootfs_uuid somewhere in that recipe ie.
"${@get_rootfs_uuid(d)}"
3. bitbake some-recipe

Pawel

On Fri, 6 Jan 2023 at 12:43, Alexander Kanavin <alex.kanavin@gmail.com> wrote:
>
> Can you please describe steps to reproduce?
>
> Alex
>
> On Fri 6. Jan 2023 at 13.40, Pawel Zalewski <pzalewski@thegoodpenguin.co.uk> wrote:
>>
>> It is not actually being used by default in Yocto ?
>> This was found in kirkstone.
>> But regardless, it is wrong and will drop an error.
>>
>> Kind regards,
>> Pawel
>>
>> On Fri, 6 Jan 2023 at 12:06, Richard Purdie
>> <richard.purdie@linuxfoundation.org> wrote:
>> >
>> > On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
>> > > The default return value from subprocess.check_output is an encoded byte.
>> > > The applied fix will decode the value to a string.
>> > >
>> > > Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
>> > > ---
>> > >  meta/classes/fs-uuid.bbclass | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
>> > > index 9b53dfba7a..731ea575bd 100644
>> > > --- a/meta/classes/fs-uuid.bbclass
>> > > +++ b/meta/classes/fs-uuid.bbclass
>> > > @@ -4,7 +4,7 @@
>> > >  def get_rootfs_uuid(d):
>> > >      import subprocess
>> > >      rootfs = d.getVar('ROOTFS')
>> > > -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
>> > > +    output = subprocess.check_output(['tune2fs', '-l', rootfs], text=True)
>> > >      for line in output.split('\n'):
>> > >          if line.startswith('Filesystem UUID:'):
>> > >              uuid = line.split()[-1]
>> >
>> >
>> > That looks reasonable, I just wonder how this has worked until now? Why
>> > aren't we seeing errors due to this?
>> >
>> > Does it mean we don't have some test coverage? or was there silent
>> > breakage of some kind this fixes?
>> >
>> > Cheers,
>> >
>> > Richard
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#175583): https://lists.openembedded.org/g/openembedded-core/message/175583
>> Mute This Topic: https://lists.openembedded.org/mt/95823853/1686489
>> Group Owner: openembedded-core+owner@lists.openembedded.org
>> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>

Re: [OE-core] [PATCH] classes: decode output data to text

[Alexander Kanavin]

Now I see what's going on. The fs-uuid class contains two functions:
get_rootfs_uuid, and replace_rootfs_uuid. The former is not used
anywhere, but the latter is. Can you introduce get_rootfs_uuid() into
something in oe-core, perhaps it could enhance existing code somewhere
(maybe complement replace_rootfs_uuid usage?), or a test could be made
better?

Otherwise, it's still not tested, and will regress without notice.

Alex

On Fri, 6 Jan 2023 at 14:11, Pawel Zalewski
<pzalewski@thegoodpenguin.co.uk> wrote:
>
> 1. inherit fs-uuid in some-recipe
> 2. set ROOTFS to some-existing-rootfs
> 3. make call to get_rootfs_uuid somewhere in that recipe ie.
> "${@get_rootfs_uuid(d)}"
> 3. bitbake some-recipe
>
> Pawel
>
> On Fri, 6 Jan 2023 at 12:43, Alexander Kanavin <alex.kanavin@gmail.com> wrote:
> >
> > Can you please describe steps to reproduce?
> >
> > Alex
> >
> > On Fri 6. Jan 2023 at 13.40, Pawel Zalewski <pzalewski@thegoodpenguin.co.uk> wrote:
> >>
> >> It is not actually being used by default in Yocto ?
> >> This was found in kirkstone.
> >> But regardless, it is wrong and will drop an error.
> >>
> >> Kind regards,
> >> Pawel
> >>
> >> On Fri, 6 Jan 2023 at 12:06, Richard Purdie
> >> <richard.purdie@linuxfoundation.org> wrote:
> >> >
> >> > On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
> >> > > The default return value from subprocess.check_output is an encoded byte.
> >> > > The applied fix will decode the value to a string.
> >> > >
> >> > > Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> >> > > ---
> >> > >  meta/classes/fs-uuid.bbclass | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
> >> > > index 9b53dfba7a..731ea575bd 100644
> >> > > --- a/meta/classes/fs-uuid.bbclass
> >> > > +++ b/meta/classes/fs-uuid.bbclass
> >> > > @@ -4,7 +4,7 @@
> >> > >  def get_rootfs_uuid(d):
> >> > >      import subprocess
> >> > >      rootfs = d.getVar('ROOTFS')
> >> > > -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
> >> > > +    output = subprocess.check_output(['tune2fs', '-l', rootfs], text=True)
> >> > >      for line in output.split('\n'):
> >> > >          if line.startswith('Filesystem UUID:'):
> >> > >              uuid = line.split()[-1]
> >> >
> >> >
> >> > That looks reasonable, I just wonder how this has worked until now? Why
> >> > aren't we seeing errors due to this?
> >> >
> >> > Does it mean we don't have some test coverage? or was there silent
> >> > breakage of some kind this fixes?
> >> >
> >> > Cheers,
> >> >
> >> > Richard
> >>
> >> -=-=-=-=-=-=-=-=-=-=-=-
> >> Links: You receive all messages sent to this group.
> >> View/Reply Online (#175583): https://lists.openembedded.org/g/openembedded-core/message/175583
> >> Mute This Topic: https://lists.openembedded.org/mt/95823853/1686489
> >> Group Owner: openembedded-core+owner@lists.openembedded.org
> >> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
> >> -=-=-=-=-=-=-=-=-=-=-=-
> >>

Re: [OE-core] [PATCH] classes: decode output data to text

[Richard Purdie]

On Fri, 2023-01-06 at 14:32 +0100, Alexander Kanavin wrote:
> Now I see what's going on. The fs-uuid class contains two functions:
> get_rootfs_uuid, and replace_rootfs_uuid. The former is not used
> anywhere, but the latter is. Can you introduce get_rootfs_uuid() into
> something in oe-core, perhaps it could enhance existing code somewhere
> (maybe complement replace_rootfs_uuid usage?), or a test could be made
> better?
> 
> Otherwise, it's still not tested, and will regress without notice.

I still didn't quite understand this explanation since 
replace_rootfs_uuid() calls get_rootfs_uuid().

The issue is that UUID_PLACEHOLDER isn't used anywhere in core:

UUID_PLACEHOLDER = '<<uuid-of-rootfs>>'

which means that get_rootfs_uuid() is never called.

There are some hints about how this should be used here:

https://git.yoctoproject.org/poky/commit/meta/?id=6d7bcd4df5722f3ccebbbf73361838cb0a5dc0ee

The fix is therefore probably ok but a test of UUID_PLACEHOLDER is
missing.

Looking at the class code, these two functions should be moved to
lib/oe/ as well, we don't need a bbclass for this.

Cheers,

Richard

Re: [OE-core] [PATCH] classes: decode output data to text

[Richard Purdie]

On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
> The default return value from subprocess.check_output is an encoded byte.
> The applied fix will decode the value to a string.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  meta/classes/fs-uuid.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
> index 9b53dfba7a..731ea575bd 100644
> --- a/meta/classes/fs-uuid.bbclass
> +++ b/meta/classes/fs-uuid.bbclass
> @@ -4,7 +4,7 @@
>  def get_rootfs_uuid(d):
>      import subprocess
>      rootfs = d.getVar('ROOTFS')
> -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
> +    output = subprocess.check_output(['tune2fs', '-l', rootfs], text=True)
>      for line in output.split('\n'):
>          if line.startswith('Filesystem UUID:'):
>              uuid = line.split()[-1]

You mentioned running into this on kirkstone. One problem is that
text=True is python 3.7 syntax so whilst this is fine for master,
kirkstone supports older versions of python. 

Cheers,

Richard

Re: [OE-core] [PATCH] classes: decode output data to text

[Pawel Zalewski]

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

That is a good point, looking at the codebase I think that the go-to for
dealing with it is using the call to ".decode('utf8')".

Thanks,
Pawel

On Mon, 16 Jan 2023 at 11:01, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Thu, 2022-12-22 at 10:38 +0000, Pawel Zalewski wrote:
> > The default return value from subprocess.check_output is an encoded byte.
> > The applied fix will decode the value to a string.
> >
> > Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> > ---
> >  meta/classes/fs-uuid.bbclass | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/meta/classes/fs-uuid.bbclass b/meta/classes/fs-uuid.bbclass
> > index 9b53dfba7a..731ea575bd 100644
> > --- a/meta/classes/fs-uuid.bbclass
> > +++ b/meta/classes/fs-uuid.bbclass
> > @@ -4,7 +4,7 @@
> >  def get_rootfs_uuid(d):
> >      import subprocess
> >      rootfs = d.getVar('ROOTFS')
> > -    output = subprocess.check_output(['tune2fs', '-l', rootfs])
> > +    output = subprocess.check_output(['tune2fs', '-l', rootfs],
> text=True)
> >      for line in output.split('\n'):
> >          if line.startswith('Filesystem UUID:'):
> >              uuid = line.split()[-1]
>
> You mentioned running into this on kirkstone. One problem is that
> text=True is python 3.7 syntax so whilst this is fine for master,
> kirkstone supports older versions of python.
>
> Cheers,
>
> Richard
>

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