Re: [PATCH v3 2/2] isar-image: refactor do_rootfs()

[Jan Kiszka <jan.kiszka@siemens.com>]

On 08.11.18 08:59, Hombourger, Cedric wrote:
> Hi Jan,
> 
>> Why making this a python function?
> 
> To align with Yocto and ease extension of do_rootfs should we need to (we probably want to have some guidelines for public APIs and one of them should be: make them Python functions because it allows children classes to use the full Python paradigm instead of the somewhat limited shell environment)

That is not the actual reason why Yocto did this. The reason, which would have 
helped me to understand the design, is that it allows downstream to define the 
language of the subfunctions freely. bb.build.exec_func will take care of 
calling the function as shell or as python, whatever the user chose.

But, as the subfunctions are shell code, appending/prepending still requires 
shell. If you do that for the rootfs function, that must now be python, even if 
shell would be more appropriate. Here, it is impossible with bitbake to provide 
freedom. If you want flexibility, you better model this with tasks. That is 
something to consider seriously if there is a hooking use case.

> 
>> What is the reasoning for the chosen split-up?
> 
> If you are referring to the split of do_rootfs() into smaller functions, the primary objective was the reduction of the function complexity. Another candidate for such refactoring is IMO setup_root_file_system(): it's a function doing dozens of little things. It also allows classes inheriting from isar-image to hook themselves at specific points in the do_rootfs flow instead of being limited to pre-processing and post-processing extension points (do_rootfs_prepend / do_rootfs_append).

I'm not against breaking this task up, but the question is where, and also how. 
And that should be driven by foreseeable use cases.

Eg., isar_image_conf_rootfs seems pointless at this stage because there is 
already DISTRO_CONFIG_SCRIPT to do customizations. If we want that thing to be 
extensible (rather than just replaceable), we need something else.

isar_image_gen_fstab is possibly something we do not want users to play with 
because that could easily conflict with what wic does.

> 
>> These are all recipe APIs, so we must be more careful with such declarations.
> 
> You are right. If isar-image-base was viewed as an API, we probably want to highlight the change in the ChangeLog. I can do that.

Once it is clear why and how a user should overload / append to those APIs.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux