[PATCH] Add missing WicError import

[PATCH] Add missing WicError import

[Wadim Klincov]

This patch adds the missing import of `WicError`, making any potential errors when building wic images easier to understand.

Signed-off-by: Wadim Klincov <wadim@klincov.com>
---
 meta/scripts/lib/wic/plugins/isarpluginbase.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/meta/scripts/lib/wic/plugins/isarpluginbase.py b/meta/scripts/lib/wic/plugins/isarpluginbase.py
index 185e2ea..68af220 100644
--- a/meta/scripts/lib/wic/plugins/isarpluginbase.py
+++ b/meta/scripts/lib/wic/plugins/isarpluginbase.py
@@ -8,6 +8,8 @@

 import os

+from wic import WicError
+
 def isar_populate_boot_cmd(rootfs_dir, hdddir):
     # copy all files from rootfs/boot into boot partition
     # no not copy symlinks (ubuntu places them here) because targetfs is fat
--
2.35.1

Re: [PATCH] Add missing WicError import

[Henning Schild]

Am Mon, 14 Mar 2022 10:55:53 +0000
schrieb Wadim Klincov <wadim@klincov.com>:

> This patch adds the missing import of `WicError`, making any
> potential errors when building wic images easier to understand.
> 
> Signed-off-by: Wadim Klincov <wadim@klincov.com>
> ---
>  meta/scripts/lib/wic/plugins/isarpluginbase.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/meta/scripts/lib/wic/plugins/isarpluginbase.py
> b/meta/scripts/lib/wic/plugins/isarpluginbase.py index
> 185e2ea..68af220 100644 ---
> a/meta/scripts/lib/wic/plugins/isarpluginbase.py +++
> b/meta/scripts/lib/wic/plugins/isarpluginbase.py @@ -8,6 +8,8 @@
> 
>  import os
> 
> +from wic import WicError
> +

This is already done in all the plugins we have in tree. The file you
are touching here is "what we need to fork", and we do not need that.
Or does the import in the existing plugins also not work?

In case you need that in your custom plugin, just include the import
into your own plugin. Maybe you do not even need that "base" lib for
your custom plugin. Since it is ... what we need to fork. Not what we
might need for our own plugins.

regards,
Henning

>  def isar_populate_boot_cmd(rootfs_dir, hdddir):
>      # copy all files from rootfs/boot into boot partition
>      # no not copy symlinks (ubuntu places them here) because
> targetfs is fat --
> 2.35.1
> 
> 

Re: [PATCH] Add missing WicError import

[Anton Mikanovich]

16.03.2022 11:24, Henning Schild wrote:
> This is already done in all the plugins we have in tree. The file you
> are touching here is "what we need to fork", and we do not need that.
> Or does the import in the existing plugins also not work?
>
> In case you need that in your custom plugin, just include the import
> into your own plugin. Maybe you do not even need that "base" lib for
> your custom plugin. Since it is ... what we need to fork. Not what we
> might need for our own plugins.
>
> regards,
> Henning

I agree we should add this import of WicError in isarpluginbase.py 
because it
is really used inside it.
Any script using isar_get_filenames() can not care about its internal
implementation and can not even use WicError. So the requirement of 
importing
something not even used looks wrong in that case.

Re: [PATCH] Add missing WicError import

[Wadim Klincov]

On 3/16/22 09:24, Henning Schild wrote:
> Am Mon, 14 Mar 2022 10:55:53 +0000
> schrieb Wadim Klincov <wadim@klincov.com>:
>
>> This patch adds the missing import of `WicError`, making any
>> potential errors when building wic images easier to understand.
>>
>> Signed-off-by: Wadim Klincov <wadim@klincov.com>
>> ---
>>   meta/scripts/lib/wic/plugins/isarpluginbase.py | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/meta/scripts/lib/wic/plugins/isarpluginbase.py
>> b/meta/scripts/lib/wic/plugins/isarpluginbase.py index
>> 185e2ea..68af220 100644 ---
>> a/meta/scripts/lib/wic/plugins/isarpluginbase.py +++
>> b/meta/scripts/lib/wic/plugins/isarpluginbase.py @@ -8,6 +8,8 @@
>>
>>   import os
>>
>> +from wic import WicError
>> +
>
> This is already done in all the plugins we have in tree. The file you
> are touching here is "what we need to fork", and we do not need that.
> Or does the import in the existing plugins also not work?
>
> In case you need that in your custom plugin, just include the import
> into your own plugin. Maybe you do not even need that "base" lib for
> your custom plugin. Since it is ... what we need to fork. Not what we
> might need for our own plugins.
>

Thanks for your reply!

Do you mean `isarpluginbase.py` is something I, as a user, should fork
and change to my liking? Right now using `BootimgEFIPlugin` (or a
slightly modified version for better resolution) works great and it
wasn't clear that it's not something which should be used.

The issue on my side happens when `BootimgEFIPlugin` calls
`isar_get_filenames` and the kernel is not available for whatever
reason. In this case `isar_get_filenames` tries to raise `WicError`, but
since it's not imported directly in `isarpluginbase.py` a bitbake error
is thrown instead. It's still clear that the kernel is not available,
it's just not very readable.

> regards,
> Henning
>
>>   def isar_populate_boot_cmd(rootfs_dir, hdddir):
>>       # copy all files from rootfs/boot into boot partition
>>       # no not copy symlinks (ubuntu places them here) because
>> targetfs is fat --
>> 2.35.1
>>
>>
>
Wadim

Re: [PATCH] Add missing WicError import

[Henning Schild]

Am Wed, 16 Mar 2022 12:03:36 +0300
schrieb Anton Mikanovich <amikan@ilbers.de>:

> 16.03.2022 11:24, Henning Schild wrote:
> > This is already done in all the plugins we have in tree. The file
> > you are touching here is "what we need to fork", and we do not need
> > that. Or does the import in the existing plugins also not work?
> >
> > In case you need that in your custom plugin, just include the import
> > into your own plugin. Maybe you do not even need that "base" lib for
> > your custom plugin. Since it is ... what we need to fork. Not what
> > we might need for our own plugins.
> >
> > regards,
> > Henning  
> 
> I agree we should add this import of WicError in isarpluginbase.py 
> because it
> is really used inside it.

Ah yes, ACK!

Henning

> Any script using isar_get_filenames() can not care about its internal
> implementation and can not even use WicError. So the requirement of 
> importing
> something not even used looks wrong in that case.
> 

Re: [PATCH] Add missing WicError import

[Henning Schild]

Am Wed, 16 Mar 2022 09:12:48 +0000
schrieb Wadim Klincov <wadim@klincov.com>:

> On 3/16/22 09:24, Henning Schild wrote:
> > Am Mon, 14 Mar 2022 10:55:53 +0000
> > schrieb Wadim Klincov <wadim@klincov.com>:
> >  
> >> This patch adds the missing import of `WicError`, making any
> >> potential errors when building wic images easier to understand.
> >>
> >> Signed-off-by: Wadim Klincov <wadim@klincov.com>
> >> ---
> >>   meta/scripts/lib/wic/plugins/isarpluginbase.py | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/meta/scripts/lib/wic/plugins/isarpluginbase.py
> >> b/meta/scripts/lib/wic/plugins/isarpluginbase.py index
> >> 185e2ea..68af220 100644 ---
> >> a/meta/scripts/lib/wic/plugins/isarpluginbase.py +++
> >> b/meta/scripts/lib/wic/plugins/isarpluginbase.py @@ -8,6 +8,8 @@
> >>
> >>   import os
> >>
> >> +from wic import WicError
> >> +  
> >
> > This is already done in all the plugins we have in tree. The file
> > you are touching here is "what we need to fork", and we do not need
> > that. Or does the import in the existing plugins also not work?
> >
> > In case you need that in your custom plugin, just include the import
> > into your own plugin. Maybe you do not even need that "base" lib for
> > your custom plugin. Since it is ... what we need to fork. Not what
> > we might need for our own plugins.
> >  
> 
> Thanks for your reply!
> 
> Do you mean `isarpluginbase.py` is something I, as a user, should fork
> and change to my liking? Right now using `BootimgEFIPlugin` (or a
> slightly modified version for better resolution) works great and it
> wasn't clear that it's not something which should be used.

Ideally nobody should fork anything. Isar had to fork some plugins from
OE, to keep those forks small and maintainable the pluginbase code was
written to keep the isar-version as close as possible to OE.

You hopefully do not have to fork again ... Please contribute your
changes to the EFI plugin back here, or ideally into OE directly.

In case you are writing your complete own isar-/layer-specific wic
plugin, you might not want to use "pluginbase". Since its main goal is
to cater for the OE plugins that isar re-uses in a modified way.

> The issue on my side happens when `BootimgEFIPlugin` calls
> `isar_get_filenames` and the kernel is not available for whatever
> reason. In this case `isar_get_filenames` tries to raise `WicError`,
> but since it's not imported directly in `isarpluginbase.py` a bitbake
> error is thrown instead. It's still clear that the kernel is not
> available, it's just not very readable.

Yes i think your patch is valid. No matter what you might be doing in
terms of forking or writing your own plugins.

regards,
Henning

> > regards,
> > Henning
> >  
> >>   def isar_populate_boot_cmd(rootfs_dir, hdddir):
> >>       # copy all files from rootfs/boot into boot partition
> >>       # no not copy symlinks (ubuntu places them here) because
> >> targetfs is fat --
> >> 2.35.1
> >>
> >>  
> >  
> Wadim
> 

Re: [PATCH] Add missing WicError import

[Wadim Klincov]

On 3/16/22 12:13, Henning Schild wrote:
> Am Wed, 16 Mar 2022 09:12:48 +0000
> schrieb Wadim Klincov <wadim@klincov.com>:
>
>> On 3/16/22 09:24, Henning Schild wrote:
>>> Am Mon, 14 Mar 2022 10:55:53 +0000
>>> schrieb Wadim Klincov <wadim@klincov.com>:
>>>
>>>> This patch adds the missing import of `WicError`, making any
>>>> potential errors when building wic images easier to understand.
>>>>
>>>> Signed-off-by: Wadim Klincov <wadim@klincov.com>
>>>> ---
>>>>    meta/scripts/lib/wic/plugins/isarpluginbase.py | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/meta/scripts/lib/wic/plugins/isarpluginbase.py
>>>> b/meta/scripts/lib/wic/plugins/isarpluginbase.py index
>>>> 185e2ea..68af220 100644 ---
>>>> a/meta/scripts/lib/wic/plugins/isarpluginbase.py +++
>>>> b/meta/scripts/lib/wic/plugins/isarpluginbase.py @@ -8,6 +8,8 @@
>>>>
>>>>    import os
>>>>
>>>> +from wic import WicError
>>>> +
>>>
>>> This is already done in all the plugins we have in tree. The file
>>> you are touching here is "what we need to fork", and we do not need
>>> that. Or does the import in the existing plugins also not work?
>>>
>>> In case you need that in your custom plugin, just include the import
>>> into your own plugin. Maybe you do not even need that "base" lib for
>>> your custom plugin. Since it is ... what we need to fork. Not what
>>> we might need for our own plugins.
>>>
>>
>> Thanks for your reply!
>>
>> Do you mean `isarpluginbase.py` is something I, as a user, should fork
>> and change to my liking? Right now using `BootimgEFIPlugin` (or a
>> slightly modified version for better resolution) works great and it
>> wasn't clear that it's not something which should be used.
>
> Ideally nobody should fork anything. Isar had to fork some plugins from
> OE, to keep those forks small and maintainable the pluginbase code was
> written to keep the isar-version as close as possible to OE.
>
> You hopefully do not have to fork again ... Please contribute your
> changes to the EFI plugin back here, or ideally into OE directly.
>
> In case you are writing your complete own isar-/layer-specific wic
> plugin, you might not want to use "pluginbase". Since its main goal is
> to cater for the OE plugins that isar re-uses in a modified way.
>

Great, then I just misunderstood, thanks a lot for the clarification!

>> The issue on my side happens when `BootimgEFIPlugin` calls
>> `isar_get_filenames` and the kernel is not available for whatever
>> reason. In this case `isar_get_filenames` tries to raise `WicError`,
>> but since it's not imported directly in `isarpluginbase.py` a bitbake
>> error is thrown instead. It's still clear that the kernel is not
>> available, it's just not very readable.
>
> Yes i think your patch is valid. No matter what you might be doing in
> terms of forking or writing your own plugins.
>
> regards,
> Henning
>
>>> regards,
>>> Henning
>>>
>>>>    def isar_populate_boot_cmd(rootfs_dir, hdddir):
>>>>        # copy all files from rootfs/boot into boot partition
>>>>        # no not copy symlinks (ubuntu places them here) because
>>>> targetfs is fat --
>>>> 2.35.1
>>>>
>>>>
>>>
>> Wadim
>>
>

Re: [PATCH] Add missing WicError import

[Anton Mikanovich]

14.03.2022 13:55, Wadim Klincov wrote:
> This patch adds the missing import of `WicError`, making any potential errors when building wic images easier to understand.
>
> Signed-off-by: Wadim Klincov <wadim@klincov.com>

Applied to next, thanks.