| Message ID | 20221222103829.55230-1-pzalewski@thegoodpenguin.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | classes: decode output data to text | expand |
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
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
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] > -=-=-=-=-=-=-=-=-=-=-=- > >
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]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>
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] > >> -=-=-=-=-=-=-=-=-=-=-=- > >>
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
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
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
>
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]
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(-)